search

binref / refinery

High Octane Triage Analysis
Other
635 stars 63 forks source link

fix off-by-one error in hexdump #8

Closed baderj closed 2 years ago

baderj commented 2 years ago

The hexdump preview of peek displays one more line than specified.

So peek without -l / --lines shows 11 lines instead of the default 10. You can see the bug in https://github.com/binref/refinery/blob/master/tutorials/tbr-files.v0x01.netwalker.dropper.ipynb which always shows 11 lines for peek when called without -l / --lines.

For example before:

$ emit CHANGELOG.md  | peek -l 1
----------------------------------------------------------------------------------------------------------------------------
00000: 23 20 42 69 6E 61 72 79 20 52 65 66 69 6E 65 72 79 20 43 68 61 6E 67 65 6C 6F 67 0D 0A  #.Binary.Refinery.Changelog..
0001D: 0D 0A 23 23 20 56 65 72 73 69 6F 6E 20 30 2E 34 2E 36 0D 0A 2D 20 54 68 65 20 60 6F 66  ..##.Version.0.4.6..-.The.`of
----------------------------------------------------------------------------------------------------------------------------

This pull request fixes the bug which was caused by a simple > instead of >= check.

huettenhain commented 2 years ago

Hey! The one pathetic test I wrote for peek fails, but I would suggest we simply change the test: I understand now what happened there: In the test, I specify 14 lines of output, and then require that peek produces 14 lines of output. Past me did not realize that the hexdump function will not count this type of line:

..: === repeats 2 times ===  ========

towards the maximum. Hence, to get all the lines of output we want from the test, we should require 15 lines.

TL/DR: Would you do me a favour and change the 14 to a 15 over here and include that in the pull request?

https://github.com/binref/refinery/blob/1cc714184d98ac4ad4ca95e366f27b03ec6b64de/test/units/sinks/test_peek.py#L21

That should greenlight the test. Kindof a cheeky solution, but I am ok with it ;-).

huettenhain commented 2 years ago

PS: I could also just merge this PR and then fix the tests later, but this is my first experience merging something while using the GitHub actions CI pipeline. In theory, we should be able to get the tests all green, and I am super curious to see that work =). I figured this PR is straightforward enough to give me that small pleasure.

huettenhain commented 2 years ago

Awesome, thank you very much @baderj!