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
--
<[EMAIL PROTECTED]> | Fujitsu Siemens
http://www.fujitsu-siemens.com/imprint.html | 81730 Munich, Germany
