testdup test fails when compiled in debug mode on Windows

[Mike Rumph <mi...@oracle.com>]

The apr_file_dup2() function in apr/file_io/win32/filedup.c calls 
_commit() for standard file handles 0, 1 and 2.
The _commit() function will assert with the message "Invalid file 
descriptor. File possibly closed by a different thread" or return a 
value of -1 if the file handle refers to a device.
The assert will appear if APR is compiled in debug mode.

This can be seen by running "testall.exe testdup".
But if file redirection is used (such as "testall.exe testdup > outfile 
2>&1"), then the test completes successfully.

I have attached a patch that corrects this problem by checking _isatty().

Credit goes to Jeff Trawick for catching this problem in the first place.

Thanks,

Mike Rumph

Re: testdup test fails when compiled in debug mode on Windows

[Jeff Trawick <tr...@gmail.com>]

On Thu, Nov 7, 2013 at 11:20 AM, William A. Rowe Jr. <wr...@rowe-clan.net>wrote:

> On Thu, 7 Nov 2013 09:43:07 -0500
> Jeff Trawick <tr...@gmail.com> wrote:
>
> > On Thu, Nov 7, 2013 at 12:33 AM, William A. Rowe Jr.
> > <wr...@rowe-clan.net>wrote:
> >
> > > On Wed, 06 Nov 2013 16:07:37 -0800
> > > Mike Rumph <mi...@oracle.com> wrote:
> > >
> > > >
> > > > On 11/6/2013 1:06 PM, Jeff Trawick wrote:
> > > > >
> > > > > I just played with _commit() on stdin a bit.  It turns out that
> > > > > _commit(0) fails if stdin is redirected (main.exe < somefile)
> > > > > but works if stdin is a tty.  That's the opposite of _commit(1
> > > > > or 2). But I don't see how _commit(0) makes sense anyway, so I
> > > > > simply removed the call instead of reversing the corresponding
> > > > > _isatty() check in your patch.
> > > > >
> > > > > trunk: r1539455
> > > > > 1.5.x branch: r1539461
> > > > Okay Jeff,
> > > >
> > > > I just tried both stdout and stdin, and got the same results that
> > > > you did. Strange but true.
> > >
> > > IIRC the choice to _commit ahead of any handling of stdin/out/err
> > > reflected the possibility that bytes were queued/stuck in the FILE
> > > or the msvcrt 'fd' (not really an fd at all) before assuming
> > > ownership of the file handle.  It might have been an overreaction
> > > to a problem that wouldn't exist in practice.  But I'd prefer if
> > > this were left context sensitive to _DEBUG mode builds.
> > >
> >
> > "The *_commit* function forces the operating system to write the file
> > associated with *handle* to disk"
> >
> > _commit(fileno_stdout or fileno_stderr) fails if it refers to the
> > terminal whether or not it is a debug build.  It simply isn't a valid
> > call.  I called out the debug build issue in CHANGES because that is
> > likely the only case where anyone would encounter a problem symptom.
> > (_commit() otherwise continues to return -1/EBADF and nobody notices.)
> >
> > The fileno_stdin issue is even more odd as it took an opposite sense
> > of _isatty(fileno_stdin) to keep it from reporting an error, but I
> > don't see any connection between _commit() and stdin so it seemed
> > most appropriate to simply remove the call for stdin.
> >
> > IOW, I don't see the need to tie this to debug builds because the
> > calls are invalid whether or not the RTL pops up the dialog.
>
> In that case I can see the benefit to dropping the call entirely.
>

Maybe we've lost communication here...  The calls that are invalid are,
essentially, _commit(console), which we've filtered out via a check to
_isatty().  _commit(file) still makes sense AFAIK and still is used.


>
> On Thu, 07 Nov 2013 07:09:18 -0800
> Mike Rumph <mi...@oracle.com> wrote:
>
> > Do you remember if the reasons for using _commit() would distinguish
> > between input and output?
>
> No, not offhand.  In any case, they were in inverted order to clean up
> any lingering input ahead of dup'ing or creating an apr file_t from an
> apr_os_file_t.
>
> Let's axe it.
>
>


-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Re: testdup test fails when compiled in debug mode on Windows

["William A. Rowe Jr." <wr...@rowe-clan.net>]

On Thu, 7 Nov 2013 09:43:07 -0500
Jeff Trawick <tr...@gmail.com> wrote:

> On Thu, Nov 7, 2013 at 12:33 AM, William A. Rowe Jr.
> <wr...@rowe-clan.net>wrote:
> 
> > On Wed, 06 Nov 2013 16:07:37 -0800
> > Mike Rumph <mi...@oracle.com> wrote:
> >
> > >
> > > On 11/6/2013 1:06 PM, Jeff Trawick wrote:
> > > >
> > > > I just played with _commit() on stdin a bit.  It turns out that
> > > > _commit(0) fails if stdin is redirected (main.exe < somefile)
> > > > but works if stdin is a tty.  That's the opposite of _commit(1
> > > > or 2). But I don't see how _commit(0) makes sense anyway, so I
> > > > simply removed the call instead of reversing the corresponding
> > > > _isatty() check in your patch.
> > > >
> > > > trunk: r1539455
> > > > 1.5.x branch: r1539461
> > > Okay Jeff,
> > >
> > > I just tried both stdout and stdin, and got the same results that
> > > you did. Strange but true.
> >
> > IIRC the choice to _commit ahead of any handling of stdin/out/err
> > reflected the possibility that bytes were queued/stuck in the FILE
> > or the msvcrt 'fd' (not really an fd at all) before assuming
> > ownership of the file handle.  It might have been an overreaction
> > to a problem that wouldn't exist in practice.  But I'd prefer if
> > this were left context sensitive to _DEBUG mode builds.
> >
> 
> "The *_commit* function forces the operating system to write the file
> associated with *handle* to disk"
> 
> _commit(fileno_stdout or fileno_stderr) fails if it refers to the
> terminal whether or not it is a debug build.  It simply isn't a valid
> call.  I called out the debug build issue in CHANGES because that is
> likely the only case where anyone would encounter a problem symptom.
> (_commit() otherwise continues to return -1/EBADF and nobody notices.)
> 
> The fileno_stdin issue is even more odd as it took an opposite sense
> of _isatty(fileno_stdin) to keep it from reporting an error, but I
> don't see any connection between _commit() and stdin so it seemed
> most appropriate to simply remove the call for stdin.
> 
> IOW, I don't see the need to tie this to debug builds because the
> calls are invalid whether or not the RTL pops up the dialog.

In that case I can see the benefit to dropping the call entirely.

On Thu, 07 Nov 2013 07:09:18 -0800
Mike Rumph <mi...@oracle.com> wrote:

> Do you remember if the reasons for using _commit() would distinguish 
> between input and output?

No, not offhand.  In any case, they were in inverted order to clean up
any lingering input ahead of dup'ing or creating an apr file_t from an
apr_os_file_t.

Let's axe it.

Re: testdup test fails when compiled in debug mode on Windows

[Jeff Trawick <tr...@gmail.com>]

On Thu, Nov 7, 2013 at 12:33 AM, William A. Rowe Jr. <wr...@rowe-clan.net>wrote:

> On Wed, 06 Nov 2013 16:07:37 -0800
> Mike Rumph <mi...@oracle.com> wrote:
>
> >
> > On 11/6/2013 1:06 PM, Jeff Trawick wrote:
> > >
> > > I just played with _commit() on stdin a bit.  It turns out that
> > > _commit(0) fails if stdin is redirected (main.exe < somefile) but
> > > works if stdin is a tty.  That's the opposite of _commit(1 or 2).
> > > But I don't see how _commit(0) makes sense anyway, so I simply
> > > removed the call instead of reversing the corresponding _isatty()
> > > check in your patch.
> > >
> > > trunk: r1539455
> > > 1.5.x branch: r1539461
> > Okay Jeff,
> >
> > I just tried both stdout and stdin, and got the same results that you
> > did. Strange but true.
>
> IIRC the choice to _commit ahead of any handling of stdin/out/err
> reflected the possibility that bytes were queued/stuck in the FILE or
> the msvcrt 'fd' (not really an fd at all) before assuming ownership of
> the file handle.  It might have been an overreaction to a problem that
> wouldn't exist in practice.  But I'd prefer if this were left context
> sensitive to _DEBUG mode builds.
>

