-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fix LZO decompression #28
Conversation
Codecov Report
@@ Coverage Diff @@
## main #28 +/- ##
==========================================
+ Coverage 84.97% 85.00% +0.03%
==========================================
Files 16 16
Lines 1118 1107 -11
==========================================
- Hits 950 941 -9
+ Misses 168 166 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -58,54 +46,49 @@ def decompress(src: Union[bytes, BinaryIO], header: bool = True, buflen: int = - | |||
val = src.read(1)[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't something like this add a bit more readability to the whole thing?
LZO_VERSION_1 = 0x10
LZO_VERSION_2 = 0x11
def _determine_lzo_version(src: io.BytesIO, dst: bytearray) -> int:
"""Determines the LZO version of the src.
Returns:
The next value in LZO Stream
"""
val = src.read(1)[0]
if val == LZO_VERSION_1:
raise ValueError("LZOv1")
elif val > LZO_VERSION_2:
# LZO is a stream
dst += src.read(val - LZO_VERSION_2)
val = src.read(1)[0]
if val < LZO_VERSION_1:
raise ValueError("Invalid LZO stream")
return val
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This turns out to be not quite correct. See also https://docs.kernel.org/staging/lzo.html and https://github.com/torvalds/linux/blob/master/lib/lzo/lzo1x_decompress_safe.c.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe good to add these references.
I looked a bit at making the code more readable, but that is far from trivial. Maybe comment the if statements a bit about which byte encoding is indicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added them as references
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be an idea to add some comments about which instruction encoding is used?
Including the first byte encodings
|
||
trailing = 0 | ||
if val > 17: | ||
dst += src.read(val - 17) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this need an additional state
variable?
As, according to the documentation you provided, state is already assigned at the first byte?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
state
looks unused there: https://github.com/FFmpeg/FFmpeg/blob/master/libavutil/lzo.c#L157
You seem to understand the algorithm better than me by now so feel free to make some suggestions 😄 |
Co-authored-by: Miauwkeru <Miauwkeru@users.noreply.github.com>
Fixes fox-it/dissect.squashfs#11