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

Resolve feature regression in hexdump(..., cyclic=True) #837

Merged
merged 1 commit into from
Jan 10, 2017

Conversation

zachriggle
Copy link
Member

@zachriggle zachriggle commented Jan 9, 2017

This code wasnt updated when the code was changed to operate on file descriptors.

Additionally, there was a functional regression, that the first line would be skipped.

Fixes: #836
Caused by: #695 (d65991d)

@zachriggle zachriggle added the bug label Jan 9, 2017
@zachriggle zachriggle added this to the 3.3.0 milestone Jan 9, 2017
@zachriggle zachriggle self-assigned this Jan 9, 2017
@zachriggle
Copy link
Member Author

There are no tests for any of the hexdump functionality, because it currently does not have any mechanism to NOT emit ANSI color codes.

@zachriggle
Copy link
Member Author

Before

>>> print hexdump(fit({64: 'X'*20}, length=0xc0), cyclic=1)
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
<ipython-input-1-d2d4799be547> in <module>()
----> 1 print hexdump(fit({64: 'X'*20}, length=0xc0), cyclic=1)

/Users/riggle/pwntools/pwnlib/util/fiddling.py in hexdump(s, width, skip, hexii, begin, style, highlight, cyclic)
    707                                   style,
    708                                   highlight,
--> 709                                   cyclic))
    710
    711 def negate(value, width = None):

/Users/riggle/pwntools/pwnlib/util/fiddling.py in hexdump_iter(fd, width, skip, hexii, begin, style, highlight, cyclic)
    629
    630     if cyclic:
--> 631         update_cyclic_pregenerated(len(s))
    632
    633     numb = 0

NameError: global name 's' is not defined

After

>>> print hexdump(fit({64: 'X'*20}, length=0xc0), cyclic=1)
00000000  61 61 61 61  62 61 61 61  63 61 61 61  64 61 61 61  │aaaa│baaa│caaa│daaa│
*
00000040  58 58 58 58  58 58 58 58  58 58 58 58  58 58 58 58  │XXXX│XXXX│XXXX│XXXX│
00000050  58 58 58 58  76 61 61 61  77 61 61 61  78 61 61 61  │XXXX│vaaa│waaa│xaaa│
00000060  79 61 61 61  7a 61 61 62  62 61 61 62  63 61 61 62  │yaaa│zaab│baab│caab│
*
000000c0

@zachriggle
Copy link
Member Author

Added @br0ns as a reviewer since his commit d65991d caused the regression

Copy link
Contributor

@TethysSvensson TethysSvensson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TethysSvensson
Copy link
Contributor

Ping me once this lands, and I will forwards-merge + do a new release.

@zachriggle zachriggle force-pushed the issue_836 branch 2 times, most recently from 0c5e243 to d8cdc46 Compare January 9, 2017 18:51
@zachriggle
Copy link
Member Author

Don't merge this yet, I broke something in attempting to make the code less unreadable.

@TethysSvensson
Copy link
Contributor

Sure, I will leave merging to you. Just ping me once you want the forward merge + release. :)

@zachriggle
Copy link
Member Author

What the fuck did I even do to this. Wat.

@zachriggle zachriggle force-pushed the issue_836 branch 2 times, most recently from f3d00dc to 7e72312 Compare January 10, 2017 02:53
This code wasnt updated when the code was changed to operate on file descriptors.

Additionally, there was a functional regression, that the first line would be skipped.

Fixes: Gallopsled#836
Caused by: Gallopsled#695 (d65991d)
@zachriggle zachriggle merged commit 0426d49 into Gallopsled:stable Jan 10, 2017
@zachriggle zachriggle deleted the issue_836 branch January 10, 2017 02:59
@zachriggle
Copy link
Member Author

@idolf ping

@TethysSvensson
Copy link
Contributor

Forward-merge + release done.

zachriggle added a commit to zachriggle/pwntools that referenced this pull request Jan 10, 2017
zachriggle added a commit that referenced this pull request Jan 10, 2017
Kyle-Kyle pushed a commit to Kyle-Kyle/pwntools that referenced this pull request Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants