Re: [libmicrohttpd] Issue on receiving very long URI

[Christian Grothoff]

Dear Shikha,

I'm not sure how this can happen, as the function specifically first
computes the available space in the pool before making the allocation
request.

Could you please check if the

if (pool->pos == ROUND_TO_ALIGN (old_offset + old_size))

condition in memorypool.c::MHD_pool_reallocate() was 'true' as one would
expect? Overall it would be good to know which path is taken through
MHD_pool_reallocate() and/or what the various variables ended up being
when 'NULL' was returned. I've looked at the code for a while now, but
could not find anything wrong :-(.

So please help; providing a failing test would also be great.

I have adjusted the code to 'assert' properly if the function
unexpectedly returns NULL. Alas, that'll just convert the
NULL-dereferencing into an abort().

Happy hacking!

Christian

On 8/19/20 9:25 AM, Shikha Sharma wrote:
> Hi
> 
> I have observed a crash in MHD after using very long URI. In function
> 
> int
> try_grow_read_buffer (struct MHD_Connection *connection)
> {
>     size_t new_size;
>   size_t avail_size;
> 
>   avail_size = MHD_pool_get_free (connection->pool);
>   if (0 == avail_size)
>     return false; /* No more space available */
>   if (0 == connection->read_buffer_size)
>     new_size = avail_size / 2; /* Use half of available buffer for
> reading */
>   else
>     {
>       size_t grow_size;
> 
>       grow_size = avail_size / 8;
>       if (MHD_BUF_INC_SIZE > grow_size)
>       { /* Shortage of space */
>           /* Shortage of space, but grow is mandatory */
>           static const size_t small_inc = MHD_BUF_INC_SIZE / 8;
>           if (small_inc < avail_size)
>               grow_size = small_inc;
>           else
>               grow_size = avail_size;
>       }
>       new_size = connection->read_buffer_size + grow_size;
>     }
>   /* we can actually grow the buffer, do it! */
>   connection->read_buffer = MHD_pool_reallocate (connection->pool,
> connection->read_buffer,
> connection->read_buffer_size,
>                                                  new_size);
>   mhd_assert (NULL != connection->read_buffer);
>   connection->read_buffer_size = new_size;
>   return MHD_YES;
> }
> 
> 
> In my scenario, MHD_pool_reallocate returns NULL as it can not
> reallocate the memory. Thus connection->read_buffer is set to NULL and
> the function returns MHD_YES as if it was able to grow the buffer.
> 
> The next access to connection->read_buffer leads to crash.
> 
> 
> Debug trace for MHD_pool_reallocate returning NULL :
> 
> Breakpoint 2, MHD_pool_reallocate (pool=0x7fffa0008d90,
> old=0x7fffa0000c70, old_size=23164, new_size=new_size@entry=24364) at
> memorypool.c:252
> 252    {
> (gdb) p *pool
> $88 = {memory = 0x7fffa0000c70 "GET
> /q/init-req/val1/callingParty/316543216/hostid/bfx1/SessionId/123/calledParty/1415926535897932384626433832795028841971693993751058209749445923078164062862089986280348253421170679821480865132823066"...,
> size = 32768, pos = 23168, end = 32768, is_mmap = 0}
> (gdb) n
> 256      asize = ROUND_TO_ALIGN (new_size);
> (gdb)
> 257      if ( (0 == asize) &&
> (gdb)
> 260      if ( (pool->end < old_size) ||
> (gdb)
> 264      if ( (pool->pos >= old_size) &&
> (gdb)
> 265           (&pool->memory[pool->pos - old_size] == old) )
> (gdb)
> 264      if ( (pool->pos >= old_size) &&
> (gdb)
> 281      if (asize <= old_size)
> (gdb)
> 283      if ((pool->pos + asize >= pool->pos) &&
> (gdb)
> 297    }
> (gdb)
> 259        return NULL; /* new_size too close to SIZE_MAX */
> (gdb)
> 297    }
> 
> The functionality of try_grow_read_buffer was changed and the snippet
> above is latest. In the old functionality:
> 
> buf = MHD_pool_reallocate (connection->pool,
>                              connection->read_buffer,
>                              connection->read_buffer_size,
>                              new_size);
>   if (NULL == buf)
>     return MHD_NO;
>   /* we can actually grow the buffer, do it! */
>   connection->read_buffer = buf;
>   connection->read_buffer_size = new_size;
>   return MHD_YES;
> 
> 
> which returns MHD_NO in case reallocation fails.
> 
> Thanks & Regards,
> 
> Shikha
> 
> 
>

signature.asc
Description: OpenPGP digital signature