Re: [Dnsmasq-discuss] Is wrapping close() in retry_send() required ?

2019-03-29 Thread Simon Kelley
On 26/03/2019 19:33, Pali Rohár wrote:
> On Wednesday 27 February 2019 17:07:21 Simon Kelley wrote:
>> On 27/02/2019 15:06, Bogdan Harjoc wrote:
>>> There are 50 calls to close() in dnsmasq-2.80, and 10 of them are
>>> wrapped in retry_send().
>>>
>>> "man close" has this paragraph in the section "Dealing with error
>>> returns from close":
>>>
>>> "Retrying the close() after a failure return is the wrong thing to do,
>>> since this may cause a reused file descriptor from another thread to
>>> be closed. This can occur because the Linux kernel always releases the
>>> file descriptor early in the close operation, freeing it for reuse;
>>> the steps that may return an error, such as flushing data to the
>>> filesystem or device, occur only later in the close operation".
>>>
>>> Dnsmasq is single-threaded so the retry_send() call is probably
>>> harmless. Are there other OSes that may return an error before the fd
>>> is released, in other words is there an OS where wrapping close in
>>> retry_send is required ?
>>
>>
>> Good questions.
>>
>> Note that retry_send() only deals with EINTR return codes, ie
>> interrupted system calls. (Ok there are other return codes in there, but
>> nothing which might be returned by close())
>>
>> So the use of retry_send(close(...)) is simply to restart the close()
>> syscall if a signal arrives during the syscall.
>>
>>
>> HOWEVER, whilst the man page for close() on my Linux machine states that
>> EINTR is a possible error return, man (7) signal does NOT include
>> close() in the set of syscalls which can be interrupted.
>>
>> Clearly I was reading the close() man page when I used the wrapper, and
>> signal man page when I didn't :)
>>
>>
>> You're probably right that it doesn't matter, but it would be nice to
>> make this at least consistent.
>>
>> Anyone know the answer?
> 
> See manpage: http://man7.org/linux/man-pages/man2/close.2.html
> 
> 
> The EINTR error is a somewhat special case.  Regarding the EINTR
> error, POSIX.1-2013 says:
> 
>If close() is interrupted by a signal that is to be caught, it
>shall return -1 with errno set to EINTR and the state of
>fildes is unspecified.
> 
> This permits the behavior that occurs on Linux and many other
> implementations, where, as with other errors that may be reported by
> close(), the file descriptor is guaranteed to be closed.  However, it
> also permits another possibility: that the implementation returns an
> EINTR error and keeps the file descriptor open.  (According to its
> documentation, HP-UX's close() does this.)  The caller must then once
> more use close() to close the file descriptor, to avoid file
> descriptor leaks.  This divergence in implementation behaviors
> provides a difficult hurdle for portable applications, since on many
> implementations, close() must not be called again after an EINTR
> error, and on at least one, close() must be called again.  There are
> plans to address this conundrum for the next major release of the
> POSIX.1 standard.
> 
> 
> So retrying close() on EINTR on Linux is an application bug. On the
> other hand it is required on HP-UX.
> 
>

Thanks Pali, that looks pretty definitive. As dnsmasq is supported on
Linux but not on HP-UX, I've made the relevant changes.


Cheers,

Simon.


___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss


Re: [Dnsmasq-discuss] Is wrapping close() in retry_send() required ?

2019-03-26 Thread Pali Rohár
On Wednesday 27 February 2019 17:07:21 Simon Kelley wrote:
> On 27/02/2019 15:06, Bogdan Harjoc wrote:
> > There are 50 calls to close() in dnsmasq-2.80, and 10 of them are
> > wrapped in retry_send().
> > 
> > "man close" has this paragraph in the section "Dealing with error
> > returns from close":
> > 
> > "Retrying the close() after a failure return is the wrong thing to do,
> > since this may cause a reused file descriptor from another thread to
> > be closed. This can occur because the Linux kernel always releases the
> > file descriptor early in the close operation, freeing it for reuse;
> > the steps that may return an error, such as flushing data to the
> > filesystem or device, occur only later in the close operation".
> > 
> > Dnsmasq is single-threaded so the retry_send() call is probably
> > harmless. Are there other OSes that may return an error before the fd
> > is released, in other words is there an OS where wrapping close in
> > retry_send is required ?
> 
> 
> Good questions.
> 
> Note that retry_send() only deals with EINTR return codes, ie
> interrupted system calls. (Ok there are other return codes in there, but
> nothing which might be returned by close())
> 
> So the use of retry_send(close(...)) is simply to restart the close()
> syscall if a signal arrives during the syscall.
> 
> 
> HOWEVER, whilst the man page for close() on my Linux machine states that
> EINTR is a possible error return, man (7) signal does NOT include
> close() in the set of syscalls which can be interrupted.
> 
> Clearly I was reading the close() man page when I used the wrapper, and
> signal man page when I didn't :)
> 
> 
> You're probably right that it doesn't matter, but it would be nice to
> make this at least consistent.
> 
> Anyone know the answer?

See manpage: http://man7.org/linux/man-pages/man2/close.2.html


The EINTR error is a somewhat special case.  Regarding the EINTR
error, POSIX.1-2013 says:

   If close() is interrupted by a signal that is to be caught, it
   shall return -1 with errno set to EINTR and the state of
   fildes is unspecified.

This permits the behavior that occurs on Linux and many other
implementations, where, as with other errors that may be reported by
close(), the file descriptor is guaranteed to be closed.  However, it
also permits another possibility: that the implementation returns an
EINTR error and keeps the file descriptor open.  (According to its
documentation, HP-UX's close() does this.)  The caller must then once
more use close() to close the file descriptor, to avoid file
descriptor leaks.  This divergence in implementation behaviors
provides a difficult hurdle for portable applications, since on many
implementations, close() must not be called again after an EINTR
error, and on at least one, close() must be called again.  There are
plans to address this conundrum for the next major release of the
POSIX.1 standard.


So retrying close() on EINTR on Linux is an application bug. On the
other hand it is required on HP-UX.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: PGP signature
___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss


Re: [Dnsmasq-discuss] Is wrapping close() in retry_send() required ?

2019-02-27 Thread Simon Kelley
On 27/02/2019 15:06, Bogdan Harjoc wrote:
> There are 50 calls to close() in dnsmasq-2.80, and 10 of them are
> wrapped in retry_send().
> 
> "man close" has this paragraph in the section "Dealing with error
> returns from close":
> 
> "Retrying the close() after a failure return is the wrong thing to do,
> since this may cause a reused file descriptor from another thread to
> be closed. This can occur because the Linux kernel always releases the
> file descriptor early in the close operation, freeing it for reuse;
> the steps that may return an error, such as flushing data to the
> filesystem or device, occur only later in the close operation".
> 
> Dnsmasq is single-threaded so the retry_send() call is probably
> harmless. Are there other OSes that may return an error before the fd
> is released, in other words is there an OS where wrapping close in
> retry_send is required ?


Good questions.

Note that retry_send() only deals with EINTR return codes, ie
interrupted system calls. (Ok there are other return codes in there, but
nothing which might be returned by close())

So the use of retry_send(close(...)) is simply to restart the close()
syscall if a signal arrives during the syscall.


HOWEVER, whilst the man page for close() on my Linux machine states that
EINTR is a possible error return, man (7) signal does NOT include
close() in the set of syscalls which can be interrupted.

Clearly I was reading the close() man page when I used the wrapper, and
signal man page when I didn't :)


You're probably right that it doesn't matter, but it would be nice to
make this at least consistent.

Anyone know the answer?


Cheers,

Simon.


___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss


[Dnsmasq-discuss] Is wrapping close() in retry_send() required ?

2019-02-27 Thread Bogdan Harjoc
There are 50 calls to close() in dnsmasq-2.80, and 10 of them are
wrapped in retry_send().

"man close" has this paragraph in the section "Dealing with error
returns from close":

"Retrying the close() after a failure return is the wrong thing to do,
since this may cause a reused file descriptor from another thread to
be closed. This can occur because the Linux kernel always releases the
file descriptor early in the close operation, freeing it for reuse;
the steps that may return an error, such as flushing data to the
filesystem or device, occur only later in the close operation".

Dnsmasq is single-threaded so the retry_send() call is probably
harmless. Are there other OSes that may return an error before the fd
is released, in other words is there an OS where wrapping close in
retry_send is required ?

Cheers,
Bogdan

___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss