Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid memory accesses in calc_coupling_elem_spin0and2_pure? #80

Closed
mreineck opened this issue Oct 25, 2023 · 2 comments · Fixed by #82
Closed

Invalid memory accesses in calc_coupling_elem_spin0and2_pure? #80

mreineck opened this issue Oct 25, 2023 · 2 comments · Fixed by #82

Comments

@mreineck
Copy link

While running benchmarks on the code I noticed a block in hte Fortran sources that looks dangerous to me: https://github.com/simonsobs/pspy/blob/1d5d8734f489c6a15fe6ee53d8d61b25478fd9b4/pspy/mcm_fortran/mcm_fortran.f90#L77C1-L86C1

        if (i2 < 0) then
            fac_b = 0d0
        end if

        if (i3 < 0) then
            fac_c = 0d0
        end if

        combin = thrcofa(i1) + fac_b * thrcofb(i2) + fac_c * thrcofc(i3)

Here, i2 and i3 can obviously take values below 1, but they are used to access the arrays thrcofb and thrcofc whose first index is 1. This is undefined behaviour and can cause segmentation faults.
Even if this currently doesn't cause any issues, I'd still suggest to avoid these accesses, since they will make debugging of potential other problems much harder.

@xgarrido
Copy link
Collaborator

I think a correct way to handle these cases is the following

        if (i2 < 1) then
            combin2 = 0d0
         else
            combin2 = fac_b * thrcofb(i2)
        end if

        if (i3 < 1) then
            combin3 = 0d0
         else
            combin3 = fac_c * thrcofc(i3)
        end if

        combin = thrcofa(i1) + combin2 + combin3

where combin2 and combin3 are real(8). I've added a test for pure BB mode with comparisons with NaMaster (see PR #82). The above code (not yet implemented in PR #82) is still "physically" correct but prevent access to undefined array values. Does it look fine for you ?

@mreineck
Copy link
Author

Looks great to me, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants