On Tue, Dec 3, 2013 at 9:15 AM, Daniel Lescohier
<[email protected]>wrote:
> A null pointer will cause a segmentation fault. Is it guaranteed that the
> memory page that the pointer 0xdeadbeef points to is one that is not marked
> readable & writable, so that one would get a segmentation fault on
> reading/writing to it?
>
> However, the functions still need to change to return errors for the cases
> when they are unbuffered files. The buffer won't be referenced (and cause
> a crash) in those cases.
>
> I think the read/write functions should also do *nbytes=0 when filedes<0
> so caller's loops will stop looping.
>
For unbuffered files they'll generally get rv == EBADF for free, right?
And apr_file_read() for Unix seems to set *nbytes = 0 if an error occurs??
>
> In addition, apr_file_eof should return an error APR_EBADF:
>
> APR_DECLARE(apr_status_t) apr_file_eof(apr_file_t *fptr)
> {
> + if (fptr->filedes < 0) {
> + return APR_EBADF;
> + }
> if (fptr->eof_hit == 1) {
> return APR_EOF;
> }
> return APR_SUCCESS;
> }
>
>
(shrug)
* Cheap mechanisms in apr_FOO_close to help catch application bugs later
(e.g., invalidating pointers or file descriptors): awesome
* Adding sanity checking like this for application bugs: never-ending work
* Crashes due to application bugs: fine with me
* Negative feedback due to application bugs as long as we don't have to
check for the condition: awesome
* Hangs or incorrect/invalid feedback due to application bugs when
otherwise APR would require explicit code to check for it: fine with me
But then this particular mod_cache_disk bug doesn't seem so interesting to
me personally :) (My own Mileage May Vary)
Maybe the extra sanity check is not in an API called over and over, so it
doesn't clutter the main path. (not so bad)
>
> On Tue, Dec 3, 2013 at 8:41 AM, Stefan Ruppert <[email protected]> wrote:
>
>> Am 03.12.2013 14:18, schrieb Jeff Trawick:
>>
>>> On Tue, Dec 3, 2013 at 3:16 AM, Stefan Ruppert <[email protected]
>>> <mailto:[email protected]>> wrote:
>>>
>>> Am 03.12.2013 00:37, schrieb William A. Rowe Jr.:
>>>
>>> On Mon, 02 Dec 2013 01:34:58 +0100
>>> Branko Čibej <[email protected] <mailto:[email protected]>> wrote:
>>>
>>> On 02.12.2013 01:29, Eric Covener wrote:
>>>
>>> I am looking at a httpd bug that causes a hang on
>>> windows but
>>> succeeds on unix.
>>>
>>> It seems that (short) files are opened w/ buffering,
>>> read,
>>> apr_file_closed, and read again [succesfully on unix]
>>>
>>> On Unix, they sare satisfied out of the buffer.
>>> file->fileset is
>>> -1. On Windows, the destroyed apr_thread_mutex causes a
>>> hang.
>>>
>>> Is reading from the closed file on the extreme bogus end
>>> of the
>>> spectrum as I suspect and just getting lucky on the unix
>>> case?
>>>
>>>
>>> I'd certainly call a successful read from a closed file a
>>> bug.
>>>
>>> Should they blow up in 2.0 during a read if they've been
>>> closed?
>>>
>>>
>>> Dunno ... my gut feeling in cases like this is to just leave
>>> it be.
>>> Developers should have some fun finding heisenbugs, too. :)
>>>
>>>
>>> If we fix this, it needs to be minimal impact. Zero'ing out the
>>> buffer on close seems like the lowest cpu impact.
>>>
>>>
>>> A minimal impact and IMHO the correct fix is to return an error in
>>> buffered I/O if the file was closed.
>>>
>>> An application should not call a read or write function on a closed
>>> file. If it does its a bug and it should be notified about that fact.
>>>
>>>
>>> I keep telling myself over the years that APR's propensity to crash when
>>> called in situations as wrong as this (quite different from proprietary
>>> libraries with other considerations) is actually very good for code
>>> quality in the long run.
>>>
>>> What's the least we can do for this case that avoids having to check
>>> good calls for validity and yet give unmistakable feedback? Clear the
>>> buffer pointer during close?
>>>
>>
>> You are absolutly right, after a close the file structure isn't valid
>> anymore thus checking the file descriptor is a shot in the dark...
>>
>> Maybe not clearing the buffer pointer but assigning 0xdeadbeef to force a
>> crash?
>>
>> Regards,
>> Stefan
>>
>>
>
--
Born in Roswell... married an alien...
http://emptyhammock.com/
