httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Kraemer <mar...@apache.org>
Subject Re: [PATCH] proxy/ajp_header.c: Fix header detection
Date Fri, 31 Aug 2007 09:11:15 GMT
On Thu, Aug 30, 2007 at 06:24:55PM +0200, Rainer Jung wrote:
> >The patch replaces the memcmp by a strcmp to check for the trailing
> >NIL character, too.
> 
> For mod_jk the problem you found here is the same. Thanks for finding it!
> 
> We finally applied a slightly different patch, by keeping the memcmp, 
> but simply incrementing the number of bytes to compare by one. This 
> should work for mod_proxy also.
> 
[...]
> Our variant of the patch is at
> 
> http://marc.info/?l=tomcat-dev&m=118849057126771&w=2

On Thu, Aug 30, 2007 at 07:33:55PM +0200, jean-frederic clere wrote:
> 
> +1 mod_jk fixed it by additing one to each length, that is probably more
> efficent, no?

I had considered incrementing the byte count by one, too, yet I
went for

* readability
  A memcmp(xxx,"some arbitrary string", 22) is not really
  obviously a comparison of the string including its trailing NIL
  byte) -- I would have at least wrapped such a comparison into a
  commented macro like
    /* We use sizeof() to include comparison of the trailing NIL character
     * of the literal string, and use memcmp() instead of the
     * more obvious strcmp(cmpstr, literalconst) for efficiency reasons.
     */
    #define FAST_LITSTRING_STREQ(cmpstr, literalconst) \
            (0==memcmp(cmpstr, literalconst, sizeof(literalconst)))
  and then used
    if (FAST_LITSTRING_STREQ(p, "LANGUAGE")) {..}
  in place of
    if (memcmp(p, "LANGUAGE", 9) == 0) {..}
  which is, in spite of its absolute correctness, illegible.

* maintainability
  Few readers of the code will actually count the characters of all
  the bytes of the string "LANGUAGE" manually, to see that it has 8,
  in the line
    if (memcmp(p, "LANGUAGE", 9) == 0) {..}
  And because it is so non-obvious, a code maintainer adding code
  will add another if() with the incorrect byte count.

* efficiency
  Well, strcmp() is optimized on most platforms too, but I confess
  that, by definition, it has to waste some more machine
  instructions than memcpy(). However, maintainability and
  readability are higher values to me than utmost speed at the
  cost of maintainability.

Please go for "obvious" algorithms, or simply automate them (as in
the example macro above) rather than "coding in assembler code" for
efficiency, dropping even the slightest trace of explanation what
the code is intended to do, and leaving unmaintainable code behind you.

Because the strcmp() is so obvious, and does exactly what it's
supposed to do in this case, namely compare two strings over their
complete length, I went against the choice of "using memcmp for
efficiency" to save myself and many others from long nights of
head-scratching and -bashing to find out where some off-by-one
error was lingering deep in the AJP code.

Just my $.02,

   Martin
-- 
<Martin.Kraemer@Fujitsu-Siemens.com>        |     Fujitsu Siemens
http://www.fujitsu-siemens.com/imprint.html | 81730  Munich,  Germany

Mime
View raw message