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

check for number of scanlines read from jpeg to prevent integer underflow #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

the-shank
Copy link

jpeg_read_scanlines() can read less lines that requested in certain conditions (ref). When it returns without reading any rows, the output_scanline can be 0 (refer here). When this happens, it can lead to a integer underflow here.

This patch adds check for the return value of jpeg_read_scanlines() and in case the return value is 0, it just skips that loop and continues.

@Talinx
Copy link
Owner

Talinx commented Apr 9, 2023

I don't think that this problem can occur here. According to the libjpeg-turbo docs jpeg_read_scanlines() is guaranteed to read at least one scanline unless a suspending data source is used (which jp2a does not). The doc also implies that a buffer size of 1 (as used here) will always be fully filled with only one call to jpeg_read_scanlines() while larger buffer sizes necessitate checks/multiple calls.

Do you have an example that does not work currently but would with your code change?

@the-shank
Copy link
Author

We found this as a part of our research study on Library API Misuse (handling of return values from library calls, etc.). We don't have a specific example available.

As stated by you, jp2a does not use a suspending data source, and so this is infeasible as of now. Would it be ok to add an assert to check the return value of jpeg_read_scanlines() so that if it ever changes, it could be flagged and attended to promptly.

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