Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/ImageSharp/Metadata/Profiles/Exif/ExifReader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -189,10 +189,16 @@ protected void ReadSubIfd(List<IExifValue> values)
{
if (this.subIfds is not null)
{
foreach (ulong subIfdOffset in this.subIfds)
do
{
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.subIfds.Clear();
foreach (ulong subIfdOffset in buf)
{
this.ReadValues(values, (uint)subIfdOffset);
}
}
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)

}
}

Expand Down Expand Up @@ -447,6 +453,7 @@ private void ReadValue64(List<IExifValue> values, Span<byte> offsetBuffer)
ExifTagValue.TileByteCounts => new ExifLong8Array(ExifTagValue.TileByteCounts),
_ => ExifValues.Create(tag) ?? ExifValues.Create(tag, dataType, numberOfComponents),
};

if (exifValue is null)
{
this.AddInvalidTag(new UnkownExifTag(tag));
Expand Down Expand Up @@ -481,8 +488,9 @@ private void Add(IList<IExifValue> values, ExifValue exif, object? value)

foreach (IExifValue val in values)
{
// Sometimes duplicates appear, can compare val.Tag == exif.Tag
if (val == exif)
// to skip duplicates must be used Equals method,
// == operator not defined for ExifValue and IExifValue
if (exif.Equals(val))
{
Debug.WriteLine($"Duplicate Exif tag: tag={exif.Tag}, dataType={exif.DataType}");
return;
Expand Down
42 changes: 40 additions & 2 deletions tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Metadata.cs
Original file line number Diff line number Diff line change
Expand Up @@ -439,9 +439,8 @@ public void JpegDecoder_DecodeMetadataComment<TPixel>(TestImageProvider<TPixel>
Assert.Equal(expectedComment, metadata.Comments.ElementAtOrDefault(0).ToString());
image.DebugSave(provider);
image.CompareToOriginal(provider);

}

// https://github.com/SixLabors/ImageSharp/issues/2758
[Theory]
[WithFile(TestImages.Jpeg.Issues.Issue2758, PixelTypes.L8)]
Expand All @@ -468,6 +467,45 @@ public void Issue2758_DecodeWorks<TPixel>(TestImageProvider<TPixel> provider)
image.Save(ms, new JpegEncoder());
}

// https://github.com/SixLabors/ImageSharp/issues/2857
[Theory]
[WithFile(TestImages.Jpeg.Issues.Issue2857, PixelTypes.Rgb24)]
public void Issue2857_SubSubIfds<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
using Image<TPixel> image = provider.GetImage(JpegDecoder.Instance);

Assert.Equal(5616, image.Width);
Assert.Equal(3744, image.Height);

JpegMetadata meta = image.Metadata.GetJpegMetadata();
Assert.Equal(92, meta.LuminanceQuality);
Assert.Equal(93, meta.ChrominanceQuality);

ExifProfile exifProfile = image.Metadata.ExifProfile;
Assert.NotNull(exifProfile);

using MemoryStream ms = new();
bool hasThumbnail = exifProfile.TryCreateThumbnail(out _);
Assert.False(hasThumbnail);

Assert.Equal("BilderBox - Erwin Wodicka / [email protected]", exifProfile.GetValue(ExifTag.Copyright).Value);
Assert.Equal("Adobe Photoshop CS3 Windows", exifProfile.GetValue(ExifTag.Software).Value);

Assert.Equal("Carers; seniors; caregiver; senior care; retirement home; hands; old; elderly; elderly caregiver; elder care; elderly care; geriatric care; nursing home; age; old age care; outpatient; needy; health care; home nurse; home care; sick; retirement; medical; mobile; the elderly; nursing department; nursing treatment; nursing; care services; nursing services; nursing care; nursing allowance; nursing homes; home nursing; care category; nursing class; care; nursing shortage; nursing patient care staff\0", exifProfile.GetValue(ExifTag.XPKeywords).Value);

Assert.Equal(
new EncodedString(EncodedString.CharacterCode.ASCII, "StockSubmitter|Miscellaneous||Miscellaneous$|00|0000330000000110000000000000000|22$@NA_1005010.460@[email protected]$$@$@26$$@$@$@$@205$@$@$@$@$@$@$@$@$@43$@$@$@[email protected]$$@90$$@22$@$@$@$@$@$@$|||"),
exifProfile.GetValue(ExifTag.UserComment).Value);

// the profile contains 4 duplicated UserComment
Assert.Equal(1, exifProfile.Values.Count(t => t.Tag == ExifTag.UserComment));

image.Mutate(x => x.Crop(new(0, 0, 100, 100)));

image.Save(ms, new JpegEncoder());
}

private static void VerifyEncodedStrings(ExifProfile exif)
{
Assert.NotNull(exif);
Expand Down
1 change: 1 addition & 0 deletions tests/ImageSharp.Tests/TestImages.cs
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ public static class Issues
public const string Issue2067_CommentMarker = "Jpg/issues/issue-2067-comment.jpg";
public const string Issue2638 = "Jpg/issues/Issue2638.jpg";
public const string Issue2758 = "Jpg/issues/issue-2758.jpg";
public const string Issue2857 = "Jpg/issues/issue-2857-subsub-ifds.jpg";

public static class Fuzz
{
Expand Down
3 changes: 3 additions & 0 deletions tests/Images/Input/Jpg/issues/issue-2857-subsub-ifds.jpg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading