cvs 1.12 expand_path variable expansion bug

[Dan Riley]

On alphaev67-dec-osf4.0f, cvs 1.12.7 and 1.12.9, configured with

./configure '--enable-encryption' '--with-external-zlib' '--with-gssapi' 'CC=cc 
-std' 'CFLAGS=-fast' 'CPPFLAGS=-I/usr/local/include' 'LDFLAGS=-L/usr/local/lib'

with this at line 32 of CVSROOT/loginfo:

ALL     $CVSROOT/CVSROOT/log.pl -m dsr -m dab66 -m crn -f 
$CVSROOT/CVSROOT/commitlog %s

(also with s/%s/%{s}/), every cvs commit prints a message like

cvs commit: loginfo:32: no such internal variable $Cal/bin/túÿÿÿÿÿÿÿ

or

cvs commit: loginfo:32: no such internal variable $C

This did not happen with this line:

ALL     $CVSROOT/CVSROOT/log.pl -m dsr -m dab66 -f $CVSROOT/CVSROOT/commitlog %s
(shorter by 7 characters) and does not happen with cvs 1.11.17.

with a little debugging:

(ladebug) where  
#0  0x12003207c in 
expand_variable(name=0x14004203f="C.lns.cor\372\377\377\377\377\377\377\377", 
file=0x140025bf8="loginfo", line=32) "expand_path.c":333
>1  0x120031adc in expand_path(name=0x140037b64="$CVSROOT/CVSROOT/log.pl -m dsr 
>-m dab66 -m crn -f $CVSROOT/CVSROOT/commitlog %s", file=0x140025bf8="loginfo", 
>line=32, formatsafe=1) "expand_path.c":177
[...]

and in expand_path, p is no longer in mybuf:

(ladebug) p p  
0x14004203f="C.lns.cor\372\377\377\377\377\377\377\377"
(ladebug) p mybuf  
0x1400420a0="/home/cmpgrp/cvsroot/CVSROOT/log.pl -m dsr -m dab66 -m crn -f 
$CVSROOT"

because in this chunk of code:

            char *p = d;
            char *e;
            int flag = (*s == '{');

            doff = d - mybuf;
            expand_string (&mybuf, &mybuf_size, doff + 1);
            d = mybuf + doff;
            for (; (*d++ = *s); s++)
            {
                if (flag
                    ? *s =='}'
                    : isalnum ((unsigned char) *s) == 0 && *s != '_')
                    break;
                doff = d - mybuf;
                expand_string (&mybuf, &mybuf_size, doff + 1);
                d = mybuf + doff;
            }
            *--d = '\0';
            e = expand_variable (&p[flag], file, line);

p depends on mybuf not getting relocated.  Below is a proposed patch,
which changes p to an offset inside mybuf, similar to doff.  I have
not audited the routine for any other mistakes of this sort, but on
casual inspection it looks like the next block has the same problem
wrt the variable e:

            if (e)
            {
                doff = d - mybuf;
                expand_string (&mybuf, &mybuf_size, doff + 1);
                d = mybuf + doff;
                for (d = &mybuf[poff-1]; (*d++ = *e++);)

so I'd say someone needs to go over the whole routine carefully.  My
officemate suggests rewriting it in a language with a proper string
type--he recommends C++ :-).  A less radical possibility would be to
eliminate the pointer arithmetic and only use array style indexing
with mybuf[].

This patch also contains an unrelated fix, changing

                        *d++ = d[-1];

to

                        *d = d[-1];
                        ++d;

The former is undefined behavior--the value of d on the rhs is
undefined because there's no sequence point between the auto-increment
and the rhs.  "*d++ = '%';" would also work.

--- src/expand_path.c~  2004-03-22 10:37:34.000000000 -0500
+++ src/expand_path.c   2004-07-23 16:07:00.000000000 -0400
@@ -156,7 +156,7 @@
        }
        else if (*s++ == '$')
        {
-           char *p = d;
+           char poff = d - mybuf;
            char *e;
            int flag = (*s == '{');
 
@@ -174,14 +174,14 @@
                d = mybuf + doff;
            }
            *--d = '\0';
-           e = expand_variable (&p[flag], file, line);
+           e = expand_variable (&mybuf[poff+flag], file, line);
 
            if (e)
            {
                doff = d - mybuf;
                expand_string (&mybuf, &mybuf_size, doff + 1);
                d = mybuf + doff;
-               for (d = &p[-1]; (*d++ = *e++);)
+               for (d = &mybuf[poff-1]; (*d++ = *e++);)
                {
                    doff = d - mybuf;
                    expand_string (&mybuf, &mybuf_size, doff + 1);
@@ -209,7 +209,8 @@
                        doff = d - mybuf;
                        expand_string (&mybuf, &mybuf_size, doff + 1);
                        d = mybuf + doff;
-                       *d++ = d[-1];
+                       *d = d[-1];
+                       ++d;
                    }
                }
                --d;


-- 
Dan Riley                                         dsr@mail.lns.cornell.edu
Wilson Lab, Cornell University      <URL:http://www.lns.cornell.edu/~dsr/>
          "A new life awaits you in the off-world colonies"