Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fcase() may trigger segfaults in certain cases #4378

Closed
fredguinog opened this issue Apr 13, 2020 · 14 comments
Closed

fcase() may trigger segfaults in certain cases #4378

fredguinog opened this issue Apr 13, 2020 · 14 comments

Comments

@fredguinog
Copy link

fredguinog commented Apr 13, 2020

I'm using data.table 1.12.9, the most recent development version

Minimal reproducible example

qtty_row_table <- 1000000L

a <- data.table::data.table(
  V1 = rnorm(qtty_row_table),
  V2 = rnorm(qtty_row_table),
  V3 = rnorm(qtty_row_table)
)

a[, v0 := data.table::fcase(
  V1 > 0 & V2 <= 1 & V3 > 1, V2 * 100L,
  V1 > 1 & V2 <= 0 & V3 > 0, V3 * 100L,
  V1 > -1 & V2 <= 2 & V3 > 1, V1 * 100L,
  V1 > 1 & V2 <= 0 & V3 > 2, 300,
  V1 > 0 & V2 <= 1 & V3 > 1, 100,
  V1 > -1 & V2 <= 0 & V3 > -1, V1 * 100L,
  default = 0
)]

Output of sessionInfo()

R version 3.5.2 (2018-12-20)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows 10 x64 (build 18363)

Matrix products: default

locale:
[1] LC_COLLATE=English_United States.1252  LC_CTYPE=English_United States.1252    LC_MONETARY=English_United States.1252
[4] LC_NUMERIC=C                           LC_TIME=English_United States.1252    

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

loaded via a namespace (and not attached):
[1] compiler_3.5.2 tools_3.5.2   
@MichaelChirico
Copy link
Member

Working fine for me on Mac / 3.6. Could you share more about the crash? Is it a segfault? Out of memory?

R version 3.6.0 (2019-04-26)
Platform: x86_64-apple-darwin15.6.0 (64-bit)
Running under: macOS Mojave 10.14.6

Matrix products: default
BLAS:   /Library/Frameworks/R.framework/Versions/3.6/Resources/lib/libRblas.0.dylib
LAPACK: /Library/Frameworks/R.framework/Versions/3.6/Resources/lib/libRlapack.dylib

locale:
[1] C/UTF-8/C/C/C/C

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

loaded via a namespace (and not attached):
[1] compiler_3.6.0    data.table_1.12.9

@fredguinog
Copy link
Author

The rsession just crashes without showing any message. Using the event viewer from windows 10, it generates this log:

Log Name:      Application
Source:        Application Error
Date:          4/13/2020 11:00:49 PM
Event ID:      1000
Task Category: (100)
Level:         Error
Keywords:      Classic
User:          N/A
Computer:      LAPTOP-RLEB1RA2
Description:
Faulting application name: Rterm.exe, version: 3.52.10334.0, time stamp: 0x5c1b62e0
Faulting module name: R.dll, version: 3.52.10334.0, time stamp: 0x5c1b62d0
Exception code: 0xc0000005
Fault offset: 0x0000000000051373
Faulting process id: 0x4f84
Faulting application start time: 0x01d6120081d45d88
Faulting application path: C:\PROGRA~1\R\R-35~1.2\bin\x64\Rterm.exe
Faulting module path: C:\PROGRA~1\R\R-35~1.2\bin\x64\R.dll
Report Id: fce4b052-d417-4390-ac37-5e28bfc2d145
Faulting package full name: 
Faulting package-relative application ID: 

@shrektan
Copy link
Member

shrektan commented Apr 14, 2020

I can reproduce this on Windows R3.5.3.


UPDATE: Super weird. Can't reproduce on Windows R3.6.2 so I believe it only occurs on Windows with R versions older than or equal to R3.5.3.

On R3.6.2 it crashes as well but needs to run it without RStudio (in RStudio you have to run it repeatedly for 2-3 times before it crashes).

@shrektan shrektan added the bug label Apr 14, 2020
@MichaelChirico
Copy link
Member

Also works for me on rocker/r-ver:3.5.3 so it must be a windows thing

@jangorecki
Copy link
Member

Unless there will be a report of this issue on windows R > 3.5.3 I think it is safe close.

@ChristK
Copy link

ChristK commented Apr 14, 2020

I can reproduce in R 3.6.2 on W10. It crashes R every time (not using Rstudio). In Rstudio sometimes I have to run the code 2 or 3 times before it crashes. But it always crashes eventually.

@MichaelChirico
Copy link
Member

MichaelChirico commented Apr 14, 2020

this build of fcase-windows branch also failed on appveyor

https://ci.appveyor.com/project/Rdatatable/data-table/builds/32166226

@shrektan
Copy link
Member

@ChristK You are right. It crashes in R3.6.2 on Windows. I didn't see it because I run it in RStudio. In the terminal, the R session just quits (assumes crashed).

@sindribaldur
Copy link

Crashed on first run in Rstudio, R 4.0.0, W10

@shrektan
Copy link
Member

According to my test, it should have been fixed by #4401. However, additional verifications are welcome (install the PR via remotes::install('rdatatable/data.table#4401') )

@fredguinog
Copy link
Author

It worked like a charm
Thank you very much

@shrektan
Copy link
Member

Thanks for the response but we need to reopen it as it should be closed after the PR gets merged to the master branch.

@shrektan shrektan reopened this May 12, 2020
@2005m
Copy link
Contributor

2005m commented May 12, 2020

@fredguinog , you can use kit::nif (look at the news file for more info) until fcase gets fixed. You might be waiting for some times. You might also be interested by kit::vswitch which is an order of magnitude faster.

@shrektan shrektan changed the title fcase crashes my rsession fcase() may trigger segfaults in certain cases Oct 31, 2020
@shrektan shrektan added this to the 1.13.3 milestone Oct 31, 2020
@mattdowle mattdowle added the dev label Nov 3, 2020
@mattdowle
Copy link
Member

mattdowle commented Nov 3, 2020

Thanks for the example and details. Turns out it was fixed by #4629 before fcase was released in 1.13.0. Thanks to static code analysis by the rchk tool in release procedures. Point 2 mentions rework to fifelse.c and the first value not being protected.
Sorry I didn't see your good example, or @shrektan's PR #4401 at the time. Double-checks to be sure in #4401 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants