Re: [Rd] another fix for R crashes under enable-strict-barrier, lto, trunk@72156

[luke-tierney]

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 <maech...@stat.math.ethz.ch> wrote:

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

>>>>>
Hin-Tak Leung <ht...@users.sourceforge.net>
>>>>>     on Sat, 11
Feb 2017 19:30:26 + 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)  ==  SXP)
is used in many places in the R sources, and I
think we never
prepend th

Re: [Rd] another fix for R crashes under enable-strict-barrier, lto, trunk@72156

[Hin-Tak Leung]

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 <maech...@stat.math.ethz.ch> wrote:

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

 >>>>>
 Hin-Tak Leung <ht...@users.sourceforge.net>
 >>>>>     on Sat, 11
 Feb 2017 19:30:26 + 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)  ==  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@r-project.org mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel

Re: [Rd] another fix for R crashes under enable-strict-barrier, lto, trunk@72156

[Martin Maechler]

> Hin-Tak Leung 
> on Sat, 11 Feb 2017 19:30:26 + 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)  ==  SXP)
is used in many places in the R sources, and I think we never
prepend that with a  'x && '  like you propose above.




> So there.

> While I subscribe to R-devel, I switched off delivery, so
> please CC if a response is required.

__
R-devel@r-project.org mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel

[Rd] another fix for R crashes under enable-strict-barrier, lto, trunk@72156

[Hin-Tak Leung]

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

So there.

While I subscribe to R-devel, I switched off delivery, so please CC if a 
response is required.

__
R-devel@r-project.org mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel