odd paddings are inserted when rop.raw() is called with a string

[IngyuTae]

pwntools 3.4.0 / python 2.7.6 / ubuntu 14.04 x64

I wanted to put some (str) paddings into the rop chain but odd paddings are also inserted.

>>> from pwn import *
>>> r = ROP('/bin/sh')
[*] '/bin/sh'
    Arch:     amd64-64-little
    RELRO:    Full RELRO
    Stack:    Canary found
    NX:       NX enabled
    PIE:      PIE enabled
    FORTIFY:  Enabled
[*] Loading gadgets for '/bin/sh'
>>> r.raw('AAAAAAAA')
>>> r.raw('BBBBBBBB')
>>> r.raw('CCCCCCCC')
>>> print r.dump()
0x0000:           'AAAA' 'AAAAAAAA'
0x0004:           'AAAA'
0x0008:           'BBBB' 'BBBBBBBB'
0x000c:           'BBBB'
0x0010:           'aaaa'
0x0014:           'baaa'
0x0018:           'CCCC' 'CCCCCCCC'
0x001c:           'CCCC'
0x0020:           'aaaa'
0x0024:           'baaa'
0x0028:           'caaa'
0x002c:           'daaa'
0x0030:           'eaaa'
0x0034:           'faaa'

What I expected was:

>>> print r.dump()
0x0000:           'AAAA' 'AAAAAAAA'
0x0004:           'AAAA'
0x0008:           'BBBB' 'BBBBBBBB'
0x000c:           'BBBB'
0x0010:           'CCCC' 'CCCCCCCC'
0x0014:           'CCCC'

Did I miss something?

[IngyuTae]

I found that generatePadding returns whole the padding generated even when count is zero. Below code will fix this issue:

    def generatePadding(self, offset, count):
        """
        Generates padding to be inserted into the ROP stack.
        """
        if count:
            return cyclic.cyclic(offset + count)[-count:]
        return ''

[kristoff3r]

Thanks, that's definitely a bug, it's fixed on the dev branch now.

@idolf: since it is so simple, we might want to add it to the beta branch as well?

[TethysSvensson]

Yep, I will do a backport and a new beta release.

[TethysSvensson]

Actually, could you add a test for this before I backport it?

[zachriggle]

@kristoff3r Can you use Pull Requests for bug fixes like this? Thanks :)

[kristoff3r]

@kristoff3r Can you use Pull Requests for bug fixes like this? Thanks :)

Yeah my bad, I just thought "well, an obvious 2 line fix that doesn't touch anything else, easy", without thinking about changelogs, tests or backports. I'll just stick to PR's.

[zachriggle]

@kristoff3r No problem ^_^. Feel free to push to dev with impunity with non-backported-bugfixes.

[kristoff3r]

@idolf: backport 52aadfa and 8b8a50b to stable and beta.

[zachriggle]

@kristoff3r Please create a new pull request against stable, with a branch based on stable, with those commits.

[TethysSvensson]

@zachriggle I promised to backport them for him this time, since they both merge cleanly, but he said he would do it the right way next time.

[TethysSvensson]

I ended up backporting only the fix and not the test, since the test used features not yet present on stable.