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

Jpeg HuffmanTable improvements #1926

Merged

Conversation

br3aker
Copy link
Contributor

@br3aker br3aker commented Jan 6, 2022

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

This PR removes redundant Configure() calls for HuffmanTable and removes stack pressure from stackalloc calls.

TODO

  • Remove Configure() method
  • Remove stackalloc calls
  • Create out-of-bounds huffman table test image

@br3aker br3aker changed the title Removed Configure() method Jpeg HuffmanTable improvements Jan 6, 2022
@br3aker
Copy link
Contributor Author

br3aker commented Jan 6, 2022

Gotcha

Message: 
    Assert.Throws() Failure
    Expected: typeof(SixLabors.ImageSharp.InvalidImageContentException)
    Actual:   typeof(System.IndexOutOfRangeException): Index was outside the bounds of the array.
    ---- System.IndexOutOfRangeException : Index was outside the bounds of the array.

  Stack Trace: 
    HuffmanTable.ctor(ReadOnlySpan`1 codeLengths, ReadOnlySpan`1 values) line 58
    JpegDecoderCore.ProcessDefineHuffmanTablesMarker(BufferedReadStream stream, Int32 remaining) line 1098
    JpegDecoderCore.ParseStream(BufferedReadStream stream, HuffmanScanDecoder scanDecoder, CancellationToken cancellationToken) line 358
    ...

One of the invalid test images was going out of bounds during new HuffmanTable ctor. The only reason it wasn't spotted before is that exposed code in Configure() was called after checks for invalid image which led to these crashes.

@JimBobSquarePants
Copy link
Member

Ah nice, well done finding that. So now I guess we have to find a way to sanitise cheaply and then try to move the table building to the constructor.

@br3aker
Copy link
Contributor Author

br3aker commented Jan 7, 2022

It's actually a bug, there's a possibility for malformed image go out of bounds without any exceptions in current master. I found a way to check that without a noticable slowdown. I'd also create an example image for a test suite.

@codecov
Copy link

codecov bot commented Jan 7, 2022

Codecov Report

Merging #1926 (544c557) into master (ce34e84) will decrease coverage by 0%.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1926   +/-   ##
======================================
- Coverage      88%     88%   -1%     
======================================
  Files         966     966           
  Lines       51350   51320   -30     
  Branches     6401    6397    -4     
======================================
- Hits        45218   45188   -30     
  Misses       5074    5074           
  Partials     1058    1058           
Flag Coverage Δ
unittests 88% <100%> (-1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...mats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs 93% <100%> (-1%) ⬇️
...rp/Formats/Jpeg/Components/Decoder/HuffmanTable.cs 100% <100%> (ø)
src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs 89% <100%> (-1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce34e84...544c557. Read the comment docs.

@br3aker br3aker marked this pull request as ready for review January 9, 2022 01:42
@br3aker
Copy link
Contributor Author

br3aker commented Jan 9, 2022

High hopes it would pass all the tests, it is ready to be reviewed:

  • Rewritten part of the huffman table building routine, it now requires 256 bytes less memory workspace
  • No stackalloc calls
  • Rewritten DHT marker parsing code, now it fetches a big single buffer for binary parsing and table building routine
  • Added sanity checks for huffman table input, now it should be impossible to break decoder with malformed DHT marker

There's no need for a new test image, image from #839 has malformed DHT marker which goes out-of-bounds during huffman table for unsafe pointers (which is unspottable without wrapping them with Span). I've left a note just in case but this PR should nullify this error.

this is my favourite
On top of a critical security breach fix and extra checks we are not actually slower:

MASTER
// Decoding
// Elapsed: 4643ms across 400 iterations
// Average: 11,6075ms

PR
// Decoding
// Elapsed: 4634ms across 400 iterations
// Average: 11,585ms

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

Faster... Fantastic 🤣

This is very, very good as always. Thanks! 👍

@JimBobSquarePants JimBobSquarePants merged commit 576a69f into SixLabors:master Jan 9, 2022
@br3aker br3aker deleted the dp/huff-table-improvements branch January 9, 2022 15:09
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.

None yet

2 participants