Skip to content

fix: prevent crash by using a fallback value with bad std::clamp arguments. #2219

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

Merged
merged 4 commits into from
May 5, 2025

Conversation

ThePirate42
Copy link
Contributor

@ThePirate42 ThePirate42 commented Apr 29, 2025

Problem description

When ImHex is built in such a way that the std has asserts activated, the program crashes if the pattern editor is too short. This is the crash log: imhex.log. As you can see, an assert is triggered in a call to std::clamp.

Implementation description

I'm almost sure the crash is caused by the call to std::clamp in ViewPatternEditor::drawContent(): if textEditorSize.y
is less than 400.0F the min value is greater than the max value, which is undefined behavior. I changed the code so that 200.0F is used as a fallback when that is the case.

Additional things

I didn't build and test the change locally because at the moment I don't have the time.

@ThePirate42 ThePirate42 changed the title Prevent crash by using a fallback value with bad std::clamp arguments. fix: prevent crash by using a fallback value with bad std::clamp arguments. Apr 29, 2025
@paxcut
Copy link
Collaborator

paxcut commented Apr 29, 2025

Some background on the code that this PR aims to change.

For the longest time it was not possible to change the relative sizes of the text editor and the console windows. A movable separator bar was added but it was possible to move the bar so to make one of the two windows disappear together with the separator bar. As a temporary fix for that problem I clamp the size of the text editor window so that its smallest height would be 200 pixels and its largest height would make the console window 200 pixels tall. As long as the imhex window is at least 400 pixels tall, that code will do exactly that job.

You can resize the entire imhex window and make it as short as possible in the y direction, That makes available.y 280 pixels.
When you move the bar as high up as it would go the values of height (the separator bar deviation from the default position) become negative and bigger than the height of the window. For this case the minimum clamping value will be 200 and the maximum is 80. If ImHex is compiled with normal flags the clamp function will always return 80, so the text editor is 80 pixels tall and the console window is 200 pixels tall. The fact that min and max are reversed doesn't prevent the function from returning the correct value.

If you move the separator bar as far low as you can then the value of height becomes positive and larger than 200. Again, using normal compiler flags the function always returns 200. Since the text editor window is 200 pixels tall, the console window becomes 80 pixels tall which is the opposite of the above and again the correct behavior is observed despite the claims of undefined behavior. This is not surprising. Std standards are notorious for not handling the simplest exceptions that sane programmers would implement like swapping min and max values if they are not in the correct order. The purpose of the standard is the creation of a common frame for all programmers, not to dictate what should be implemented and what not.

I implemented and tested the fix presented here and found that it alters the behavior described above by having it return 200 instead of 80 when the bar is at the tallest point making the separator bar unable to move and forcing the console to only show 80 pixels. I could merge the min/max swap if the code is changed to implement it.

That being said please note that the code is meant to be a temporary fix to the window disappearing problem so it is good enough if it just works for now. Among its real problems are the fact that the deviation of the separator bar from its default position is not updated on window resizes which can potentially lock the separator bar depending on how the windows are resized. A more complete solution requires information about available and text editor sizes and for the value of deviation from default of the separator bar across two frames for comparison. Since in a given frame only a window resize or a separator move can occur and not both, then we must detect which ,if any occurred, and update the remaining variables accordingly. This way when the window size is changed, the new value for the separator bar location can be calculated or simply updated if thats what changed. Additionally the maximum and minimum places the separator bar would be allowed to go to should display one line of the text editor or the console and not 80 pixels.

The code to do all this is in the planning stage and will be implemented sometime soon. Sorry about the long winded explanation but I felt it was necessary to explain why the code has this form.

@ThePirate42
Copy link
Contributor Author

ThePirate42 commented Apr 30, 2025

@paxcut
I updated the code according to your instructions.

Std standards are notorious for not handling the simplest exceptions that sane programmers would implement like swapping min and max values if they are not in the correct order. The purpose of the standard is the creation of a common frame for all programmers, not to dictate what should be implemented and what not.

I understand your reasoning, but since different std implementations may (and will) be implemented differently, I think you should be cautious before trusting not standard-guaranteed behavior to be sane.

BTW, I got my version of ImHex from the AUR, and this is how it was compiled. Is there some flag that may be causing this? Because I've been getting other crashes from asserts being triggered here and there in the codebase.

Copy link
Collaborator

@paxcut paxcut left a comment

Choose a reason for hiding this comment

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

ready to merge

@paxcut paxcut merged commit 36eeee5 into WerWolv:master May 5, 2025
18 of 20 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