This is the mail archive of the cygwin mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: rcs 5.8-1 checks out wrong version of file when using similar mark symbols


On 2012-03-28, Peter Rosin wrote:
> Gary Johnson skrev 2012-03-28 08:55:
> > On 2012-03-27, Peter Rosin wrote:
> >> But the point still stands, don't assume the original authors were
> >> idiots, and dig into the reasons for them to not having used
> >> strcmp from the start.
> > 
> > I don't know, the "original" authors seem to have gotten it right,
> > as version 5.7 works correctly on my Fedora system, and the function
> > in question was added between versions 5.7 and 5.8.
> 
> What are you trying to say here?  That whoever it was that brought
> rcs from 5.7 to 5.8 are a bunch of idiots?  I'm sure not, but that's
> what it sounds like...

I'm just saying that 5.7 works and 5.8 apparently does not.  Draw
you own conclusions.

One doesn't have to be an idiot to make a mistake.  The author of
that function just may not have been thinking clearly when he or she
wrote it, for any of a number of possible reasons.

> Cheech, I just said that it looked suspect that strcmp was not used
> from the start and that someone needed to look at the code and double-
> check if strlen/strcmp was safe to use before running full-steam into
> a segfault.
> 
> So, go look at the code.  I just did, and the suggested changes are
> indeed broken since the id string is *not* guaranteed to be zero-
> terminated.  It appears that the original authors (of 5.8 of course,
> that's the version we are discussing) are not idiots, since you can
> neither use strlen on the id string nor can you use strcmp on it.
> 
> However, it seems as if d->meaningful is zero-terminated (as far as
> I can tell strcmp, via the STR_SAME macro, is used on it at other
> locations in the code).

I wasn't defending the proposed changes, just saying that just
because code comes from a respected group of developers doesn't mean
it can't contain errors.

I didn't look at enough of the code to determine the characteristics
of those variables, but I'll take your word for it.  If
d->meaningful is null-terminated, id-string is not, and id->size is
the length of id->string, then this expression from the 5.8 source
is wrong:

11: ? ? ? ?if (!strncmp (d->meaningful, id->string, id->size))

as it ignores any characters in d->meaningful after the first
id->size characters.

> So, this is probably safe for line 11:
> 
>    if ((strlen(d->meaningful) == id->size) && !strncmp(d->meaningful, id->string, id->size))
> 
> If d->meaningful is not guaranteed to be zero-terminated, this bug
> is not fixable from within rev_from_symbol().

Agreed.

Regards,
Gary


--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]