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

Fix buffer overflow from scoring vacuum media dose #861

Merged
merged 1 commit into from Jun 27, 2022

Conversation

mxxo
Copy link
Contributor

@mxxo mxxo commented Mar 30, 2022

Vacuum has a special medium number -1, while all other media get offsets
from 0 to nmedia. Before this change, there was a buffer overflow if
media dose scoring was used with a geometry that had vacuum as one of
the media. The vector massM is indexed by medium number, but for any
vacuum regions, it would read and write memory at the memory location
one before the buffer bounds: massM[-1].

Skipping the vacuum region masses shouldn't matter, as vacuum dose is
always zero. massM is only used later on with non-vacuum media, so
skipping vacuum regions is the right choice instead of an alternative
like appending the vacuum mass to massM.

Fixes #860.

Vacuum has a special medium number -1, while all other media get offsets
from 0 to nmedia. Before this change, there was a buffer overflow if
media dose scoring was used with a geometry that had vacuum as one of
the media. The vector massM is indexed by medium number, but for any
vacuum regions, it would read and write memory at the memory location
one before the buffer bounds: massM[-1].

Skipping the vacuum region masses shouldn't matter, as vacuum dose is
always zero. massM is only used later on with non-vacuum media, so
skipping vacuum regions is the right choice instead of an alternative
like appending the vacuum mass to massM.
@mxxo mxxo requested a review from a team as a code owner March 30, 2022 14:57
Copy link
Contributor

@mainegra mainegra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch Max!

@mainegra mainegra added the bug label Mar 30, 2022
@mainegra mainegra self-assigned this Mar 30, 2022
@rtownson
Copy link
Contributor

I believe this is a duplicate of one of the fixes in #719. I.e. egs_advanced_application.cpp the ind < 0 ? 0.0 : was added:

EGS_Float EGS_AdvancedApplication::getMediumRho(int ind) {
    return ind < 0 ? 0.0 : the_media->rho[ind];
}

@mxxo
Copy link
Contributor Author

mxxo commented Mar 30, 2022

@rtownson, thank you for pointing out that issue. I think this stands alone though, since even with the fix to getMediumRho there is still an bug reading and writing to massM[-1], which I don't think is fixed by that linked PR or others.

@ftessier ftessier merged commit efd0221 into nrc-cnrc:develop Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants