[eio] Fix memory leak in eio__scandir()

Ben Noordhuis info at bnoordhuis.nl
Tue Sep 13 19:46:45 CEST 2011

On Tue, Sep 13, 2011 at 07:14, Marc Lehmann <schmorp at schmorp.de> wrote:
> On Mon, Sep 12, 2011 at 05:28:36PM +0200, Ben Noordhuis <info at bnoordhuis.nl> wrote:
>> eio__scandir() leaks memory when opendir() returns NULL. It sets
>> req->result == -1 but doesn't free the memory it allocates.
> Hmm, cannot reproduce this here, do you have a testcase that shows this
> behaviour?
>> Attached are two patches (different takes, same solution) that fix
> Neither of these patches seem to actually do anything (they just free the
> data earlier) - both ptr1 and ptr2 get freed in eio_destroy, which is
> called after the user callback returns. If your patches fix a memleak,
> then there must be a bug somewhere else.

That's a misunderstanding on my part then. Quoting the docs:

        The "req->result" indicates either the number of files found, or -1
        on error. On success, null-terminated names can be found as
        "req->ptr2", and "struct eio_dirents", if requested by "flags", can
        be found via "req->ptr1".

I interpret this as either req->result == -1 or req->ptr2 points to
the requested data.

There was an assertion in place[1] to that effect that was being
triggered. Back-tracing in gdb showed that libeio was still hanging on
to the malloc'd memory, hence the confusion. Maybe you should take the
patch anyway, as a principle of least surprise thing.

[1] assert((req->result == -1 && req->ptr2 == NULL) || (req->result >=
0 && req->ptr2 != NULL))

>> 1. doesn't set req->errorno when opendir() fails (both patches fix that), and
> There is actually no possible codepath in libeio that doesn't set errorno
> (it's set at the end of eio_execute), and it is certainly set when I just
> tried to open a nonexistand path. Do you have a testcase that shows this?

Tentatively speaking, this may be a glibc bug. I tested against a
debug build of glibc and it seems to zero errno inside malloc's arena

>> 2. uses readdir(), which is not guaranteed to be thread-safe. glibc
> libeio doesn't rely on it being threadsafe - it never calls readdir from
> multiple threads on the same dirstream.
> a lock inside readdir doesn't make it threadsafe, btw., as readdir can
> return a pointer to a per-dirstream structure that changes between calls.

I suspect you mean that there's only one thread at a time operating on
the DIR pointer. That's not enough: there's nothing that forbids
readdir() from having some kind of global state like a dirent cache.
It should (hopefully) be an academic discussion by now, all the Unices
that matter do the right thing.

More information about the libev mailing list