Skip to content

Fix GH-10834: exif_read_data() cannot read smaller stream wrapper chunk sizes #10837

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

Closed
wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

Exif uses php_stream_read() to read data from the stream and process it. In this case, it's a userspace stream. Userspace streams can either by local or url streams.

In 5060fc2 and subsequently 0a45e8f, the behaviour of the check was changed twice in total. In the latter commit the description talks about exceptions for streams that are local to the process. It looks like the exception for local userspace streams was forgotten. This patch updates the check such that local userspace streams also read greedily, while keeping url userspace streams unchanged.

This also updates the existing test to test both local and url userspace streams.

Targeting master because I think this could be a breaking change.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Far from an EXIF expert but this looks sensible to me, maybe @sgolemon wants to have a look at it.

…chunk sizes

Exif uses php_stream_read() to read data from the stream and process it.
In this case, it's a userspace stream. Userspace streams can either by
local or url streams.

In 5060fc2 and subsequently 0a45e8f, the behaviour of the
check was changed twice in total. In the latter commit the description
talks about exceptions for streams that are local to the process. It
looks like the exception for local userspace streams was forgotten. This
patch updates the check such that local userspace streams also read
greedily, while keeping url userspace streams unchanged.

This also updates the existing test to test both local and url userspace
streams.
@nielsdos
Copy link
Member Author

Thanks to you both for the review. I fixed the comments.

@bukka
Copy link
Member

bukka commented Mar 17, 2023

I'm not sure that this is a good idea as it might break some user space wrappers that expect to not be greedily read. I don't exactly see the complete correlation between url wrapper and greedy reading. For sure if it's URL one it should not read greedily but why non url ones needs to be read greedily?

I think the fix should really happen in exif to handle non greedily read streams which would be also functionality improvement as you could just provide image URL and it would work (assuming that it is not currently supported when there's an issue with non greedy reading).

@nielsdos
Copy link
Member Author

Hmm I see.
I got the idea for url greedy vs non greedy from past commits, but I see why this is probably a bad idea.
I can look into making exif behave nicely with nongreedy streams. The same issue might also apply to other extensions though, and not only exif. So we might need to check all uses of stream reading in extensions. This is also what lead me to make the change in streams instead of exif.

@nielsdos nielsdos closed this Mar 17, 2023
@bukka
Copy link
Member

bukka commented Mar 17, 2023

As a side note I think it would make sense to have that information (whether it uses greedy and non greedy read) available in the stream struct so it can be checked elsewhere (e.g. in exif). That could be even set during the stream creation so we don't have that ugly ops check in the read that lists all greedy stream ops (it would be just a simple field check).

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.

exif_read_data() cannot read smaller stream wrapper chunk sizes
4 participants