Open ziriax opened 3 years ago
Actually, the ifdoffset
can also be negative... I guess more checks should be added. At least
if (ifdoffset < 0 || ifdoffset > header.Length - 1 || ifdoffset + 2 > header.Length)
See attached image that reproduces this.
I also notice that you are converting uint
to int
in a lot of places. Maybe it would be wiser to keep uint
where possible, so negative values cannot occur, not sure.
Also, the line int totallength = (int)(count * baselength);
can cause an overflow.
It would be better to use a long
to store this.
Otherwise the line else if (totallength > int.MaxValue)
has no effect.
I just encountered this same issue in a product I maintain; I was about to file a new issue and stumbled across this one. I'm going to file a PR to fix the first two things @Ziriax mentioned (moving the ushort fieldcount = ...
line to below the if
, and adding a negative check) as a start.
The issue is more severe I'm afraid. conv.ToUInt16
etc is called in many many places without checking the offset. A better solution would be to have these methods return a nullable type, and then force a check on each. Or, maybe easier, throw a specified exception in this converter...
This line of code should be moved to before the for loop
Otherwise
conv.ToUInt16(header, ifdoffset);
crashes and no more metadata is read.So the code should be
I can provide a PR if you want, but that is most likely more overhead than just moving the line; unless you also want unit tests for this of course...