add error to prevent segfault when setDT and attr<- (#6419)

* add error to prevent segfault * refine error msg Co-authored-by: Michael Chirico <[email protected]> * add tests * add issue to tests * add NEWS * fix wording Co-authored-by: Michael Chirico <[email protected]> --------- Co-authored-by: Michael Chirico <[email protected]> Co-authored-by: Michael Chirico <[email protected]>
Rdatatable · Sep 12, 2024 · 4d42cdc · 4d42cdc
1 parent 038ce2b
commit 4d42cdc
Show file tree

Hide file tree

Showing 3 changed files with 10 additions and 0 deletions.
diff --git a/NEWS.md b/NEWS.md
@@ -65,6 +65,8 @@ rowwiseDT(
 
 9. `rbindlist` and `rbind` binding `bit64::integer64` columns with `character`/`complex`/`list` columns now works, [#5504](https://github.com/Rdatatable/data.table/issues/5504). Thanks to @MichaelChirico for the request and @ben-schwen for the PR.
 
+10. Fixed possible segfault in `setDT(df); attr(df, key) <- value; set(df, ...)`, i.e. adding columns to an object with `set()` that was converted to data.table with `setDT()` and later had attributes add with `attr<-`, [#6410](https://github.com/Rdatatable/data.table/issues/6410). Thanks to @hongyuanjia for the report and @ben-schwen for the PR. Note that `setattr()` should be preferred for adding attributes to a data.table.
+
 ## NOTES
 
 1. Tests run again when some Suggests packages are missing, [#6411](https://github.com/Rdatatable/data.table/issues/6411). Thanks @aadler for the note and @MichaelChirico for the fix.

diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw
@@ -19240,3 +19240,9 @@ test(2290.2, DT[, `:=`(a := 2, c := 3)], error="It looks like you re-used `:=` i
 test(2290.3, DT[, `:=`(a, c := 3)], error="It looks like you re-used `:=` in argument 2")
 # partially-named `:=`(...) --> different branch, same error
 test(2290.4, DT[, `:=`(a = 2, c := 3)], error="It looks like you re-used `:=` in argument 2")
+
+# segfault when selfref is not ok before set #6410
+df = data.frame(a=1:3)
+setDT(df)
+attr(df, "att") = 1
+test(2291.1, set(df, NULL, "new", "new"), error="attributes .* have been reassigned")
diff --git a/src/assign.c b/src/assign.c
@@ -529,6 +529,8 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values)
     else if (TRUELENGTH(names) != oldtncol)
       // Use (long long) to cast R_xlen_t to a fixed type to robustly avoid -Wformat compiler warnings, see #5768, PRId64 didn't work
       internal_error(__func__, "selfrefnames is ok but tl names [%lld] != tl [%d]", (long long)TRUELENGTH(names), oldtncol);  // # nocov
+    if (!selfrefok(dt, verbose)) // #6410 setDT(dt) and subsequent attr<- can lead to invalid selfref
+      error(_("It appears that at some earlier point, attributes of this data.table have been reassigned. Please use setattr(DT, name, value) rather than attr(DT, name) <- value. If that doesn't apply to you, please report your case to the data.table issue tracker."));
     SETLENGTH(dt, oldncol+LENGTH(newcolnames));
     SETLENGTH(names, oldncol+LENGTH(newcolnames));
     for (int i=0; i<LENGTH(newcolnames); ++i)