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

[Peter Rosin <peda at lysator dot liu dot se>]

Csaba Raduly skrev 2012-03-27 09:37:
> On Mon, Mar 26, 2012 at 9:29 PM, Richard Gribble  wrote:
>> Using rcs 5.8-1:
>>
>> Synopsis:
>>    Given two mark symbols, abc (version 1.1) and abcd (version 1.2),
>>    executing "co -rabc <file>" will check out version 1.2, when it
>>    should check out version 1.1.
>>
>> I set the mark symbols as follows:
>>    rcs -nabc:1.1 -nabcd:1.2 <file>
>>
>>
>> I dug into it, and found the following (rcsrev.c):
>>
>> 01:  static char const *
>> 02:  rev_from_symbol (struct cbuf const *id)
>> 03:  /* Look up "id" in the list of symbolic names starting with pointer
>> 04:     "GROK (symbols)", and return a pointer to the corresponding
>> 05:     revision number.  Return NULL if not present.  */
>> 06:  {
>> 07:    for (struct link *ls = GROK (symbols); ls; ls = ls->next)
>> 08:      {
>> 09:        struct symdef const *d = ls->entry;
>> 10:
>> 11:        if (!strncmp (d->meaningful, id->string, id->size))
>> 12:          return d->underlying;
>> 13:      }
>> 14:    return NULL;
>> 15:  }
>>
>> Note that line 11 tests the name of the requested mark symbol
>> (id->string) against each element of a linked-list (ls) containing all
>> the mark symbols for the file.  The problem is that it will only test
>> the first 'id->size' characters - so R25 (from the command line) matches
>> R25a (from the list) because the first three characters match and it
>> won't test any more than that (strncmp).
>>
>> I recommend modifying line 11 as follows:
>>    if ((strlen(d->meaningful) == strlen(id->size)) && !strncmp
>> (d->meaningful, id->string, id->size))
> 
> I think strlen(id->size) is overkill :)
> 
> If both strings (d->meaningful and id->string) are NULL-terminated ,
> then a simple call to strlen would do the job:

You mean a simple call to strcmp here.

> if (!strcmp(d->meaningful, id->string))
> 
> It would do the right thing w.r.t. different lengths.
> 
>> Of course, since you now know that the two strings are the same size,
>> you could use strcmp - but that assumes that the && short-circuits, and
>> I don't know if that's guaranteed.
> 
> Yes, it required by the C standard.

But...careful!  You have to assume that the original authors were not
idiots!  Maybe the obvious strcmp was not used for a *good* reason?  I
can't tell if d->meaningful is guaranteed to be zero-terminated from the
limited context, but it sure is suspect.  Assuming that the original
authors were not idiots but instead made an off-by one error missing a
corner-case, the safer approach is to replace line 11 with:

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

That is, if id->string is guaranteed to be zero-terminated, which is
not clear either, but perhaps easier to verify...

Cheers,
Peter

--
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