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 OOB access in _CalcCircleAutoSegmentCount #6657

Closed

Conversation

EggsyCRO
Copy link
Contributor

An OOB (out-of-bounds) access can happen inside of ImDrawList::_CalcCircleAutoSegmentCount, given a sufficiently large radius value (of approximately 2,147,483,584 or higher).

This will happen when attempting to index into the CircleSegmentCounts field using an invalid radius_idx value.

This bug can be reproduced by running the following code:
DrawList->AddCircle(ImVec2(100, 100), FLT_MAX, 0xFF0000FF);

An OOB (out-of-bounds) access can happen inside of _CalcCircleAutoSegmentCount, given a sufficiently large radius value (approximately 2,147,483,584 or higher).
ocornut pushed a commit that referenced this pull request Aug 6, 2023
@ocornut
Copy link
Owner

ocornut commented Aug 6, 2023

Merged as 52587c2, thank you !
Note this is the same as #5317 which was lacking a repro and at the time other changes were made so I wasn't sure it was necessary. I'm honestly not too fond of excessive range-checking but should be ok here, expecially as the OOB wouldn't otherwise lead to an runtime assert.

@jdpatdiscord
Copy link

Hi I just wanted to note that my not being able to reproduce it was not due to it not being reproducible; it was, however I had been gone from that job and therefore codebase. Little code stinks like this eventually crop up, don't they? Sorry for the confusion at the time.

@ocornut
Copy link
Owner

ocornut commented Aug 6, 2023

I absolutely understand, didn't mean to suggest there was a grudge or anything but I tend to need the clear explanation/repro to only add checks when they are meaningful and for known reasons.

I've been bitten in the past by codebases with excessive amount of "safety checks", the problem if you start checking for too many things (e.g. NULL pointer checks everywhere) it that they propagate automatically and no-one knows if a check if meaningful anymore. I'd rather enforce doing checks in a minimum amount of places (e.g. API entry points), which in turns allows to provide more explicit and stronger contracts (e.g. this pointer is going to be valid).

In your specific case the confusion came with the fact I did made related changes in that area of the code, so there was an assumption that the issue could have been fixed already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants