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

Remove NCrystal_sample.comp possible out-of-bounds memory access #1821

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

g5t
Copy link
Contributor

@g5t g5t commented Jan 23, 2025

memset(&params,0,sizeof(geoparams)) would cause an out-of-bounds memory error if sizeof(geoparams) > sizeof(params); and anyway does not appear to do what is intended.

`memset(&params,0,sizeof(geoparams))` would cause an out-of-bounds memory error if `sizeof(geoparams)` > `sizeof(params)`; and anyway does not appear to do what is intended.
@willend
Copy link
Contributor

willend commented Jan 23, 2025

@g5t thanks. Could you have a look if the related union/NCrystal_process.comp may have similar potential issues?

I will consider this PR a work in progress until you 'say when' - OK?

@g5t
Copy link
Contributor Author

g5t commented Jan 23, 2025

NCrystal_process only zeros the params structure (not surprising, since the geometry is handled elsewhere in Union)

memset(&params,0,sizeof(params));

And, as far as I could find, memset is not used in any other components. So this could be merged safely.

(Alternatively, dropping the second memset is probably fine since a) it wasn't being zeroed before anyway and b) the call to ncrystalsample_initgeom on the next line will either initialize all fields of the struct or cause an error due to mismatched component parameters.)

@willend
Copy link
Contributor

willend commented Jan 23, 2025

Interestingly the patch still doesn't seem to fix the intermittent issue/failed test with MPI on macOS. Will merge anyhow.

@willend willend merged commit 729d517 into main Jan 23, 2025
31 of 32 checks passed
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 this pull request may close these issues.

2 participants