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

Aha, this makes sense, and explains why random replacing of code didn't
trigger the tests to fail.

I changed the test lines 82-84 to contain the $$$ signs.

When I run the tests now, I see that we need the '>=' instead of '>' (
translate-test run will crash otherwise ). Moving the test "b->keyword_off
>= SVN_KEYWORD_MAX_LEN" before the translate will fail atleast the
translate-test in line 80. The new tests seem to confirm your assumption on
how the algorithm should behave, so I include them in the attached patch.

Thanks for taking your time on this patch.

Lieven.

> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> Sent: dinsdag 14 februari 2006 23:52
> To: Lieven Govaerts
> Cc: dev@subversion.tigris.org
> Subject: Re: [PATCH] issue 1780: Keyword values with dollar
> signs causebadness.
>
> 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