"The *_commit* function forces the operating system to write the file
associated with *handle* to disk"

_commit(fileno_stdout or fileno_stderr) fails if it refers to the terminal
whether or not it is a debug build.  It simply isn't a valid call.  I
called out the debug build issue in CHANGES because that is likely the only
case where anyone would encounter a problem symptom.  (_commit() otherwise
continues to return -1/EBADF and nobody notices.)

The fileno_stdin issue is even more odd as it took an opposite sense of
_isatty(fileno_stdin) to keep it from reporting an error, but I don't see
any connection between _commit() and stdin so it seemed most appropriate to
simply remove the call for stdin.

IOW, I don't see the need to tie this to debug builds because the calls are
invalid whether or not the RTL pops up the dialog.


-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Re: testdup test fails when compiled in debug mode on Windows

[Mike Rumph <mi...@oracle.com>]

Hello Bill,

That is a very good point, since the assert box will only appear under 
debug mode.
But when not in debug mode _commit(2) will send back a "-1" return code 
and set errno to EBADF,
if stderr is not redirected to a file.

The error existed in both cases, but the apr_file_dup2() function in 
apr/file_io/win32/filedup.c was simply ignoring the return code from 
_commit().  Perhaps it should be checked.

There is also the issue that Jeff uncovered for stdin having a reversed 
effect from stdout and stderr.
Do you remember if the reasons for using _commit() would distinguish 
between input and output?
We should probably also consider whether or not the fflush(stdin) call 
makes sense as well, dependent on whether or not stdin is redirected or 
piped in from another process.

Thanks,

Mike Rumph


On 11/6/2013 9:33 PM, William A. Rowe Jr. wrote:
> On Wed, 06 Nov 2013 16:07:37 -0800
> Mike Rumph <mi...@oracle.com> wrote:
>
>> On 11/6/2013 1:06 PM, Jeff Trawick wrote:
>>> I just played with _commit() on stdin a bit.  It turns out that
>>> _commit(0) fails if stdin is redirected (main.exe < somefile) but
>>> works if stdin is a tty.  That's the opposite of _commit(1 or 2).
>>> But I don't see how _commit(0) makes sense anyway, so I simply
>>> removed the call instead of reversing the corresponding _isatty()
>>> check in your patch.
>>>
>>> trunk: r1539455
>>> 1.5.x branch: r1539461
>> Okay Jeff,
>>
>> I just tried both stdout and stdin, and got the same results that you
>> did. Strange but true.
> IIRC the choice to _commit ahead of any handling of stdin/out/err
> reflected the possibility that bytes were queued/stuck in the FILE or
> the msvcrt 'fd' (not really an fd at all) before assuming ownership of
> the file handle.  It might have been an overreaction to a problem that
> wouldn't exist in practice.  But I'd prefer if this were left context
> sensitive to _DEBUG mode builds.
>

Re: testdup test fails when compiled in debug mode on Windows

["William A. Rowe Jr." <wr...@rowe-clan.net>]

On Wed, 06 Nov 2013 16:07:37 -0800
Mike Rumph <mi...@oracle.com> wrote:

> 
> On 11/6/2013 1:06 PM, Jeff Trawick wrote:
> >
> > I just played with _commit() on stdin a bit.  It turns out that 
> > _commit(0) fails if stdin is redirected (main.exe < somefile) but 
> > works if stdin is a tty.  That's the opposite of _commit(1 or 2).
> > But I don't see how _commit(0) makes sense anyway, so I simply
> > removed the call instead of reversing the corresponding _isatty()
> > check in your patch.
> >
> > trunk: r1539455
> > 1.5.x branch: r1539461
> Okay Jeff,
> 
> I just tried both stdout and stdin, and got the same results that you
> did. Strange but true.

IIRC the choice to _commit ahead of any handling of stdin/out/err
reflected the possibility that bytes were queued/stuck in the FILE or
the msvcrt 'fd' (not really an fd at all) before assuming ownership of
the file handle.  It might have been an overreaction to a problem that
wouldn't exist in practice.  But I'd prefer if this were left context
sensitive to _DEBUG mode builds.

Re: testdup test fails when compiled in debug mode on Windows

[Mike Rumph <mi...@oracle.com>]

On 11/6/2013 1:06 PM, Jeff Trawick wrote:
>
> I just played with _commit() on stdin a bit.  It turns out that 
> _commit(0) fails if stdin is redirected (main.exe < somefile) but 
> works if stdin is a tty.  That's the opposite of _commit(1 or 2).  But 
> I don't see how _commit(0) makes sense anyway, so I simply removed the 
> call instead of reversing the corresponding _isatty() check in your patch.
>
> trunk: r1539455
> 1.5.x branch: r1539461
Okay Jeff,

I just tried both stdout and stdin, and got the same results that you did.
Strange but true.

>
> -- 
> Born in Roswell... married an alien...
> http://emptyhammock.com/

Re: testdup test fails when compiled in debug mode on Windows

[Jeff Trawick <tr...@gmail.com>]

On Wed, Nov 6, 2013 at 2:13 PM, Jeff Trawick <tr...@gmail.com> wrote:

> On Wed, Nov 6, 2013 at 2:06 PM, Mike Rumph <mi...@oracle.com> wrote:
>
>> The apr_file_dup2() function in apr/file_io/win32/filedup.c calls
>> _commit() for standard file handles 0, 1 and 2.
>> The _commit() function will assert with the message "Invalid file
>> descriptor. File possibly closed by a different thread" or return a value
>> of -1 if the file handle refers to a device.
>> The assert will appear if APR is compiled in debug mode.
>>
>> This can be seen by running "testall.exe testdup".
>> But if file redirection is used (such as "testall.exe testdup > outfile
>> 2>&1"), then the test completes successfully.
>>
>> I have attached a patch that corrects this problem by checking _isatty().
>>
>> Credit goes to Jeff Trawick for catching this problem in the first place.
>>
>
> Well, there's a lot of unhappiness when you have to answer the
> abort/retry/ignore dialog when you run httpd with a debug flavor of apr ;)
>
>
>> Thanks,
>>
>> Mike Rumph
>>
>
> The existing flush-stdin path sure looks funny now that it is highlighted
> by the format of the patch, but that's not related to this bug and google
> seems to think that it is acceptable on Windows.
>
> I hope to commit shortly.  Thanks!
>
> --
> Born in Roswell... married an alien...
> http://emptyhammock.com/
>

I just played with _commit() on stdin a bit.  It turns out that _commit(0)
fails if stdin is redirected (main.exe < somefile) but works if stdin is a
tty.  That's the opposite of _commit(1 or 2).  But I don't see how
_commit(0) makes sense anyway, so I simply removed the call instead of
reversing the corresponding _isatty() check in your patch.

trunk: r1539455
1.5.x branch: r1539461

-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Re: testdup test fails when compiled in debug mode on Windows

[Jeff Trawick <tr...@gmail.com>]

On Wed, Nov 6, 2013 at 2:06 PM, Mike Rumph <mi...@oracle.com> wrote:

> The apr_file_dup2() function in apr/file_io/win32/filedup.c calls
> _commit() for standard file handles 0, 1 and 2.
> The _commit() function will assert with the message "Invalid file
> descriptor. File possibly closed by a different thread" or return a value
> of -1 if the file handle refers to a device.
> The assert will appear if APR is compiled in debug mode.
>
> This can be seen by running "testall.exe testdup".
> But if file redirection is used (such as "testall.exe testdup > outfile
> 2>&1"), then the test completes successfully.
>
> I have attached a patch that corrects this problem by checking _isatty().
>
> Credit goes to Jeff Trawick for catching this problem in the first place.
>

Well, there's a lot of unhappiness when you have to answer the
abort/retry/ignore dialog when you run httpd with a debug flavor of apr ;)


> Thanks,
>
> Mike Rumph
>

The existing flush-stdin path sure looks funny now that it is highlighted
by the format of the patch, but that's not related to this bug and google
seems to think that it is acceptable on Windows.

I hope to commit shortly.  Thanks!

-- 
Born in Roswell... married an alien...
http://emptyhammock.com/