Rucksack not found error when binary contains zero bytes

[jgillich]

Hey, first let me say thanks for this project, this is exactly what I needed!

I was getting the "Rucksack not found" error. I've tracked it down to this line: https://github.com/busyloop/rucksack/blob/d55cb271aa19d0d435b58978ed5dce8fc1ba2b75/src/rucksack.cr#L39

buf contained a Bytes[48], so like a literal 0 character? Very weird.

Then I checked offset. 6045696 so around 6MB. My base binary is twice that. So it seems like I have a bunch of null bytes somewhere that Rucksack thinks is the knautschzone.

Then I went and doubled the size of the padding and knautschzone. That makes it work. Still, this feels fragile, you really can't make any assumptions about the content of the binary.

It's probably better to append the files first and the metadata last, so you can just read from the end of the file. What do you think?

[m-o-e]

@jgillich Ouch, interesting!

Hmm. Yes, Rucksack prepends 16kb of null bytes (Knautschzone) under the assumption that half of that (8kb of nulls) would not appear naturally in a binary.

It does that in order to be able to read the file faster (in 8kb chunks) to find the start of the appended data.

I'm surprised you have a real case where that breaks, and so soon 🙈

I believe the solution here is to resume the search instead of aborting it when a "false hit" happens. Will push a fix for that today, would be great if you could keep the source that produces a failing binary around and re-test then.

[m-o-e]

For reference: It expects to find 8kb of nullbytes, followed by the string ==RUCKSACK==\n and currently aborts when it finds 8kb of nulls that are not followed by that.

I'll patch it to not abort but instead restart the search. I believe that should be safe for all cases then (unless you explicitly embed a String of that format in your code).

[m-o-e]

It's probably better to append the files first and the metadata last, so you can just read from the end of the file. What do you think?

That is another interesting idea indeed! Won't do it in this patch as it will need more fundamental changes, but I think it may be a good improvement for version 2.

[m-o-e]

@jgillich Ok, v1.0.1 is up. ~Can you still reproduce the problem with this new version?~

(I'm also building some test-cases to verify right now)

[m-o-e]

@jgillich Ah sorry, turned out v1.0.1 wasn't fully fixed.

Please re-test with v1.0.2. It now also has test-coverage for this case; https://github.com/busyloop/rucksack/commit/b17f1d3d70801ad3ed2c91b357a8394b35225c6b#diff-b8765e8e12fe5b90ee3f4f3a864b5d96

[jgillich]

I was about to comment that haha. I'm waiting for the compiler to finish but I expect this to work.

[jgillich]

Works great. Thank you! :+1:

[m-o-e]

Awesome, thanks a lot for reporting and for your help with testing! 🙇