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

Nested sub ifds parsing fix #2869

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

IldarKhayrutdinov
Copy link
Contributor

@IldarKhayrutdinov IldarKhayrutdinov commented Jan 27, 2025

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

  • Fix for parsing multiple nested sub ifds
  • Fix for skipping duplicated tags

Fixes #2857

{
this.ReadValues(values, (uint)subIfdOffset);
ulong[] buf = [.. this.subIfds];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this allocation be avoided?
Either by stack-alloc or renting the array from the array-buffer.

Copy link
Contributor Author

@IldarKhayrutdinov IldarKhayrutdinov Jan 27, 2025

Choose a reason for hiding this comment

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

reverted by 40b7be8 because a build failure https://github.com/SixLabors/ImageSharp/actions/runs/12982387631/job/36202132583,
I haven't found a beautiful solution yet

this.subIfds.lenght is almost always a small number: 1,2, <5

Copy link
Member

Choose a reason for hiding this comment

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

You can stackalloc an ulong[128] working buffer outside the loop, then in the loop slice it down if sz <= 128 or allocate an array otherwise.

Copy link
Contributor Author

@IldarKhayrutdinov IldarKhayrutdinov Jan 28, 2025

Choose a reason for hiding this comment

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

@antonfirsov I forgot to mention that the loop body is executed almost always 1 time,
or frequently it is not even executed at all (subIfds==null),
the task file is the only file where there are 4 loop iterations.

Copy link
Member

@antonfirsov antonfirsov Jan 31, 2025

Choose a reason for hiding this comment

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

almost always 1 time

Is there any practical limit on the maximum number of subIfd-s? CA2014: Potential stack overflow. is a valid static analyzer finding if a malicious actor can construct a file with a high number of subIfd-s. We need to prepare the code for such edge-cases while optimizing it for the sane ones.

}
while (this.subIfds.Count > 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

L195 clears the list (so count = 0). Is this condition necessary then?

Copy link
Contributor Author

@IldarKhayrutdinov IldarKhayrutdinov Jan 27, 2025

Choose a reason for hiding this comment

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

L198 can add to subIfds,
nested sub ifd(s)

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.

BaseExifReader.ReadSubIfd throws InvalidOperationException with "Collection was modified"
3 participants