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

String#unpack with Z and x results in corrupted data #2659

Closed
postmodern opened this issue May 15, 2022 · 5 comments
Closed

String#unpack with Z and x results in corrupted data #2659

postmodern opened this issue May 15, 2022 · 5 comments
Assignees
Labels
Milestone

Comments

@postmodern
Copy link

postmodern commented May 15, 2022

One of my specs which tests unpacking a repeating series of binary fields fails on truffleruby. I managed to narrow the bug down to using Z and x in a pack-string along with s and L after it. Changing the Z to an A or removing the x and the \x00 byte seems to work, implying there's some kind of interaction between the handling of Z and x.

Steps To Reproduce

"A\x00\xFF\xFF\xFF\xFF\xFF\xFF".unpack('Zxsl')

Expected Result

["A", -1, -1]

Actual Result

["A", -1, nil]

Versions

  • truffleruby 22.0.0.2, like ruby 3.0.2, GraalVM CE Native [x86_64-linux]
  • truffleruby 22.1.0, like ruby 3.0.2, GraalVM CE Native [x86_64-linux]

Additional Insights

If we add another x and another \x00 along with a L and \xFF\xFF\xFF\xFF we get this strange result:

"A\x00\xFF\xFF\xFF\xFF\xFF\xFF\x00\xFF\xFF\xFF\xFF".unpack('ZxslxL')
# => ["A", -1, 16777215, nil]

If we pack 16777215 as an unsigned 32bit integer (L) we get these bytes:

[16777215].pack('L')
# => "\xFF\xFF\xFF\x00"

This seems to imply there's an off-by-one error in the String#unpack logic caused by x.

@postmodern
Copy link
Author

Just tested on truffleruby 22.1.0 and seeing the same behavior.

@eregon
Copy link
Member

eregon commented May 16, 2022

Thank you for the report.

@aardvark179 Can you take a look?

@eregon eregon added the bug label May 16, 2022
@aardvark179
Copy link
Contributor

I'll take a look.

@eregon
Copy link
Member

eregon commented May 16, 2022

It's quite weird .unpack('Z') doesn't consume the \0 on CRuby, even though it obviously reads it. Sounds very error-prone.
It's easy to be compatible with CRuby for that, though.

@aardvark179
Copy link
Contributor

aardvark179 commented May 17, 2022

A fix for this has now been merged into master (b1dbba7).

@aardvark179 aardvark179 added this to the 22.2.0 milestone May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants