This is the mail archive of the
cygwin-patches
mailing list for the Cygwin project.
Re: fix off-by-one in dup2
- From: Christopher Faylor <cgf-use-the-mailinglist-please at cygwin dot com>
- To: cygwin-patches at cygwin dot com
- Date: Thu, 5 Dec 2013 14:55:55 -0500
- Subject: Re: fix off-by-one in dup2
- Authentication-results: sourceware.org; auth=none
- References: <52437121 dot 1070507 at redhat dot com> <20131204093238 dot GA28314 at calimero dot vinschen dot de> <20131204113626 dot GB29444 at calimero dot vinschen dot de> <20131204120408 dot GC29444 at calimero dot vinschen dot de> <20131204170028 dot GA2590 at ednor dot casa dot cgf dot cx> <20131204172324 dot GA13448 at calimero dot vinschen dot de> <20131204175108 dot GB2590 at ednor dot casa dot cgf dot cx> <52A08372 dot 7080402 at redhat dot com>
- Reply-to: cygwin-patches at cygwin dot com
On Thu, Dec 05, 2013 at 06:45:22AM -0700, Eric Blake wrote:
>On 12/04/2013 10:51 AM, Christopher Faylor wrote:
>
>>>>> One question, though. Assuming start is == size, then the current code
>>>>> in CVS extends the fd table by only 1. If that happens often, the
>>>>> current code would have to call ccalloc/memcpy/cfree a lot. Wouldn't
>>>>> it in fact be better to extend always by at least NOFILE_INCR, and to
>>>>> extend by (1 + start - size) only if start is > size + NOFILE_INCR?
>>>>> Something like
>>>>>
>>>>> size_t extendby = (start >= size + NOFILE_INCR) ? 1 + start - size : NOFILE_INCR;
>>>>>
>
>Always increasing by a minimum of NOFILE_INCR is wrong in one case - we
>should never increase beyond OPEN_MAX_MAX (currently 3200). dup2(0,
>3199) should succeed (unless it fails with EMFILE due to rlimit, but we
>already know that our handling of setrlimit(RLIMIT_NOFILE) is still a
>bit awkward); but dup2(0, 3200) must always fail with EBADF. I think
>the code in CVS is still wrong: we want to increase to the larger of the
>value specified by the user or NOFILE_INCR to minimize repeated calloc,
>but we also need to cap the increase to be at most OPEN_MAX_MAX
>descriptors, to avoid having a table larger than what the rest of our
>code base will support.
I made some more changes to CVS. Incidentally did you catch the fact
that you broke how this worked in 1.7.26? You were taking a MAX of a
signed and unsigned quantity so the signed quantity was promoted to a
huge positive number.
>Not having NOFILE_INCR free slots after a user allocation is not fatal;
No one implied it was.
>it means that the first allocation to a large number will not have tail
>padding, but the next allocation to fd+1 will allocate NOFILE_INCR slots
>rather than just one. My original idea of MAX(NOFILE_INCR, start -
>size) expresses that.
That wasn't Corinna's concern. My replacement code would have called
calloc for every one of:
dup2(0, 32);
dup2(1, 33);
dup2(2, 34);
Obviously there are different ways to avoid this and I chose to extend
the table after the "start" location.
>>> That might be helpful. Tcsh, for instance, always dup's it's std
>>> descriptors to the new fds 15-19. If it does so in this order, it would
>>> have to call extend 5 times.
>>
>> dtable.h:#define NOFILE_INCR 32
>>
>> It shouldn't extend in that scenario. The table starts with 32
>> elements.
>
>Rather, the table starts with 256 elements; which is why dup2 wouldn't
>crash until dup'ing to 256 or greater before I started touching this.
The table is initialized in dtable_init() with 32 elements. When it
enters main, it is still 32 elements, at least according to
cygheap->fdtab.size. I just checked this with gdb.
cgf