Seg fault in http read event handler caused by rouge event call without context

[Maxim Dounin]

Hello!

On Mon, Nov 18, 2019 at 08:05:39PM +0100, Sergey Brester wrote:

> 18.11.2019 18:11, Maxim Dounin wrote: 
> 
> > Hello!
> > 
> > On Mon, Nov 18, 2019 at 05:48:46PM +0100, Sergey Brester wrote:
> > 
> >> Looks like [efd71d49bde0 [2]] could be indeed responsible for that: I see at least one state where rev->ready could remain 1 (after rev->available gets 0) e. g. deviation between blocks [efd71d49bde0#l10.8 [3]] and [efd71d49bde0#l11.8 [4]] where first did not reset rev->ready and for example if ngx_socket_nread in [efd71d49bde0#l10.38 [5]] would write 0 into rev->available, so rev->ready remains 1 yet.
> > 
> > There is no deviation here. The rev->available field holds the 
> > number of bytes available for reading (or -1 if it's not known), 
> > while rev->ready indicates if reading is possible. The reading 
> > can be possible even WHEN REV->AVAILABLE IS ZERO - notably, when 
> > there is a potential pending EOF.
> 
> Sure, 
> but I told about the case where rev->ready indicates the READING IS
> POSSIBLE, 
> but REV->AVAILABLE IS LESS AS ZERO. 
> 
> If you mean this is to noglect, then it is ok, but somehow it looks
> backwards incompatible to me.
> At least other async-IO procedures (for example KQUEUE [1]) handle this
> differently. 

The code you are referring to defines how nginx behaves when 
rev->available becomes zero or negative after it is decremented by 
the amount of bytes read.

In case of kqueue, rev->available is set to 0 to indicate there 
are no data to read, and rev->ready is also set to 0 unless 
pending EOF is indicated by kqueue.

In case of unspecified event methods, we cannot rely on the 
pending EOF indication (as this indication is only available with 
kqueue and with epoll with recent enough kernels).  As such, we 
can only reset rev->ready when rev->available becomes negative 
(and then set to 0), but not when rev->available becomes exactly 
zero.

> Anyway I saw never REV->AVAILABLE LESS AS ZERO, if rev->ready got 1
> until now.

This is explained in the commit log of the revant commit.  Quoting 
it here:

: With other event methods rev->available is now set to -1 when the socket
: is ready for reading.  Later in ngx_recv() and ngx_recv_chain(), if
: full buffer is received, real number of bytes in the socket buffer is
: retrieved using ioctl(FIONREAD).  Reading more than this number of bytes
: ensures that even with edge-triggered event methods the event will be
: triggered again, so it is safe to stop processing of the socket and
: switch to other connections.

Further, it is now explicitly documented in the relevant comment in
src/event/ngx_event.h, see here:

http://hg.nginx.org/nginx/rev/efd71d49bde0#l9.1

The rev->available set to -1 now indicates that there are 
unspecified amount of bytes to read.

Previously rev->available was never negative (and cannot be 
negative on most platforms, given that it was a single bit 
field), and certainly this is a change.  And in theory it can 
cause problems if some code relies on the previous semantics of 
rev->available being 0 or 1 with epoll, and checks for 
"rev->available == 1" for some reason (tests for "rev->available", 
"!rev->available" and "rev->available == 0" will work correctly 
without modifications).  But in practice I wouldn't expect this 
change to cause any problems with properly written code.

As already outlined in my reply to the original message, the  
reason for the segmentation faults observed seems to be failure of 
the upload module to block events after reading the request body.

-- 
Maxim Dounin
http://mdounin.ru/