R devel - Feb 2017 - another fix for R crashes under enable-strict-barrier, lto, trunk@72156

On 2nd thought, I think a better fix to the segfault is something like this:

--- a/src/main/memory.c
+++ b/src/main/memory.c
@@ -3444,6 +3444,8 @@ R_xlen_t (XTRUELENGTH)(SEXP x) { return
XTRUELENGTH(CHK2(x)); }
 int  (IS_LONG_VEC)(SEXP x) { return IS_LONG_VEC(CHK2(x)); }
 
 const char *(R_CHAR)(SEXP x) {
+    if(!x)
+       error("de-referncing null. Check the validity of your data.");
     if(TYPEOF(x) != CHARSXP)
        error("%s() can only be applied to a '%s', not a
'%s'",
              "CHAR", "CHARSXP", type2char(TYPEOF(x)));


The segfault happens in the middle of tests/no-segfault.R . I have since built R
3.2.x and 3.3.x with --enable-strict-barrier so this is probably new to R 3.4. I
think
 tests/no-segfault.R is supposed to try to trigger a segfault with invalid data,
and it is supposed to be caught. That it isn't caught with some combinations
of configure is obviously broken; OTOH, testing for segfault with invalid data
is also intentional; so I think a better solution is to be verbose about it,
instead of what I suggested earlier, silently letting the invalid data through
and let upstream cope.

I had a stack trace - but it wasn't obvious where-else a check should be
made. The segfault happens is in the eval loop, which is fairly general by
itself.

In any case, that was the whole point of me having   --enable-memory-profiling
--enable-strict-barrier --with-valgrind-instrumentation=2 : I work(ed) with
people who like to write buggy code. Invalid input data and invalid stuff
somewhere in the middle is expected, from these people...

To be honest, I think a few more null checks and a few more tests in
tests/no-segfault.R could be added.

--------------------------------------------
On Mon, 2/20/17, Martin Maechler <maechler at stat.math.ethz.ch> wrote:

 Subject: Re: [Rd] another fix for R crashes under enable-strict-barrier, lto,
trunk at 72156
 To: "Hin-Tak Leung" <htl10 at users.sourceforge.net>
 Cc: r-devel at r-project.org, "bonsai list"
<outmodedbonsai-announce at lists.sourceforge.net>
 Date: Monday, February 20, 2017, 9:56 AM

 >>>>>
 Hin-Tak Leung <htl10 at users.sourceforge.net>
 >>>>>? ???on Sat, 11
 Feb 2017 19:30:26 +0000 writes:

 ? ? > I haven' t touched R for some 18
 months, and so I have no
 ? ? > idea if
 this is a recent problems or not; but it certainly
 ? ? > did not segfault two years ago.?
 Since it has been
 ? ? > crashing
 (segfault) under 'make check-all' for over a
 ? ? > month, I reckon I'll have to
 look at it myself, to have it
 ? ? >
 fixed.

 ? ? > I have
 been having the ' --enable-memory-profiling
 --enable-strict-barrier
 --with-valgrind-instrumentation=2" options

 ? ? > for perhaps a
 decade - because I work(ed) with people who
 ? ? > like to write buggy code :-(. And I
 also run 'make
 ? ? > check-all'
 from time to time until two years ago.

 ? ? > ./configure
 --enable-memory-profiling --enable-strict-barrier
 --enable-byte-compiled-packages
 --with-valgrind-instrumentation=2 --enable-lto

 ? ? > current R dev
 crashes in make check-all . The fix is this:


 ? ? >
 --- a/src/main/memory.c
 ? ? > +++
 b/src/main/memory.c
 ? ? > @@ -3444,7
 +3444,7 @@ R_xlen_t (XTRUELENGTH)(SEXP x) { return
 XTRUELENGTH(CHK2(x)); }
 ? ? >? int?
 (IS_LONG_VEC)(SEXP x) { return IS_LONG_VEC(CHK2(x)); }

 ? ? >? const char
 *(R_CHAR)(SEXP x) {
 ? ? > -? ?
 if(TYPEOF(x) != CHARSXP)
 ? ? > +? ?
 if(x && (TYPEOF(x) != CHARSXP))
 ?
 ? >? ? ? ???error("%s() can only be
 applied to a '%s', not a '%s'",
 ? ? >? ? ? ? ? ?
 ???"CHAR", "CHARSXP",
 type2char(TYPEOF(x)));
 ? ? >? ? ?
 return (const char *)CHAR(x);


 ? ? > It is a fairly
 obvious fix to a bug since

 ? ? > include/Rinternals.h:#define
 TYPEOF(x) ((x)->sxpinfo.type)

 ? ? > and it was trying to de-reference
 "0->sxpinfo.type" (under
 ? ?
 > --enable-strict-barrier I think).

 Thank you? Hin-Tak!

 I did not yet try to reproduce the segfault,
 and I am not
 the expert here.? Just some
 remarks and a follow up question:

 Typically, the above R_CHAR() is equivalent to
 the? CHAR()
 macro which is used in many
 places.? I? _think_ that the bug is
 that
 this is called with '0' instead of a proper SEXP?
 in your
 case and the bug fix may be more
 appropriate "up stream", i.e.,
 at
 the place where that call happens? rather than inside
 R_CHAR.

 Any
 chance you saw or can get more info about the location of
 the crash, such as a stack trace ? 

 The idiom?
 ???if(TYPEOF(x)? ==? <some>SXP)
 is used in many places in the R sources, and I
 think we never
 prepend that with a? 'x
 && '? like you propose above.

I already fixed the particular upstream issue in main.c. I agree that
at least with the barrier check mmuch more null checking would be nice
to have; I put it on my TODO list but won't get there for a while --
if someone else has time earlier go ahead.

Best,

luke

On Mon, 20 Feb 2017, Hin-Tak Leung wrote:
> On 2nd thought, I think a better fix to the segfault is something like
this:
>
> --- a/src/main/memory.c
> +++ b/src/main/memory.c
> @@ -3444,6 +3444,8 @@ R_xlen_t (XTRUELENGTH)(SEXP x) { return
XTRUELENGTH(CHK2(x)); }
> int  (IS_LONG_VEC)(SEXP x) { return IS_LONG_VEC(CHK2(x)); }
> 
> const char *(R_CHAR)(SEXP x) {
> +    if(!x)
> +       error("de-referncing null. Check the validity of your
data.");
>     if(TYPEOF(x) != CHARSXP)
>        error("%s() can only be applied to a '%s', not a
'%s'",
>              "CHAR", "CHARSXP", type2char(TYPEOF(x)));
>
>
> The segfault happens in the middle of tests/no-segfault.R . I have since
built R 3.2.x and 3.3.x with --enable-strict-barrier so this is probably new to
R 3.4. I think
> tests/no-segfault.R is supposed to try to trigger a segfault with invalid
data, and it is supposed to be caught. That it isn't caught with some
combinations of configure is obviously broken; OTOH, testing for segfault with
invalid data is also intentional; so I think a better solution is to be verbose
about it, instead of what I suggested earlier, silently letting the invalid data
through and let upstream cope.
>
> I had a stack trace - but it wasn't obvious where-else a check should
be made. The segfault happens is in the eval loop, which is fairly general by
itself.
>
> In any case, that was the whole point of me having  
--enable-memory-profiling --enable-strict-barrier
--with-valgrind-instrumentation=2 : I work(ed) with people who like to write
buggy code. Invalid input data and invalid stuff somewhere in the middle is
expected, from these people...
>
> To be honest, I think a few more null checks and a few more tests in
tests/no-segfault.R could be added.
>
> --------------------------------------------
> On Mon, 2/20/17, Martin Maechler <maechler at stat.math.ethz.ch>
wrote:
>
> Subject: Re: [Rd] another fix for R crashes under enable-strict-barrier,
lto, trunk at 72156
> To: "Hin-Tak Leung" <htl10 at users.sourceforge.net>
> Cc: r-devel at r-project.org, "bonsai list"
<outmodedbonsai-announce at lists.sourceforge.net>
> Date: Monday, February 20, 2017, 9:56 AM
>
> >>>>>
> Hin-Tak Leung <htl10 at users.sourceforge.net>
> >>>>>? ???on Sat, 11
> Feb 2017 19:30:26 +0000 writes:
>
> ? ? > I haven' t touched R for some 18
> months, and so I have no
> ? ? > idea if
> this is a recent problems or not; but it certainly
> ? ? > did not segfault two years ago.?
> Since it has been
> ? ? > crashing
> (segfault) under 'make check-all' for over a
> ? ? > month, I reckon I'll have to
> look at it myself, to have it
> ? ? >
> fixed.
>
> ? ? > I have
> been having the ' --enable-memory-profiling
> --enable-strict-barrier
> --with-valgrind-instrumentation=2" options
>
> ? ? > for perhaps a
> decade - because I work(ed) with people who
> ? ? > like to write buggy code :-(. And I
> also run 'make
> ? ? > check-all'
> from time to time until two years ago.
>
> ? ? > ./configure
> --enable-memory-profiling --enable-strict-barrier
> --enable-byte-compiled-packages
> --with-valgrind-instrumentation=2 --enable-lto
>
> ? ? > current R dev
> crashes in make check-all . The fix is this:
>
>
> ? ? >
> --- a/src/main/memory.c
> ? ? > +++
> b/src/main/memory.c
> ? ? > @@ -3444,7
> +3444,7 @@ R_xlen_t (XTRUELENGTH)(SEXP x) { return
> XTRUELENGTH(CHK2(x)); }
> ? ? >? int?
> (IS_LONG_VEC)(SEXP x) { return IS_LONG_VEC(CHK2(x)); }
>
> ? ? >? const char
> *(R_CHAR)(SEXP x) {
> ? ? > -? ?
> if(TYPEOF(x) != CHARSXP)
> ? ? > +? ?
> if(x && (TYPEOF(x) != CHARSXP))
> ?
> ? >? ? ? ???error("%s() can only be
> applied to a '%s', not a '%s'",
> ? ? >? ? ? ? ? ?
> ???"CHAR", "CHARSXP",
> type2char(TYPEOF(x)));
> ? ? >? ? ?
> return (const char *)CHAR(x);
>
>
> ? ? > It is a fairly
> obvious fix to a bug since
>
> ? ? > include/Rinternals.h:#define
> TYPEOF(x) ((x)->sxpinfo.type)
>
> ? ? > and it was trying to de-reference
> "0->sxpinfo.type" (under
> ? ?
> > --enable-strict-barrier I think).
>
> Thank you? Hin-Tak!
>
> I did not yet try to reproduce the segfault,
> and I am not
> the expert here.? Just some
> remarks and a follow up question:
>
> Typically, the above R_CHAR() is equivalent to
> the? CHAR()
> macro which is used in many
> places.? I? _think_ that the bug is
> that
> this is called with '0' instead of a proper SEXP?
> in your
> case and the bug fix may be more
> appropriate "up stream", i.e.,
> at
> the place where that call happens? rather than inside
> R_CHAR.
>
> Any
> chance you saw or can get more info about the location of
> the crash, such as a stack trace ? 
>
> The idiom?
> ???if(TYPEOF(x)? ==? <some>SXP)
> is used in many places in the R sources, and I
> think we never
> prepend that with a? 'x
> && '? like you propose above.
>
> ______________________________________________
> R-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
-- 
Luke Tierney
Ralph E. Wareham Professor of Mathematical Sciences
University of Iowa                  Phone:             319-335-3386
Department of Statistics and        Fax:               319-335-3017
    Actuarial Science
241 Schaeffer Hall                  email:   luke-tierney at uiowa.edu
Iowa City, IA 52242                 WWW:  http://www.stat.uiowa.edu