[prev in list] [next in list] [prev in thread] [next in thread]
List: gnash-commit
Subject: Re: [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_final-1963-g63de409
From: Bastiaan Jacques <bastiaan () bjacques ! org>
Date: 2014-04-30 20:47:39
Message-ID: alpine.DEB.2.00.1404302228400.27680 () krusty ! bjacques ! org
[Download RAW message or body]
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
_______________________________________________
Gnash-commit mailing list
Gnash-commit@gnu.org
https://lists.gnu.org/mailman/listinfo/gnash-commit
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic