If I run following code (on Python 3.5.3, Linux) the interpreter crashes with a segfault:


def pbkdf2_bin(data, salt, iterations=1000, keylen=24, hashfunc=None):
    hashfunc = hashfunc or hashlib.sha1
    mac = hmac.new(data, None, hashfunc)
    def _pseudorandom(x, mac=mac):
        h = mac.copy()
        h.update(x)
        return h.digest()
    buf = []
    for block in range(1, -(-keylen // mac.digest_size) + 1):
        rv = u = _pseudorandom(salt + _pack_int(block))
        for i in range(iterations - 1):
            u = _pseudorandom(u)
            rv = starmap(xor, zip(rv, u))
        buf.extend(rv)
    return bytes(buf[:keylen])

pbkdf2_bin(b'1234567890', b'1234567890', 200000, 32)


I was able to track it down to the line of buf.extend(rv) which apparently is causing the segfault. Note that rv is a lazy-evaluated starmap. I also get a segfault if I evaluate it by other means (e.g. by passing it to the list constructor). However, if I evaluate it immediately by wrapping the starmap constructor with the list constructor, the code works as expected. But I wasn't able yet, to further isolate the issue. FWIW, the Python 2 version [1] of this code works just fine without forcing immediate evaluation of the starmap.

Note that the code posted, except for the bits I changed in order to make it compatible with Python 3, is under the copyright of Armin Ronacher, who published it under the BSD license.

[1]: https://github.com/mitsuhiko/python-pbkdf2

I just noticed that the segfault can also be reproduced with Python 2 [1]. So please ignore what I said before that this wouldn't be the case.

While it is debatable whether using a lazy evaluated object with so many recursions is a good idea in the first place, causing it the interpreter to crash with a segfault still seems concerning to me.

[1]: https://github.com/mitsuhiko/python-pbkdf2/issues/2

I'm not sure this can be fixed reasonably.  It would entail taking every possible iterator in all of Python and adding some sort of recursion depth check.  That approach risks introducing bugs, risks breaking existing code that relies on heavy iteration depth (we know YouTube uses such code), and it would slow-down iteration everywhere (possibly by quite a bit since iteration is a thin and fast protocol).

Also, this is a general case of recursive crashers (there are a number of open bugs on this) that are hard to detect and prevent.

IMO, what is really needed is something that monitors the C stack and gracefully detects the situation just before a crash and gracefully unwinds the calls.  This isn't easy to do but it would be a broad, complete, performant, and minimally invasive solution to a problem that is irritating but only arises very rarely (and typically only in code that isn't really sane to begin with). 

The usual recursion depth checks on function calls are reasonable because 1) their overhead is minimal compared to overall function call overhead and 2) they detect common programming errors.   

To me, the starmap() case is different because the problem is rare and because iteration is a tight fast protocol.  

One other thought:  While a RuntimeError would be much preferred to a crash, the programmer has to respond in the same way, by changing their code.  In the pbkdf2_bin() example, the code can't be made to run as-is.  The approach has to be changed to a list based approach (even using hand-rolled iterators instead of starmap() won't help).

A last thought:  I presume that the whole reason for using starmap() instead of "yield from" or somesuch is the person writing code needed good performance, so taking away performance defeats the original intent.

This looks as a duplicate of issue14010 (and many other issues).

What makes this report unique is that it is the first one occurring in the wild (i.e. wasn't an example contrived solely for the purpose of producing a crash).  Otherwise, we're not ever seeing this happen in real code.

Serhiy, do you have any ideas for a non-invasive global solution to running out of C-stack?

The GCC options for -fno-stack-limit and -fsplit-stack are one avenue.  Another is to put in a periodic check in the c-eval loop to test whether available stack is getting too small and to find a way to exit gracefully.

Options like -fno-stack-limit and -fsplit-stack can help to avoid a crash, but they are compiler-specific and we should test how they affect the performance and memory consumption.

How can we test that the available stack is too small? This is not a part of the language, we need to use some compiler-specific tricks.

If add such check, it should be added not in the c-eval loop (or not only in the c-eval loop), but for every call of tp_iternext. Recursive call can avoid the c-eval loop. Maybe add the check in PyIter_Next() and always use PyIter_Next() instead of direct calls of tp_iternext. If the check for the stack size is not cheap we can just count the deep of the recursion and check the exact stack size every say 50 levels. And this is a good opportunity to check for KeyboardInterrupt .

In any case it would be useful to add a user specified limit on the deep of the recursion similar to sys.getrecursionlimit(). Even if the stack grows unlimitedly we don't want to fall in the swapping on unexpected deep recursion.

Thanks for your response, both of you. All you said, make sense.

Just for the record, I wouldn't necessarily expect 200k nested iterators to work. Even if it could be made work, I guess it would use way too much memory. But a RuntimeError would be much preferable over a crash.

For the code above, the fix would be to just immediately convert the iterator returned by starmap() to a list. But in the end, regardless of this additional operation, it didn't perform well, so that I tossed that code, and used openssl's PBKDF2 implementation through the ctypes module.

Still, I'm somewhat concerned that code like this, will cause an unexpected crash that cannot be handled, dependent on run time variables. Could this perhaps even provide a security vulnerability? It seems to be a buffer overflow, after all.

Since this is duplicate, marking as closed.