R devel - Apr 2008 - Bug in R 2.7 for over long lines (crasher+proposed fix!) (PR#11281)

OK, I am just sending it here too as it looks like r-devel at r-project.org
is not the right place:

=EF=BB=BFOn Fri, 2008-04-25 at 08:48 +0200, Soeren Sonnenburg
wrote:
> While trying to fix swig & R2.7 I actually discovered that there is a
> bug in R 2.7 causing a crash (so R & swig might actually work):
>=20
> the bug is in ./src/main/gram.c  line 3038:
>=20
>             } else { /* over-long line */
> fixthis --> char *LongLine =3D (char *) malloc(nc);
>             if(!LongLine)
>                 error(_("unable to allocate space for source line %
d"), xxlineno);
>             strncpy(LongLine, (char *)p0, nc);
>  bug -->    LongLine[nc] =3D '\0';
>             SET_STRING_ELT(source, lines++,
>                        mkChar2((char *)LongLine));
>             free(LongLine);
>=20
> note that LongLine is only nc chars long, so the
LongLine[nc]=3D'\0'
might
> be an out of bounds write. the fix would be to do
>=20
> =EF=BB=BF            char *LongLine =3D (char *) malloc(nc+1);
>=20
> in line 3034
>=20
> Please fix and thanks to dirk for the debian r-base-dbg package!
Looking at the code again there seems to be another bug above this for
the MAXLINESIZE test too:

        if (*p =3D=3D '\n' || p =3D=3D end - 1) {
            nc =3D p - p0;
            if (*p !=3D '\n')
            nc++;
            if (nc <=3D MAXLINESIZE) {
            strncpy((char *)SourceLine, (char *)p0, nc);
bug2 -->    SourceLine[nc] =3D '\0';
            SET_STRING_ELT(source, lines++,
                       mkChar2((char *)SourceLine));
            } else { /* over-long line */
            char *LongLine =3D (char *) malloc(nc+1);
            if(!LongLine)
                error(_("unable to allocate space for source line
%d"),
xxlineno);
bug1 -->    strncpy(LongLine, (char *)p0, nc);
            LongLine[nc] =3D '\0';
            SET_STRING_ELT(source, lines++,
                       mkChar2((char *)LongLine));
            free(LongLine);
            }
            p0 =3D p + 1;
        }


So I guess the test would be for nc < MAXLINESIZE above or to change
SourceLine to have MAXLINESIZE+1 size.

Alternatively as the strncpy manpage suggests do this for all
occurrences of strncpy

           strncpy(buf, str, n);
           if (n > 0)
               buf[n - 1]=3D =E2=80=99\0=E2=80=99;

this could even be made a makro / helper function ...

And another update: This does fix the R+swig crasher for me (tested)!

Soeren

bugreports at nn7.de wrote:
> OK, I am just sending it here too as it looks like r-devel at r-project.org
> is not the right place:
>   
I think it was seen there too, just that noone got around to reply. In 
R-bugs, there's a filing system so that it won't be completely
forgotten...

However, your mail seems to have gotten encoded in quoted-printable, you 
might want to follow up with a cleaned version. (Just keep the  
(PR#11281) in the header).
> =EF=BB=BFOn Fri, 2008-04-25 at 08:48 +0200, Soeren Sonnenburg wrote:
>   
>> While trying to fix swig & R2.7 I actually discovered that there is
a
>> bug in R 2.7 causing a crash (so R & swig might actually work):
>> =20
>> the bug is in ./src/main/gram.c  line 3038:
>> =20
>>             } else { /* over-long line */
>> fixthis --> char *LongLine =3D (char *) malloc(nc);
>>             if(!LongLine)
>>                 error(_("unable to allocate space for source line
%
>>     
> d"), xxlineno);
>   
>>             strncpy(LongLine, (char *)p0, nc);
>>  bug -->    LongLine[nc] =3D '\0';
>>             SET_STRING_ELT(source, lines++,
>>                        mkChar2((char *)LongLine));
>>             free(LongLine);
>> =20
>> note that LongLine is only nc chars long, so the
LongLine[nc]=3D'\0'
>>     
> might
>   
>> be an out of bounds write. the fix would be to do
>> =20
>> =EF=BB=BF            char *LongLine =3D (char *) malloc(nc+1);
>> =20
>> in line 3034
>> =20
>> Please fix and thanks to dirk for the debian r-base-dbg package!
>>     
>
> Looking at the code again there seems to be another bug above this for
> the MAXLINESIZE test too:
>
>         if (*p =3D=3D '\n' || p =3D=3D end - 1) {
>             nc =3D p - p0;
>             if (*p !=3D '\n')
>             nc++;
>             if (nc <=3D MAXLINESIZE) {
>             strncpy((char *)SourceLine, (char *)p0, nc);
> bug2 -->    SourceLine[nc] =3D '\0';
>             SET_STRING_ELT(source, lines++,
>                        mkChar2((char *)SourceLine));
>             } else { /* over-long line */
>             char *LongLine =3D (char *) malloc(nc+1);
>             if(!LongLine)
>                 error(_("unable to allocate space for source line
%d"),
> xxlineno);
> bug1 -->    strncpy(LongLine, (char *)p0, nc);
>             LongLine[nc] =3D '\0';
>             SET_STRING_ELT(source, lines++,
>                        mkChar2((char *)LongLine));
>             free(LongLine);
>             }
>             p0 =3D p + 1;
>         }
>
>
> So I guess the test would be for nc < MAXLINESIZE above or to change
> SourceLine to have MAXLINESIZE+1 size.
>
> Alternatively as the strncpy manpage suggests do this for all
> occurrences of strncpy
>
>            strncpy(buf, str, n);
>            if (n > 0)
>                buf[n - 1]=3D =E2=80=99\0=E2=80=99;
>
> this could even be made a makro / helper function ...
>
> And another update: This does fix the R+swig crasher for me (tested)!
>
> Soeren
>
> ______________________________________________
> R-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>   

-- 
   O__  ---- Peter Dalgaard             ?ster Farimagsgade 5, Entr.B
  c/ /'_ --- Dept. of Biostatistics     PO Box 2099, 1014 Cph. K
 (*) \(*) -- University of Copenhagen   Denmark      Ph:  (+45) 35327918
~~~~~~~~~~ - (p.dalgaard at biostat.ku.dk)              FAX: (+45) 35327907

Hi Soeren,
>>>>> "SS" == Soeren Sonnenburg <bugreports at
nn7.de>
>>>>>     on Sat, 10 May 2008 05:32:14 +0000 writes:
    SS> On Sat, 2008-04-26 at 09:38 +0200, Peter Dalgaard wrote:
    >> bugreports at nn7.de wrote: > OK, I am just sending it here
    >> too as it looks like r-devel at r-project.org > is not the
    >> right place:
    >> >   
    >> I think it was seen there too, just that noone got around
    >> to reply. In R-bugs, there's a filing system so that it
    >> won't be completely forgotten...

    SS> Looks like no one cares about this :(

Just "looks like" but it aint...

    SS> What should I do now? I mean I pointed directly to the
    SS> bug and did show how it could be fixed....

I'm not among the parse experts within R-core, but I think the
main problem with your report is that 
you talk about a crash but do not provide "self-contained
reproducible" code to produce such a crash, but just the
assertion that you get crashes when working on R <-> Swig
interaction.
Can you construct simple R code producing the crash?

Best regards
Martin


    >> However, your mail seems to have gotten encoded in
    >> quoted-printable, you might want to follow up with a
    >> cleaned version. (Just keep the (PR#11281) in the
    >> header).

    SS> To me crashers are critical bugs... isn't really no one
    SS> interested in seeing this fixed?

    >> > =EF=BB=BFOn Fri, 2008-04-25 at 08:48 +0200, Soeren
    >> Sonnenburg wrote:
    >> >   
    >> >> While trying to fix swig & R2.7 I actually discovered
    >> that there is a >> bug in R 2.7 causing a crash (so R &
    >> swig might actually work): >> =20 >> the bug is in
    >> ./src/main/gram.c line 3038: >> =20 >> } else { /*
    >> over-long line */ >> fixthis --> char *LongLine =3D (char
    >> *) malloc(nc); >> if(!LongLine) >> error(_("unable
to
    >> allocate space for source line %
    >> >>     
    >> > d"), xxlineno);
    >> >   
    >> >> strncpy(LongLine, (char *)p0, nc); >> bug -->
    >> LongLine[nc] =3D '\0'; >> SET_STRING_ELT(source,
lines++,
    >> >> mkChar2((char *)LongLine)); >> free(LongLine);
>> =20
    >> >> note that LongLine is only nc chars long, so the
    >> LongLine[nc]=3D'\0'
    >> >>     
    >> > might
    >> >   
    >> >> be an out of bounds write. the fix would be to do >>
    >> =20 >> =EF=BB=BF char *LongLine =3D (char *)
    >> malloc(nc+1); >> =20 >> in line 3034 >> =20
>> Please fix
    >> and thanks to dirk for the debian r-base-dbg package!
    >> >>     
    >> >
    >> > Looking at the code again there seems to be another bug
    >> above this for > the MAXLINESIZE test too:
    >> >
    >> > if (*p =3D=3D '\n' || p =3D=3D end - 1) { > nc =3D
p -
    >> p0; > if (*p !=3D '\n') > nc++; > if (nc <=3D
    >> MAXLINESIZE) { > strncpy((char *)SourceLine, (char *)p0,
    >> nc); > bug2 --> SourceLine[nc] =3D '\0'; >
    >> SET_STRING_ELT(source, lines++, > mkChar2((char
    >> *)SourceLine)); > } else { /* over-long line */ > char
    >> *LongLine =3D (char *) malloc(nc+1); > if(!LongLine) >
    >> error(_("unable to allocate space for source line %d"),
>
    >> xxlineno); > bug1 --> strncpy(LongLine, (char *)p0, nc);
    >> > LongLine[nc] =3D '\0'; > SET_STRING_ELT(source,
    >> lines++, > mkChar2((char *)LongLine)); > free(LongLine);
    >> > } > p0 =3D p + 1; > }
    >> >
    >> >
    >> > So I guess the test would be for nc < MAXLINESIZE above
    >> or to change > SourceLine to have MAXLINESIZE+1 size.
    >> >
    >> > Alternatively as the strncpy manpage suggests do this
    >> for all > occurrences of strncpy
    >> >
    >> > strncpy(buf, str, n); > if (n > 0) > buf[n - 1]=3D
    >> =E2=80=99\0=E2=80=99;
    >> >
    >> > this could even be made a makro / helper function ...
    >> >
    >> > And another update: This does fix the R+swig crasher
    >> for me (tested)!
    >> >
    >> > Soeren

    SS> Soeren

    SS> ______________________________________________
    SS> R-devel at r-project.org mailing list
    SS> https://stat.ethz.ch/mailman/listinfo/r-devel