Re: [libmicrohttpd] Segfault after MHD_quiesce_daemon()

[Markus Doppelbauer]

Hi,

Maybe it is not allowed to call    MHD_get_daemon_info( MHD_DAEMON_INFO_CURRENT_CONNECTIONS )
at any time from any thread? Maybe only from    MHD_NotifyConnectionCallback()  ?

Thanks a lot
Markus



Am Samstag, den 18.07.2015, 17:18 +0200 schrieb Markus Doppelbauer:
>     Hi,
>     
>     Sorry - I can't provide a testcase - I only have the coredumps.
>     IMHO the problem is, that two threads work in the same critical section    MHD_cleanup_connections()
>     
>     #1 MHD-Thread from microhttpd/daemon.c:2937     from    MHD_select_thread()
>     #2 Worker-thread from microhttpd/daemon.c:4630    from    MHD_get_daemon_info( MHD_DAEMON_INFO_CURRENT_CONNECTIONS )
>     
>     Both threads meet each other in       MHD_cleanup_connections()
>     Down below an other core-dump. In that case the worker-thread crashed.
>     
>     Thanks a lot
>     Markus
>     
>     
>     
>     # Main-Thread after MHD_quiesce_daemon()
>     
>     #1  0x0000003bd6e33e05 in abort () from /lib64/libc.so.6
>     No symbol table info available.
>     #2  0x0000003bd6e70537 in __libc_message () from /lib64/libc.so.6
>     No symbol table info available.
>     #3  0x0000003bd6e75e66 in malloc_printerr () from /lib64/libc.so.6
>     No symbol table info available.
>     #4  0x000000000045fc2a in MHD_cleanup_connections (daemon=0x28d8bf0) at microhttpd/daemon.c:2038
>             pos = 0x7fb4f00009c0
>     #5  0x000000000045ff54 in MHD_get_daemon_info (daemon=0x28cf9f0, info_type=<value optimized out>) at microhttpd/daemon.c:4630
>             i = <value optimized out>
>     
>     # MHD-Thread
>     
>     #0  0x0000003bd720e264 in __lll_lock_wait () from /lib64/libpthread.so.0
>     No symbol table info available.
>     #1  0x0000003bd7209508 in _L_lock_854 () from /lib64/libpthread.so.0
>     No symbol table info available.
>     #2  0x0000003bd72093d7 in pthread_mutex_lock () from /lib64/libpthread.so.0
>     No symbol table info available.
>     #3  0x00000000006c73fe in http_NotifyConnectionCallback (cls=0x7fff32d7a1e0, connection=<value optimized out>, socket_context=<value optimized out>, toe=<value optimized out>) at global/daemon.cpp:1782
>             daemonqueue = 0x7fff32d7a1e0
>     #4  0x000000000045fc5a in MHD_cleanup_connections (daemon=0x28d8bf0) at microhttpd/daemon.c:2046
>             pos = 0x7fb4f00009c0
>     #5  0x0000000000463a25 in MHD_select_thread (cls=0x28d8bf0) at microhttpd/daemon.c:2937
>             daemon = 0x28d8bf0
>     #6  0x0000003bd72079d1 in start_thread () from /lib64/libpthread.so.0
>     No symbol table info available.
>     #7  0x0000003bd6ee88fd in clone () from /lib64/libc.so.6
>     No symbol table info available.
>     
>     
>     
>     
>     
>     
>     
>     
>     Am Samstag, den 18.07.2015, 16:31 +0200 schrieb Christian Grothoff: 
> > Hi!
> >
> > I'm sorry, but I still don't even see how the race could happen.
> > I checked all calls to MHD_pool_destroy, and the respective connection
> > is always in 1 of 3 disjoint ownership states:
> >
> > 1) never aliased, about to be freed (failures during connection setup)
> > 2) 'active' connection, going down without keep-alive, pool is freed
> >    'early' during the handler thread (technically not necessary, we
> >    could keep it around until case (3), but that'd keep the RAM busy
> >    longer than necessary).  This should only happen from the thread
> >    that handles everything wrt. this connection.
> > 3) MHD_cleanup_connections going over all connections that are really
> >    dead; here, a lock is used.
> >
> > Note that in 1&2, MHD_cleanup_connections() would never touch that pool,
> > as it is not in the respective DLL.
> >
> > So sorry, I cannot reproduce your issue.  Again, a testcase would be
> > very helpful...
> >
> > As for the need of the info call to invoke cleanup: it is necessary, as
> > otherwise the counter could be way off (I think especially in the case
> > you describe, it could cause non-termination to not call cleanup).
> >
> > Now, the MHD_pool_destroy in connection.c, that one should be truly
> > harmless to remove, so you could safely play with that...
> >
> >
> > Happy hacking!
> >
> > -Christian
> >
> > On 07/17/2015 09:57 AM, Markus Doppelbauer wrote:
> > > Sorry the NULL patch is the wrong fix - it makes it only more
> > > unlikely to double-free() the pool. The scheduler could interrupt
> > > right before "pos->pool = NULL".
> > > 
> > > Maybe the only solution is either to protect "MHD_cleanup_connections()"
> > > with a mutex or to remove the call to "MHD_cleanup_connections()"
> > > in "MHD_DAEMON_INFO_CURRENT_CONNECTIONS".
> > > 
> > > Thanks a lot
> > > Markus
> > > 
> > > 
> > > 
> > > Am Donnerstag, den 16.07.2015, 22:13 +0200 schrieb Markus Doppelbauer:
> > >> Hello,
> > >>
> > >> Maybe simply nullify "pos->pool" after MHD_pool_destroy()"?
> > >> Should avoid this double-free().
> > >>
> > >> Or is there a chance to get the number of open connections
> > >> without calling "MHD_cleanup_connections()"?
> > >>
> > >> Thanks a lot
> > >> Markus
> > >>
> > >>
> > >>             }
> > >>         }
> > >>         MHD_pool_destroy (pos->pool);
> > >> +       pos->pool = NULL;
> > >>   #if HTTPS_SUPPORT
> > >>         if (NULL != pos->tls_session)
> > >>         gnutls_deinit (pos->tls_session);
> > >>
> > > 
> >
> >     
>