Re: [libmicrohttpd] Segfault after MHD_quiesce_daemon()

[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);
>>
>

signature.asc
Description: OpenPGP digital signature