Nested sub ifds parsing fix#2869
Conversation
| do | ||
| { | ||
| this.ReadValues(values, (uint)subIfdOffset); | ||
| ulong[] buf = [.. this.subIfds]; |
There was a problem hiding this comment.
Can this allocation be avoided?
Either by stack-alloc or renting the array from the array-buffer.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good point, high number of subIfd-s is a problem. One more problem is cyclic subifds with cross-reference that perform an infinite loop.
I don't think there is any practical point in having a high number of subIfds.
I think 8 subids and a maximum nesting level of 8 are more than enough limits.
| this.ReadValues(values, (uint)subIfdOffset); | ||
| } | ||
| } | ||
| while (this.subIfds.Count > 0); |
There was a problem hiding this comment.
L195 clears the list (so count = 0). Is this condition necessary then?
There was a problem hiding this comment.
L198 can add to subIfds,
nested sub ifd(s)
|
@IldarKhayrutdinov Is there much more to be done here or are you happy for me to review? |
|
@JimBobSquarePants done |
IldarKhayrutdinov
force-pushed
the
subsub-ifd
branch
from
ff54ca2 to
14aeb3f
Compare
Legend! I’ll review today. |
JimBobSquarePants
left a comment
JimBobSquarePants
left a comment
There was a problem hiding this comment.
This looks great @IldarKhayrutdinov Thanks!
I'll clone and create a matching PR for V3.
4 participants
Prerequisites
Description
Fixes #2857