miurahr / py7zr

7zip in python3 with ZStandard, PPMd, LZMA2, LZMA1, Delta, BCJ, BZip2, and Deflate compressions, and AES encryption.
https://pypi.org/project/py7zr/
GNU Lesser General Public License v2.1
463 stars 74 forks source link

Prevent crash - Change the threshold for a full 9-byte number #591

Closed XanC closed 3 months ago

XanC commented 5 months ago

Attempting to encode numbers between 0xFFFFFFFFFFFFFF and 0x01FFFFFFFFFFFFFF would result in an error. Because the byte length is 8, the value of (8 - byte_length - 1) was negative, resulting in an attempt to shift by a negative number of bits which caused an error.

Pull request type

Which ticket is resolved?

None that I know of

What does this PR change?

Changes the threshold for when to use a nine-byte representation of a number.

Other information

miurahr commented 5 months ago

Thank you for feedback from findings! Could you add a test case that reproduce the case which should be failed without the patch? And then, you can show the fix solve the sisue.

XanC commented 5 months ago

I'll see if I have a chance to do that in the next few days. Clearly such large numbers are rarely needed in the real world!

miurahr commented 5 months ago

It is ok it is not real. I like to see a simple unit test.

miurahr commented 3 months ago

Here is a format documentation about integer representation

https://py7zr.readthedocs.io/en/latest/archive_format.html#integers

Original document is here https://github.com/p7zip-project/p7zip/blob/master/DOC/7zFormat.txt#L111

Size of encoding sequence depends from first byte: First_Byte Extra_Bytes Value (binary)
0xxxxxxx : ( xxxxxxx ) 10xxxxxx BYTE y[1] : ( xxxxxx << (8 1)) + y 110xxxxx BYTE y[2] : ( xxxxx << (8 2)) + y ... 1111110x BYTE y[6] : ( x << (8 * 6)) + y 11111110 BYTE y[7] : y 11111111 BYTE y[8] : y

Is the change consistent with the spec?

miurahr commented 3 months ago

It looks good.

XanC commented 3 months ago

Sorry I wasn't able to follow up on this better. Thanks for merging it!