Fix subtle bug in optimize_ops that could seg fault.#651
Merged
lgritz merged 1 commit intoAcademySoftwareFoundation:masterfrom Jun 30, 2016
Merged
Fix subtle bug in optimize_ops that could seg fault.#651lgritz merged 1 commit intoAcademySoftwareFoundation:masterfrom
lgritz merged 1 commit intoAcademySoftwareFoundation:masterfrom
Conversation
Contributor
|
LGTM Good catch! Also surprised that we didn't run into this earlier ... |
The optimize_ops method had a convenience reference to the op being examined in the loop. We carefully do a reserve() first to make sure that there was enough room for one insertion so that the reference would still be valid if the constant-folding function has to add an op. But this relied on the assumption that no folder would insert more than one op as it did its work, which was always true (once). Oops, much later, a new way to speed up "mix" ops had a particular corner case in which up to 7 new ops might be inserted, and I never went back and fixed the now-broken assumption here. The fix here is to change the reference to the op (which cannot be changed dynamically) into a pointer to the op, which can be re-pointed after calling the folding function to make sure that it's valid and pointing to the (potentially new) correct position as it continues on in the loop. So it is no longer sensitive to changes to the vector allocation that might happen inside the folder. I haven't changed this code for a long time. It's shocking that no problems cropped up from this bug until this week! I guess it's not a very common "corner case".
lgritz
added a commit
to lgritz/OpenShadingLanguage
that referenced
this pull request
Jul 2, 2016
…Foundation#651) The optimize_ops method had a convenience reference to the op being examined in the loop. We carefully do a reserve() first to make sure that there was enough room for one insertion so that the reference would still be valid if the constant-folding function has to add an op. But this relied on the assumption that no folder would insert more than one op as it did its work, which was always true (once). Oops, much later, a new way to speed up "mix" ops had a particular corner case in which up to 7 new ops might be inserted, and I never went back and fixed the now-broken assumption here. The fix here is to change the reference to the op (which cannot be changed dynamically) into a pointer to the op, which can be re-pointed after calling the folding function to make sure that it's valid and pointing to the (potentially new) correct position as it continues on in the loop. So it is no longer sensitive to changes to the vector allocation that might happen inside the folder. I haven't changed this code for a long time. It's shocking that no problems cropped up from this bug until this week! I guess it's not a very common "corner case".
ElaraFX
pushed a commit
to ElaraFX/OpenShadingLanguage
that referenced
this pull request
Jan 28, 2017
…Foundation#651) The optimize_ops method had a convenience reference to the op being examined in the loop. We carefully do a reserve() first to make sure that there was enough room for one insertion so that the reference would still be valid if the constant-folding function has to add an op. But this relied on the assumption that no folder would insert more than one op as it did its work, which was always true (once). Oops, much later, a new way to speed up "mix" ops had a particular corner case in which up to 7 new ops might be inserted, and I never went back and fixed the now-broken assumption here. The fix here is to change the reference to the op (which cannot be changed dynamically) into a pointer to the op, which can be re-pointed after calling the folding function to make sure that it's valid and pointing to the (potentially new) correct position as it continues on in the loop. So it is no longer sensitive to changes to the vector allocation that might happen inside the folder. I haven't changed this code for a long time. It's shocking that no problems cropped up from this bug until this week! I guess it's not a very common "corner case".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The optimize_ops method had a convenience reference to the op being
examined in the loop. We carefully do a reserve() first to make sure
that there was enough room for one insertion so that the reference would
still be valid if the constant-folding function has to add an op. But
this relied on the assumption that no folder would insert more than one
op as it did its work, which was always true (once).
Oops, much later, a new way to speed up "mix" ops had a particular corner
case in which up to 7 new ops might be inserted, and I never went back
and fixed the now-broken assumption here.
The fix here is to change the reference to the op (which cannot be
changed dynamically) into a pointer to the op, which can be re-pointed
after calling the folding function to make sure that it's valid and
pointing to the (potentially new) correct position as it continues on in
the loop. So it is no longer sensitive to changes to the vector
allocation that might happen inside the folder.
I haven't changed this code for a long time. It's shocking that no
problems cropped up from this bug until this week! I guess it's not a
very common "corner case".