httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mathihalli, Madhusudan" <mad...@hp.com>
Subject RE: [PATCH] Fix SEGV in ssl_scache_shmcb.c
Date Tue, 11 May 2004 00:50:01 GMT

>-----Original Message-----
>From: Geoff Thorpe [mailto:geoff@geoffthorpe.net] 
[SNIP]
>
>Just one :-) I hadn't been particularly clear about something 
>so wires may 
>have got crossed, there is a second patch lurking around and 
>it's purpose 
>is overlapped with the one you posted. The patch you sent reduces the 
>memcpy() overhead to the minimum required whereas previously it was 
>pegged at the maximum possible. The cost for that is the addition of 
>another member variable in the index structure. However the use of 
>"maximal" memcpy over "minimum" memcpy was not the bug, just an 
>inelegance of the code. The real bug was that no check was being made 
>that the size of the desired memcpy was less than the size of the 
>(sub-)cache, no matter whether it was maximal or minimal! :-) 
>I think the 
>bug would have been triggered by maximal and minimal 
>scenarios, provided 
>you used small enough cache sizes (less than 256kb) and waited long 
>enough.

Well.. This SEGV bug has to happen the very first time if and ONLY if
the session data is bigger than the cache size - it may never happen
otherwise.
Moreover the problem does not show up when we're writing the session
data into the cache for the first time - it's the first lookup that
causes the problem. This is because when we write to the cache, we use
the 'encoded_len' as the size - which is smaller than the cache size and
is the correct method. The patch you posted fixes the case where
encoded_len is bigger than the cache size. So, we definitely require the
patch.

However, for retrieval of the session data, we assume a standard size
(SSL_SESSION_MAX_DER) - this is wrong (in our case). Your patch
certainly fixes the problem - but is NOT a optimal fix for the problem.
We should retrieve only the same number of bytes that we've written -
for both correctness and performance.

As regards the patch attached, I'd propose to move the checking in the
initialization routine (buf_size should be atleast SSL_SESSION_MAX_DER)
rather than checking everytime (for bettter performance)

-Madhu

Mime
View raw message