Seg fault stats::runmed

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Seg fault stats::runmed

Hilmar Berger-4
Dear all,

I just found this issue:

dd1 = c(rep(NaN,82), rep(-1, 144), rep(1, 74))
xx = runmed(dd1, 21)

-> R crashes reproducibly in R 3.4.3, R3.4.4 (Ubuntu 14.04/Ubuntu 16.04)

With GDB:
Program received signal SIGSEGV, Segmentation fault.
swap (l=53, r=86, window=window@entry=0xc59308,
outlist=outlist@entry=0x12ea2e8, nrlist=nrlist@entry=0x114fdd8,
print_level=print_level@entry=0) at Trunmed.c:64
64        outlist[nr/* = nrlist[l] */] = l;

Valgrind also reports access to unallocated memory and/or writing past
the end of the heap.

The crash does not happen if the order is changed:

dd2 = c(rep(-1, 144), rep(1, 74), rep(NaN,82))
xx = runmed(dd2,21)

Error in if (a < b) { : missing value where TRUE/FALSE needed

Best regards,
Hilmar

--
Dr. Hilmar Berger, MD
Max Planck Institute for Infection Biology
Charitéplatz 1
D-10117 Berlin
GERMANY

Phone:  + 49 30 28460 430
Fax:    + 49 30 28460 401
 
E-Mail: [hidden email]
Web   : www.mpiib-berlin.mpg.de

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Seg fault stats::runmed

Martin Maechler
>>>>> Hilmar Berger
>>>>>     on Fri, 5 Oct 2018 10:17:49 +0200 writes:

    > Dear all, I just found this issue:

    > I just found this issue:

    > dd1 = c(rep(NaN,82), rep(-1, 144), rep(1, 74))
    > xx = runmed(dd1, 21)

    >> R crashes reproducibly in R 3.4.3, R3.4.4 (Ubuntu 14.04/Ubuntu 16.04)

and also in the latest development version (we call "R-devel").

THank you very much, Hilmar!

I will have a look, to ensure missing values (incl NaN) are
handled propertly.

Martin

--
Martin Maechler
ETH Zurich and R Core Team

    > With GDB:
    > Program received signal SIGSEGV, Segmentation fault.
    > swap (l=53, r=86, window=window@entry=0xc59308,
    > outlist=outlist@entry=0x12ea2e8, nrlist=nrlist@entry=0x114fdd8,
    > print_level=print_level@entry=0) at Trunmed.c:64
    > 64        outlist[nr/* = nrlist[l] */] = l;

    > Valgrind also reports access to unallocated memory and/or writing past
    > the end of the heap.

    > The crash does not happen if the order is changed:

    > dd2 = c(rep(-1, 144), rep(1, 74), rep(NaN,82))
    > xx = runmed(dd2,21)

    > Error in if (a < b) { : missing value where TRUE/FALSE needed

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Seg fault stats::runmed

Martin Maechler
>>>>> Martin Maechler
>>>>>     on Fri, 5 Oct 2018 12:16:37 +0200 writes:

>>>>> Hilmar Berger
>>>>>     on Fri, 5 Oct 2018 10:17:49 +0200 writes:

    >> Dear all, I just found this issue:

    >> I just found this issue:

    >> dd1 = c(rep(NaN,82), rep(-1, 144), rep(1, 74))
    >> xx = runmed(dd1, 21)

    >>> R crashes reproducibly in R 3.4.3, R3.4.4 (Ubuntu 14.04/Ubuntu 16.04)

    > and also in the latest development version (we call "R-devel").

    > THank you very much, Hilmar!

    > I will have a look, to ensure missing values (incl NaN) are
    > handled propertly.

That "look" had several parts to it, with long breaks in between;
finally Hilmar kindly asked me privately, and I committed
changes to R-devel (and R 3.6.0 patched), with NEWS entry

    * runmed(x, *) when x contains missing values now works for
      algorithm="Stuetzle", also based on smoothEnds(y) working with
      NA's, and no longer segfaults for the "Turlach" algorithm;
      reported by Hilmar Berger.

but the changes were not at all sufficient to correctly deal
with NA / NaN's in runmed() --- and hence the above NEWS entry
was "fake news" as some may call it.

So, the last 2 weeks or so, I've spent several working days and
some extra hours trying to get this going. {several tries proved
to be insufficient, logically wrong, too optimistic, ...}

In the end, I've implemented a simplistic "imputation"-scheme
for the default case, *and* also added a new optional argument
'na.action' to runmed(),
and committed this half an hour ago :

------------------------------------------------------------------------
r76744 | maechler | 2019-06-27 15:51:04 +0200 (Thu, 27. Jun 2019)

   M doc/NEWS.Rd
   M src/library/stats/R/runmed.R
   M src/library/stats/man/runmed.Rd
   M src/library/stats/man/smoothEnds.Rd
   M src/library/stats/src/Srunmed.c
   M src/library/stats/src/Trunmed.c
   M src/library/stats/src/init.c
   M src/library/stats/src/statsR.h
   M tests/Examples/stats-Ex.Rout.save
   M tests/reg-tests-1d.R

runmed(<NAs>, "Turlach") did still seg.fault. Now, NEWS entry (76682) should be true; new argument `na.action = ".."` determines *how* NA/NaN are treated
------------------------------------------------------------------------

As this does prevent seg.faults, and if it is acceptable to the
release process, this may also make it into the upcoming R 3.6.1.

--
Martin Maechler
ETH Zurich and R Core Team

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel