Ivan Krylov
2025-Feb-01 09:39 UTC
[Rd] [SPAM Warning!] Suggestion to emphasize Rboolean is unrelated to LGLSXP in R-exts
On Thu, 30 Jan 2025 13:07:31 -0800 Michael Chirico <michaelchirico4 at gmail.com> wrote:> There are at least dozens of other cases on CRAN [2],[3].Some of these involve casting an int to Rboolean. Best case, the int is compared against NA_LOGICAL beforehand, avoiding any mistake (there's at least one like that). Worst case, NA_LOGICAL is not considered before the cast, so NA will now be interpreted as TRUE. This is hard to check without actually reading the code. Some packages compare an Rboolean expression against NA_LOGICAL [1]. This implies having stored an int in an Rboolean value as in the previous paragraph. I think that it wasn't disallowed according to the C standard to store NA_LOGICAL in an enumeration type wide enough to fit it (and it evidently worked in practice). With typedef bool Rboolean, storing NA_LOGICAL in an Rboolean converts it to 'true', so the comparison will definitely fail: DPQ src/pnchisq-it.c:530,532 Rmpfr src/convert.c:535 checkmate src/helper.c:102 chron src/unpaste.c:21 collapse src/data.table_rbindlist.c:208,258,383,384,408,431 data.table (many; fixed in Git) ff src/ordermerge.c:5074 (one declaration, many comparisons) networkDynamic src/Rinit.c:209 src/is.active.c:75,76,96-98 slam src/util.c:258 this.path src/get_file_from_closure.h:13,43 src/thispath.c:14,17,19,39 src/ext.c:25 src/setsyspath.c:8 src/get_file_from_closure.h:13,43 Four packages cast int* pointers returned by LOGICAL() to Rboolean* or use sizeof(Rboolean) to calculate buffer sizes in calls to memcpy() with LOGICAL() buffers [2]. With typedef bool Rboolean, this is a serious mistake, because the memory layout of the types is no longer compatible: bit64 src/integer64.c:576,603,914,929,942,955,968,981,994 collapse src/data.table_rbindlist.c:19,67,105 data.table (many; fixed in Git) kit src/utils.c:390 I don't know Coccinelle that well and there may be additional cases I failed to consider. At which point is it appropriate to start notifying maintainers of the bugs not caught by their test suites? -- Best regards, Ivan [1] Coccinelle script: @@ typedef Rboolean; Rboolean E; @@ * E == NA_LOGICAL [2] Coccinelle scripts: @@ typedef Rboolean; int* E; @@ * (Rboolean*)E This one will offer a diff to fix the bug: @@ int *E1; int *E2; typedef Rboolean; @@ ( memcpy | memmove ) (E1, E2, <+... -sizeof(Rboolean) +sizeof(int) ...+> )
Ivan Krylov
2025-Feb-02 15:50 UTC
[Rd] Suggestion to emphasize Rboolean is unrelated to LGLSXP in R-exts
The good news is that without a C23-enabled compiler, the problem will only happen to source files that #include <stdbool.h>. The bad news is that such a source file will technically disagree with the rest of R about the type of Rboolean, including the prototypes of the API functions that accept Rboolean: #include <stdbool.h> #include <Rinternals.h> typedef void (*pordervector1)(int *, int, SEXP, Rboolean, Rboolean); // ... pordervector1 f = R_orderVector1; f(pindx, length(indx), arg, nalast, decreasing); foo.c:27:17: runtime error: call to function R_orderVector1 through pointer to incorrect function type 'void (*)(int *, int, struct SEXPREC *, bool, bool)' /tmp/R-devel/src/main/sort.c:1135: note: R_orderVector1 defined here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior foo.c:27:17 With sanitizers disabled, this doesn't seem to cause any real problems thanks to the calling convention, where both 'enum's and 'bool's are passed and returned in a register. -- Best regards, Ivan
Tomas Kalibera
2025-Feb-04 12:45 UTC
[Rd] Suggestion to emphasize Rboolean is unrelated to LGLSXP in R-exts
On 2/1/25 10:39, Ivan Krylov via R-devel wrote:> On Thu, 30 Jan 2025 13:07:31 -0800 > Michael Chirico <michaelchirico4 at gmail.com> wrote: > >> There are at least dozens of other cases on CRAN [2],[3]. > Some of these involve casting an int to Rboolean. Best case, the int is > compared against NA_LOGICAL beforehand, avoiding any mistake (there's > at least one like that). Worst case, NA_LOGICAL is not considered before > the cast, so NA will now be interpreted as TRUE. This is hard to check > without actually reading the code. > > Some packages compare an Rboolean expression against NA_LOGICAL [1]. > This implies having stored an int in an Rboolean value as in the > previous paragraph. I think that it wasn't disallowed according to the > C standard to store NA_LOGICAL in an enumeration type wide enough to > fit it (and it evidently worked in practice). With typedef bool > Rboolean, storing NA_LOGICAL in an Rboolean converts it to 'true', so > the comparison will definitely fail: > > DPQ src/pnchisq-it.c:530,532 > Rmpfr src/convert.c:535 > checkmate src/helper.c:102 > chron src/unpaste.c:21 > collapse src/data.table_rbindlist.c:208,258,383,384,408,431 > data.table (many; fixed in Git) > ff src/ordermerge.c:5074 (one declaration, many comparisons) > networkDynamic src/Rinit.c:209 src/is.active.c:75,76,96-98 > slam src/util.c:258 > this.path src/get_file_from_closure.h:13,43 src/thispath.c:14,17,19,39 > src/ext.c:25 src/setsyspath.c:8 src/get_file_from_closure.h:13,43 > > Four packages cast int* pointers returned by LOGICAL() to Rboolean* or > use sizeof(Rboolean) to calculate buffer sizes in calls to memcpy() > with LOGICAL() buffers [2]. With typedef bool Rboolean, this is a > serious mistake, because the memory layout of the types is no longer > compatible: > > bit64 src/integer64.c:576,603,914,929,942,955,968,981,994 > collapse src/data.table_rbindlist.c:19,67,105 > data.table (many; fixed in Git) > kit src/utils.c:390 > > I don't know Coccinelle that well and there may be additional cases I > failed to consider. At which point is it appropriate to start notifying > maintainers of the bugs not caught by their test suites?Yes, thanks, if you have the energy, I think it would be great if you could contact the maintainers directly if you find obvious problems in their packages, as much as in any open-source project. Casting int* from LOGICAL() to Rboolean* falls into that category. Any bug finding tool would only find a subset of the bugs, that's ok - helping to fix only some packages (and not all) is still a big help. Tomas
Maybe Matching Threads
- [SPAM Warning!] Suggestion to emphasize Rboolean is unrelated to LGLSXP in R-exts
- Suggestion to emphasize Rboolean is unrelated to LGLSXP in R-exts
- Problem with Rboolean in c++ code
- orderVector1 (sort.c): Tiny improvement concerning nalast
- boolean and logical types -draft