Re: [PATCH 1 of 3] Stream: socket peek in preread phase

[Roman Arutyunyan]

Hi,

On Wed, Dec 27, 2023 at 06:34:58PM +0400, Sergey Kandaurov wrote:
> On Wed, Dec 13, 2023 at 06:06:59PM +0400, Roman Arutyunyan wrote:
> 
> > # HG changeset patch
> > # User Roman Arutyunyan 
> > # Date 1702476295 -14400
> > #  Wed Dec 13 18:04:55 2023 +0400
> > # Node ID 844486cdd43a32d10b78493d7e7b80e9e2239d7e
> > # Parent  6c8595b77e667bd58fd28186939ed820f2e55e0e
> > Stream: socket peek in preread phase.
> > 
> > Previously, preread buffer was always read out from socket, which made it
> > impossible to terminate SSL on the connection without introducing additional
> > SSL BIOs.  The following patches will rely on this.
> > 
> > Now, when possible, recv(MSG_PEEK) is used instead, which keeps data in 
> > socket.
> > It's called if SSL is not already terminated and if an egde-triggered event
> > method is used.  For epoll, EPOLLRDHUP support is also required.
> > 
> > diff --git a/src/stream/ngx_stream_core_module.c 
> > b/src/stream/ngx_stream_core_module.c
> > --- a/src/stream/ngx_stream_core_module.c
> > +++ b/src/stream/ngx_stream_core_module.c
> > @@ -10,6 +10,10 @@
> >  #include 
> >  
> >  
> > +static ngx_int_t ngx_stream_preread_peek(ngx_stream_session_t *s,
> > +ngx_stream_phase_handler_t *ph);
> > +static ngx_int_t ngx_stream_preread(ngx_stream_session_t *s,
> > +ngx_stream_phase_handler_t *ph);
> >  static ngx_int_t ngx_stream_core_preconfiguration(ngx_conf_t *cf);
> >  static void *ngx_stream_core_create_main_conf(ngx_conf_t *cf);
> >  static char *ngx_stream_core_init_main_conf(ngx_conf_t *cf, void *conf);
> > @@ -203,8 +207,6 @@ ngx_int_t
> >  ngx_stream_core_preread_phase(ngx_stream_session_t *s,
> >  ngx_stream_phase_handler_t *ph)
> >  {
> > -size_t   size;
> > -ssize_t  n;
> >  ngx_int_trc;
> >  ngx_connection_t*c;
> >  ngx_stream_core_srv_conf_t  *cscf;
> > @@ -217,56 +219,40 @@ ngx_stream_core_preread_phase(ngx_stream
> >  
> >  if (c->read->timedout) {
> >  rc = NGX_STREAM_OK;
> > +goto done;
> > +}
> >  
> > -} else if (c->read->timer_set) {
> > -rc = NGX_AGAIN;
> > +if (!c->read->timer_set) {
> > +rc = ph->handler(s);
> >  
> > -} else {
> > -rc = ph->handler(s);
> > +if (rc != NGX_AGAIN) {
> > +goto done;
> > +}
> >  }
> >  
> > -while (rc == NGX_AGAIN) {
> > -
> > +if (c->buffer == NULL) {
> > +c->buffer = ngx_create_temp_buf(c->pool, 
> > cscf->preread_buffer_size);
> >  if (c->buffer == NULL) {
> > -c->buffer = ngx_create_temp_buf(c->pool, 
> > cscf->preread_buffer_size);
> > -if (c->buffer == NULL) {
> > -rc = NGX_ERROR;
> > -break;
> > -}
> > +rc = NGX_ERROR;
> > +goto done;
> >  }
> > -
> > -size = c->buffer->end - c->buffer->last;
> > -
> > -if (size == 0) {
> > -ngx_log_error(NGX_LOG_ERR, c->log, 0, "preread buffer full");
> > -rc = NGX_STREAM_BAD_REQUEST;
> > -break;
> > -}
> > +}
> >  
> > -if (c->read->eof) {
> > -rc = NGX_STREAM_OK;
> > -break;
> > -}
> > -
> > -if (!c->read->ready) {
> > -break;
> > -}
> > -
> > -n = c->recv(c, c->buffer->last, size);
> > +if (c->ssl == NULL
> > +&& (ngx_event_flags & NGX_USE_CLEAR_EVENT)
> > +&& ((ngx_event_flags & NGX_USE_EPOLL_EVENT) == 0
> > +#if (NGX_HAVE_EPOLLRDHUP)
> > +|| ngx_use_epoll_rdhup
> > +#endif
> 
> BTW, c->ssl needs to be guarded under an appropriate macro test.
> Probably, it makes sense to rewrite this in a more readable way.
> For example:
> 
> : peak = 0;
> : 
> : #if (NGX_HAVE_KQUEUE)
> : if (ngx_event_flags & NGX_USE_KQUEUE_EVENT) {
> : peak = 1;
> : }
> : #endif
> : 
> : #if (NGX_HAVE_EPOLLRDHUP)
> : if ((ngx_event_flags & NGX_USE_EPOLL_EVENT) && ngx_use_epoll_rdhup) {
> : peak = 1;
> : }
> : #endif
> : 
> : #if (NGX_STREAM_SSL)
> : if (c->ssl) {
> : peak = 0;
> : }
> : #endif

[..]

I think it's still too complicated.  I suggest a separate function:

diff --git a/src/stream/ngx_stream_core_module.c 
b/src/stream/ngx_stream_core_module.c
--- a/src/stream/ngx_stream_core_module.c
+++ b/src/stream/ngx_stream_core_module.c
@@ -10,6 +10,7 @@
 #include 
 
 
+static ngx_int_t ngx_stream_preread_can_peek(ngx_connection_t *c);
 static ngx_int_t ngx_stream_preread_peek(ngx_stream_session_t *s,
 ngx_stream_phase_handler_t *ph);
 static ngx_int_t ngx_stream_preread(ngx_stream_session_t *s,
@@ -238,14 +239,7 @@ ngx_stream_core_preread_phase(ngx_stream
 }
 }
 
-if (c->ssl == NULL
-&& (ngx_event_flags & NGX_USE_CLEAR_EVENT)
-&& ((ngx_event_flags & NGX_USE_EPOLL_EVENT) == 0
-#if (NGX_HAVE_EPOLLRDHUP)
-|| ngx_use_epoll_rdhup
-#endif
-  

Re: Core: Avoid memcpy from NULL

[Maxim Dounin]

Hello!

On Wed, Jan 03, 2024 at 11:57:57PM +, Ben Kallus wrote:

> > Still, general style guidelines suggests that the code shouldn't
> > be written this way, and the only reason for j++ in the line in
> > question is that it mimics corresponding IPv4 code.
> 
> > It's not "just happens".
> 
> The point I'm trying to make is that ensuring correctness with
> function-like macros is difficult, both because of operator precedence
> and argument reevaluation. Expecting contributors to read the
> definitions of every macro they use becomes more and more cumbersome
> as the codebase expands, especially when some symbols are variably
> macros or functions depending on the state of (even infrequently-used)
> compile-time constants.

Sure, and hence the style.

> All that said, upon further reflection, I think the UB issue is best
> solved outside of ngx_strcpy, where the overhead of an extra check may
> have a performance impact. The following patch is sufficient to
> silence UBSan in my configuration:

I've already pointed you to the Vladimir's patch which contains at 
least a dozen of places where the same "UB issue" is reported when 
running nginx tests with UBSan.  This demonstrates that your patch 
is clearly insufficient.  Further, Vladimir's patch is clearly 
insufficient too, as shown for the another patch in the same 
patch series.

If we want to follow this approach, we need some way to trace 
places where zero length memcpy() can occur.  My best guess is 
that the only way is to look through all ngx_cpymem() / 
ngx_memcpy() / ngx_memmove() / ngx_movemem() calls, as nginx 
routinely uses { 0, NULL } as an empty string.  Given that there 
are 600+ of such calls in the codebase, and at least some need 
serious audit to find out if { 0, NULL } can appear in the call, 
this is going to be a huge work.  And, given that the only 
expected effect is theoretical correctness of the code, I doubt it 
worth the effort, especially given that the end result will likely 
reduce readability of the code.

-- 
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel