Skip to content

Conversation

@nikitalita
Copy link
Contributor

@nikitalita nikitalita commented Oct 17, 2025

The bounds check for FileAccessMemory::get_buffer is length - pos, but it is saved to a unsigned integer. This means that if pos is greater than length (which the user can very easily do by seeking past the end), left will underflow and the bounds check will fail, causing a buffer over-read.

Casting the result to an int64_t and clamping it at >= 0 fixes the issue.

@nikitalita nikitalita requested a review from a team as a code owner October 17, 2025 20:38
@nikitalita nikitalita changed the title Fix integer underflow in FileAccessMemory::get_buffer Fix buffer over-read in FileAccessMemory::get_buffer Oct 17, 2025
@BlueCube3310 BlueCube3310 added this to the 4.x milestone Oct 18, 2025
Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

If the user can use seek to seek past the end, shouldn't we insert an error check there instead? Seeking past the end seems like erroneous behavior to me.

@nikitalita
Copy link
Contributor Author

If the user can use seek to seek past the end, shouldn't we insert an error check there instead? Seeking past the end seems like erroneous behavior to me.

It seemed like that to me too, but I didn’t write it and I don’t know what the logic is behind that decision, so I left it. Do you want me to add that?

@Ivorforce
Copy link
Member

I'd prefer that as a solution. FileAccessCompressed seems to behave this way too:

ERR_FAIL_COND(p_position > read_total);

@nikitalita nikitalita force-pushed the fix-fmem-integer-underflow branch from 0c5f475 to 3fa73d4 Compare November 2, 2025 20:42
@Ivorforce Ivorforce modified the milestones: 4.x, 4.6 Nov 2, 2025
@akien-mga akien-mga changed the title Fix buffer over-read in FileAccessMemory::get_buffer Fix buffer over-read in FileAccessMemory::get_buffer Nov 3, 2025
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