[eio] Fix memory leak in eio__scandir()

[Marc Lehmann]

On Tue, Sep 13, 2011 at 07:46:45PM +0200, Ben Noordhuis <info at bnoordhuis.nl> wrote:
> 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.

I interpret it the same way, and meant it that way, and that is also how
the code behaves, no?

> There was an assertion in place[1] to that effect that was being
> assert((req->result == -1 && req->ptr2 == NULL) || (req->result >= 0 && req->ptr2 != NULL))

Where is this assertion? I don't think libeio ever has had anything like
that. It is also quite wrong, as it assumes that ptr2 is set on error,
which would be an undocumented extension that libeio doesn't provide.

Assuming this was somewhere in your code, let me say that making extra
assumptions is fine when debugging, but making extra assumptions is almost
always going to shoot you in the foot later - for example, there was a
similar problem when libeio set errno to 0 in most success cases and on
most platforms, to ease debugging, and people started to rely on that and
their code subtly failed.

> to the malloc'd memory, hence the confusion. Maybe you should take the
> patch anyway, as a principle of least surprise thing.

I am still confused on what it is supposed to achieve though - the only
effect I can see is that it confuses people because they might think ptr2
is valid in error cases as well, with disastrous results. This aspect
alone seems quite harmful to me.

Even if supported and documented, it would also be a disservice because it
would make libeio work differently than other similar C apis (especially
ISO C and posix, after which libeio are modelled directly). I see no value
in that, as right now, knowledge of posix/standard C behaviour is enough
to understand libeio (result is the function return value). The only
deviation from this is that some functions in POSIX that return != -1 on
success are wrapped so that they do return -1 on error.

Maybe I am missing the obvious advantage it might bring, but I think the
arguments against it are strong and the only argument that I can see that
would be in favour is to help debugging programs. As seen by the errno
example, I think history has shown this to be a bad idea.

> 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.

If true (i.e. errno isn't restored later), that's a quite obvious posix
violation, yes, and isn't traditional behaviour either. As such, using
such a debug build is probably a rather bad idea.

> > 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

I say it is enough, and at least one austin group member agrees with me :)
So whats your evidence to the contrary?

> there's nothing that forbids
> readdir() from having some kind of global state like a dirent cache.

There is nothing that forbids readdir to always return ENOSYS either or do
a lot of other broken things. Except that the function simply wouldn't be
useful, but instead rather needlessly harmful.

As long as the function behaves as specified by POSIX _or_ uses
traditional behaviour, it will be fine.

Note that readdir_r is considered to be quite useless and even harmful
- see for example http://udrepper.livejournal.com/18555.html and
http://readlist.com/lists/securityfocus.com/bugtraq/1/8312.html.

You should definitely avoid it in your own code.

-- 
                The choice of a       Deliantra, the free code+content MORPG
      -----==-     _GNU_              http://www.deliantra.net
      ----==-- _       generation
      ---==---(_)__  __ ____  __      Marc Lehmann
      --==---/ / _ \/ // /\ \/ /      schmorp at schmorp.de
      -=====/_/_//_/\_,_/ /_/\_\