Re: [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_final-1963-g63de409

[Bastiaan Jacques]

Hi John, thank you for reviewing and writing in.

On Sun, 27 Apr 2014, John Gilmore wrote:

> >     Savannah #42199: avoid thrashing the stack in case of a high fd by
> >     using poll() instead of select().
>
> It looks like the variable <fd> has been trashed, since the second
> backtrace shows a nine-digit file descriptor that the kernel clearly
> would never implement.

Possibly, but the first backtrace has an fd that's exactly FD_SETSIZE,
which seems like too much of a coincidence to me. Another possibility is
that the second stacktrace is obfuscated by either fd_set stack
thrashing or by jemalloc, which is used in the Firefox process. Both are
known to make debugging memory issues difficult. So perhaps this is
nothing more than a file descriptor leak.

>  It would be simpler to add a check for "if (fd
> > FD_SETSIZE) abort();". -- though it looks like we crash in this case
> anyway, so we don't really need the check.

Yes, although the current abort is being generated by _FORTIFY_SOURCE,
which is not enabled by all distros. It would probably be better to
abort in our own code, if we are going to abort at all.

> What we need is to figure out why a bogus <fd> was passed in to this
> routine.

Unfortunately I see nothing out of the ordinary in the backtrace, and I
haven't been able to reproduce the issue.

> I recommend reverting the change, since it didn't actually fix a bug nor
> contribute to fixing it.

That's assuming the abort really is caused by some stack mangling prior
to FD_SET().

With regards to reverting: from what I understand that is no reason to
prefer select() over poll(), but there is reason for the reverse: not
doing horrible things to the stack. :)

Still, I don't want to wallpaper over a bug by doing that, so I'm
thinking of adding an assert(fd < FD_SETSIZE), or something along those
lines, to aid in debugging the issue.

> Also, the change introduced an extraneous change:
> the original timeout was 2 seconds and the poll changes it to 1.

Good catch, will fix.

Bastiaan