[PATCH] Mail: fixed handling of blocked client read events in proxy

[Maxim Dounin]

Hello!

On Wed, Mar 22, 2023 at 08:38:52PM +0400, Roman Arutyunyan wrote:

> On Sat, Mar 11, 2023 at 02:24:49PM +0300, Maxim Dounin wrote:
> > # HG changeset patch
> > # User Maxim Dounin <mdounin at mdounin.ru>
> > # Date 1678533841 -10800
> > #      Sat Mar 11 14:24:01 2023 +0300
> > # Node ID b97f2b983d1564d29280d03828503edca21a79ee
> > # Parent  8771d35d55d0a2b1cefaab04401d6f837f5a05a2
> > Mail: fixed handling of blocked client read events in proxy.
> > 
> > When establishing a connection to the backend, nginx blocks reading
> > from the client with ngx_mail_proxy_block_read().  Previously, such
> > events were lost, and in some cases this resulted in connection hangs.
> > 
> > Notably, this affected mail_imap_ssl.t on Windows, since the test
> > closes connections after requesting authentication, but without
> > waiting for any responses (so the connection close events might be
> > lost).
> 
> The following patch alowed me to trigger the issue on my laptop.
> It slows down the auth http backend and switches event processing to select.
> 
> diff --git a/mail_imap_ssl.t b/mail_imap_ssl.t
> --- a/mail_imap_ssl.t
> +++ b/mail_imap_ssl.t
> @@ -42,6 +42,7 @@ my $t = Test::Nginx->new()->has(qw/mail 
>  daemon off;
>  
>  events {
> +    use select;
>  }
>  
>  mail {
> @@ -104,6 +105,7 @@ http {
>      server {
>          listen       127.0.0.1:8080;
>          server_name  localhost;
> +        limit_rate   100;
>  
>          location = /mail/auth {
>              access_log auth.log test;
> 
> > Fix is to post an event to read from the client after connecting to
> > the backend if there were blocked events.
> > 
> > diff --git a/src/mail/ngx_mail_proxy_module.c b/src/mail/ngx_mail_proxy_module.c
> > --- a/src/mail/ngx_mail_proxy_module.c
> > +++ b/src/mail/ngx_mail_proxy_module.c
> > @@ -327,7 +327,9 @@ ngx_mail_proxy_pop3_handler(ngx_event_t 
> >          c->log->action = NULL;
> >          ngx_log_error(NGX_LOG_INFO, c->log, 0, "client logged in");
> >  
> > -        if (s->buffer->pos < s->buffer->last) {
> > +        if (s->buffer->pos < s->buffer->last
> > +            || s->connection->read->ready)
> > +        {
> >              ngx_post_event(c->write, &ngx_posted_events);
> >          }
> >  
> > @@ -486,7 +488,9 @@ ngx_mail_proxy_imap_handler(ngx_event_t 
> >          c->log->action = NULL;
> >          ngx_log_error(NGX_LOG_INFO, c->log, 0, "client logged in");
> >  
> > -        if (s->buffer->pos < s->buffer->last) {
> > +        if (s->buffer->pos < s->buffer->last
> > +            || s->connection->read->ready)
> > +        {
> >              ngx_post_event(c->write, &ngx_posted_events);
> >          }
> >  
> > @@ -821,7 +825,9 @@ ngx_mail_proxy_smtp_handler(ngx_event_t 
> >          c->log->action = NULL;
> >          ngx_log_error(NGX_LOG_INFO, c->log, 0, "client logged in");
> >  
> > -        if (s->buffer->pos < s->buffer->last) {
> > +        if (s->buffer->pos < s->buffer->last
> > +            || s->connection->read->ready)
> > +        {
> >              ngx_post_event(c->write, &ngx_posted_events);
> >          }
> 
> Looks ok

Thanks for the review, pushed to http://mdounin.ru/hg/nginx.

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