From tom@@@k@||ber@ @end|ng |rom gm@||@com  Tue Feb  4 13:45:49 2025
From: tom@@@k@||ber@ @end|ng |rom gm@||@com (Tomas Kalibera)
Date: Tue, 4 Feb 2025 13:45:49 +0100
Subject: [Rd] Suggestion to emphasize Rboolean is unrelated to LGLSXP in
 R-exts
In-Reply-To: <20250201123904.6fc253be@Tarkus>
References: <CAPRVBcwA_j9=nPsjo272knqzK8Bdaoi6bgsGO+5YrKLhttHvDw@mail.gmail.com>
 <20250201123904.6fc253be@Tarkus>
Message-ID: <b9e0a7c2-ea92-43d8-ac7d-db816bd31123@gmail.com>


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



[Rd] Suggestion to emphasize Rboolean is unrelated to LGLSXP in R-exts
Tomas Kalibera