On Tue, Sep 13, 2011 at 07:14, Marc Lehmann <schm...@schmorp.de> wrote:
> On Mon, Sep 12, 2011 at 05:28:36PM +0200, Ben Noordhuis <i...@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
check.
>> 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.
_______________________________________________
libev mailing list
libev@lists.schmorp.de
http://lists.schmorp.de/cgi-bin/mailman/listinfo/libev
