Created attachment 23104 [details] Backport of commit to 2.2.6 with fix Implementation of SSLRenegBufferSize in svn checkin 726109 was VERY welcome. I run 2.2.6 (Fedora), and have backported the fix. However, this uncovered a bug which applies to the checkin as well. The bug is that the code to default the buffer size is wrong. Because the per-directory structure member nRenegBufferSize is initialized to DEFAULT_RENEG_BUFFER_SIZE, cfgMergeInt will never override it with the directive value specified at a lower level. The fix is to initialize it to UNSET and handle the defaulting at point of use. I have attached the full backport patch file (with fix). To ease merging it into the trunk, the following is a diff of the backport >patch< file before vs. after this fix. I trust that this will be useful & that someone with commit privs will apply this to the source tree. diff ssl_post_renegotiate.patch.yours-backported ssl_post_renegotiate.patch.mine 34c34 { modules/ssl/ssl_engine_config.c } < + dc->nRenegBufferSize = DEFAULT_RENEG_BUFFER_SIZE; --- > + dc->nRenegBufferSize = UNSET; 166c166 { modules/ssl/ssl_engine_kernel.c } < @@ -510,15 +510,21 @@ --- > @@ -510,14 +510,23 @@ 175c175,178 < + if (dc->nRenegBufferSize > 0) { --- > + int rsize; > + rsize = dc->nRenegBufferSize; > + if( rsize == UNSET ) { rsize = DEFAULT_RENEG_BUFFER_SIZE; } > + if (rsize > 0) { 177c180 < + rv = ssl_io_buffer_fill(r, dc->nRenegBufferSize); --- > + rv = ssl_io_buffer_fill(r, rsize);
Note issue status
Thanks for the patch. Committed a slightly modified version as r733465.
Thanks for the quick action.
On reviewing your version of my patch more carefully - is apr_size_t guaranteed to be signed? I don't think it is. So there's a potential signedness issue that may bite some platforms. UNSET is defined as -1. DEFAULT_RENEG_BUFFER_SIZE is signed. nRenegBufferSize is an apr_size_t. Not pretty. I agree that apr_size_t is the right thing to use for the buffer size, but there are certainly compilers where -1 can never == unsigned. My version handwaved that by forcing a type conversion to int before the comparison...it was the minimal change to the orginal code. There are other approaches (such as casting UNSET to an apr_size_t) - a matter of style. (The other config variables that I noticed tended to use int in the structures.)
(In reply to comment #4) > On reviewing your version of my patch more carefully - is apr_size_t guaranteed > to be signed? > > I don't think it is. So there's a potential signedness issue that may bite > some platforms. This is a valid point since apr_size_t should be unsigned. Nevertheless I don't think that we have an issue here since IMHO -1 will be casted automatically to the maximum value an apr_size_t can hold.
I really don't want to be picky - httpd isn't my code. Perhaps a c language lawyer will explain the details. As indicated, my experience is that -1 does not necessarily compare equal to unsigned. I've seen (unsigned x == -1)? optimized to false on several compilers. FYI, gcc will warn about comparing signed to unsigned if you ask: cat x.c #include <stdio.h> int main( int argc, char **argv ) { unsigned int x; x = -1; if( argc == 7 ) x = 3; if( x == -1 ) printf( "equal\n" ); else printf( "not\n" ); return 0; } gcc -Wsign-compare x.c -o x.o x.c: In function ‘main’: x.c:10: warning: comparison between signed and unsigned (Yes, on my X86, this case will compare equal). If you make x a short (unlikely in real life), you'll get a "comparison is always false" error. This is correct according to the coersion/promotion rules as I understand them. (They are messier than 'cast to the maximum value x will hold') Another idiom is to compare unsigned x to (~0U/~0UL). You could have UNSET_U defined that way. and then add a cfgMergeUint. But that gets into a change to the whole defaulting mechanism, which seemed beyond the scope of just making this particular function work. You're responsible for the checkin, so I'm not going to pursue this further unless it actually bites me on one of my platforms after release. Thanks again for taking the time to make the checkin.