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

SofQW3 segfault #22194

Merged
merged 6 commits into from
Apr 3, 2018
Merged

SofQW3 segfault #22194

merged 6 commits into from
Apr 3, 2018

Conversation

mducle
Copy link
Member

@mducle mducle commented Mar 25, 2018

Fixes an out-of-bounds index error in FractionalRebinning which causes a segmentation fault when SofQW3 is called with certain parameters.

To test:

Run the following script:

ws = CreateSampleWorkspace(binWidth = 0.75, XMin = 0, XMax = 300, XUnit = 'DeltaE')
ws = ScaleX(ws, -150, "Add")
LoadInstrument(ws, InstrumentName='MARI', RewriteSpectraMap = True)
ws_sqw = SofQW3(ws, '0,10,10', 'Direct', 150, EAxisBinning='60,0.75,90')

And check that no segfault occurs.

Fixes #22191.

Release Notes


Reviewer

Please comment on the following (full description):

Code Review
  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there are changes in the release notes then do they describe the changes appropriately?
Functional Tests
  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

@mducle mducle requested a review from martyngigg March 25, 2018 23:38
@mducle mducle self-assigned this Mar 25, 2018
@mducle mducle added Bug Issues and pull requests that are regressions or would be considered a bug by users (e.g. crashing) Direct Inelastic Issues and pull requests related to direct inelastic Patch Candidate Urgent issues that must be included in a patch following a release labels Mar 25, 2018
@mducle mducle changed the base branch from release-v3.12 to master March 26, 2018 08:26
const size_t nx = x_end - x_start;
const size_t ny = y_end - y_start;
const size_t nx = x_end - x_start + 1;
const size_t ny = y_end - y_start + 1;
const double ll_x(ll.X()), ll_y(ll.Y()), ul_y(ul.Y());
const double lr_x(lr.X()), lr_y(lr.Y()), ur_y(ur.Y());
// Check if there is only a output single bin and inputQ is fully enclosed
Copy link
Member

Choose a reason for hiding this comment

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

Is it correct that the next statement would now only fire if xend==x_start ans y_end==y_start?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doh! I think I completely missed the point. I just looked at the loop in the general code (calcGeneralIntersection) and thought that it meant that when [xy]_start==[xy]_end meant there is only one bin width, and that I had misunderstood what [xy]_end meant. But actually the code before was right.

What [xy]_start == [xy]_end means is that the bin width in that direction is zero, and this was what was causing the segmentation fault.

So, actually we can fix the segfault without changing so much of the code. I'll push another commit to do this...

Copy link
Member

@martyngigg martyngigg left a comment

Choose a reason for hiding this comment

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

@mducle Are there any extra tests that are worth adding here?

Copy link
Member

@martyngigg martyngigg left a comment

Choose a reason for hiding this comment

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

The extra protection avoids the crash when encountering a bin with zero width. Good to go and is safe to include in the patch.

@SimonHeybrock SimonHeybrock merged commit 76415a7 into master Apr 3, 2018
@SimonHeybrock SimonHeybrock deleted the 22191_sofqw3_segfault branch April 3, 2018 06:49
@peterfpeterson peterfpeterson added this to the Release 3.12.1 milestone Apr 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Issues and pull requests that are regressions or would be considered a bug by users (e.g. crashing) Direct Inelastic Issues and pull requests related to direct inelastic Patch Candidate Urgent issues that must be included in a patch following a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SofQW3 segfault
4 participants