[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] issue 1780: Keyword values with dollar signs causebadness.

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: 2006-02-14 23:51:54 CET

Lieven Govaerts wrote:
>>From: Julian Foad [mailto:julianfoad@btopenworld.com]
>
>>> if (b->keyword_off > SVN_KEYWORD_MAX_LEN ||
>>
>>Should that be ">="? By the time it's greater, we'll already
>>have written at least one character off the end of the buffer.
>
> I don't think so. b-keyword_off was post-incremented in the previous line,
> so we can't write off the end of the buffer.

If it's MAX+1 at this point, then the character was written into buffer[MAX],
which is off the end of the buffer:

   char keyword_buf[SVN_KEYWORD_MAX_LEN];

   b->keyword_buf[b->keyword_off++] = *p++;

Yes? (I notice that the "keyword_name" buffer has a length of MAX+1, but
"keyword_buf" doesn't. I don't think keyword_name needs to be that long; I
think MAX-2 characters, plus one for a null, is enough.)

>>Check what's going to happen at the boundary condition: will
>>it do the right thing for a valid keyword-pattern of exactly
>>MAX_LEN? MAX_LEN-1? MAX_LEN+1?
>>What about a non-keyword pattern of those lengths?
>
> I've added lines 76-82 in translate-test.c for testing for these border
> cases. They all seem to work as expected.

Thanks, but I don't think the too-long patterns (lines 81 and 82) are caught by
your code, they're caught by the subsequent existing test (because a non-'$'
character appears at position MAX). I think you need dollar signs in the
254th/255th/256th positions so that your code is executed at those positions.
Something like:

   $Author: aaaaaa$$$ $

... where the length up to the end of "$$$" is 254/255/256.

>>I think "b->keyword_off >= SVN_KEYWORD_MAX_LEN" will need to
>>go after "translate_keyword()", but see what you think.
>
> No problem with that.
>
> However, I don't see a change in behaviour when I implement the >= change
> nor the line-order change.

This is what I expect:

If we change ">" to ">=" only, then a keyword of length exactly MAX will not be
processed because "off >= MAX" will be true and will prevent the
"translate_keyword" call.

If we change ">" to ">=" and move the test to after the "translate_keyword"
call, then the keyword of length MAX will be processed.

> The new tests should have captured this change. I
> guess that b->keyword_off == SVN_KEYWORD_MAX_LEN will not be reached there
> because it's already handled earlier in the else block at line 998.

> The attached patch is based on yours and includes these two changes and some
> extra tests, no new changes in the algorithm itself.

Thanks. Let me know what you think about the above.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Feb 14 23:52:20 2006

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.