`growVector`, `copyAsPlain` may formally cause undefined behaviour with 0-length vectors

[aitap]

Found while working on a custom Debian testing-based container (using C compiler: ‘Debian clang version 19.1.7 (1+b1)’) with Clang sanitizers enabled for #6746:

When trying to access the contents of a zero-length vector using INTEGER(...) or REAL(...) or other accessor, R may return an invalid pointer (0x1). The C standard says that giving an invalid pointer to memcpy() is undefined behaviour, even though in practice nothing breaks (memcpy sees n=0 and doesn't dereference it).

If nothing breaks, what's the risk? One of the CRAN special checks (clang-UBSAN or 0len) might pick this up too. Very far-fetched, a compiler might optimize away a chunk of code deemed to cause undefined behaviour.

Running test id 173.1

dogroups.c:541:39: runtime error: load of misaligned address 0x000000000001 for type 'int *', which requires 4 byte alignment
0x000000000001: note: pointer points here
<memory cannot be printed>
    #0 0x7f2efbaec116 in growVector /work/data.table.Rcheck/00_pkg_src/data.table/src/dogroups.c
    #1 0x7f2efbae90d5 in dogroups /work/data.table.Rcheck/00_pkg_src/data.table/src/dogroups.c:409:66

(gdb) frame 4
#4  0x00007fc6d4aec117 in growVector (x=0x52500661b038, newlen=newlen@entry=3) at dogroups.c:543
543       case CPLXSXP: memcpy(COMPLEX(newx), COMPLEX(x), len*SIZEOF(x)); break;
(gdb) p Rf_xlength(x)
$4 = 0
(gdb) p Rf_xlength(newx)
$5 = 3
(gdb) call Rf_PrintValue(R_GlobalContext->call)
`[.data.table`(DT, , B[B > 3], by = A)

Running test id 893.5

utils.c:233:12: runtime error: store to misaligned address 0x000000000001 for type 'int *', which requires 4 byte alignment
0x000000000001: note: pointer points here
<memory cannot be printed>
    #0 0x7f2efbbfd5f9 in copyAsPlain /work/data.table.Rcheck/00_pkg_src/data.table/src/utils.c:233:5
    #1 0x7f2efbbefbf9 in subsetDT /work/data.table.Rcheck/00_pkg_src/data.table/src/subset.c:317:30

(gdb) frame 4
#4  0x00007fc6d4bfd5fa in copyAsPlain (x=x@entry=0x525003c44b68) at utils.c:233
233         memcpy(INTEGER(ans), INTEGER(x), n*sizeof(int));             // covered by 10:1 after test 178
(gdb) p Rf_xlength(x)
$8 = 0
(gdb) call Rf_PrintValue(R_GlobalContext->call)
`[.data.table`(head(DT, nr), , seq_len(if (nc == 0) ncol(DT) else nc),
    with = FALSE)

Running test id 2150.21

dogroups.c:540:39: runtime error: load of misaligned address 0x000000000001 for type 'int *', which requires 4 byte alignment
0x000000000001: note: pointer points here
<memory cannot be printed>
    #0 0x7f2efbaec116 in growVector /work/data.table.Rcheck/00_pkg_src/data.table/src/dogroups.c
    #1 0x7f2efbb46d4e in allocateDT /work/data.table.Rcheck/00_pkg_src/data.table/src/freadR.c:501:36
    #2 0x7f2efbb2f967 in freadMain /work/data.table.Rcheck/00_pkg_src/data.table/src/fread.c:2666:7
    #3 0x7f2efbb42306 in freadR /work/data.table.Rcheck/00_pkg_src/data.table/src/freadR.c:222:3

(gdb) frame 4
#4  0x00007fc6d4aec117 in growVector (x=x@entry=0x525004e417e8, newlen=newlen@entry=1024)
    at dogroups.c:543
543       case CPLXSXP: memcpy(COMPLEX(newx), COMPLEX(x), len*SIZEOF(x)); break;
(gdb) p Rf_xlength(x)
$11 = 0
(gdb) call Rf_PrintValue(R_GlobalContext->call)
fread("c1\n2018-01-31 03:16:57")

(Yes, that case CPLSXP: looks a bit strange. clang must have merged the branches into one with different length arguments to memcpy().)

[MichaelChirico]

Funnily enough I managed to come across this same bug in a "different" setting, but I still can't reproduce it (even under --enable-strict-barrier) locally.

Could you share more details of the container you were using?

Does {vctrs} also segfault there? (I think it does: r-lib/vctrs#1968)

[aitap]

vctrs must be encountering some other problem. Could it be due to ABI changes? Some package needing to be reinstalled?

Here we had r-devel-clang-san complaining that calling memcpy(0x1, 0x1, 0) is formally undefined behaviour even though no modern memcpy() dereferences the pointers when n == 0. (Moreover, starting with C23, memcpy() with n == 0 will be required to be a no-op, no matter whether the pointers are valid or notmemcpy(0,0,0) will be a no-op, which is not relevant here.) It wouldn't crash without the sanitizer.

If this was a zero-length access, the crash would be due to access to 0x1 or some other low-value pointer, not 0x7f405afb3a6a (which looks like mmap area, so probably a large heap allocation).

Still, the fix shouldn't hurt and may prevent an UBSan warning during CRAN checks.

[MichaelChirico]

The only thing we did is update R 4.4.1 --> R 4.5.0. It seems that is turning on stricter checks in R:

https://github.com/r-devel/r-svn/blob/d62355fdacfd9a9a41639134ee9875e0c2502ca7/src/include/Rinlinedfuns.h#L84
https://github.com/r-devel/r-svn/blob/d62355fdacfd9a9a41639134ee9875e0c2502ca7/src/main/memory.c#L31
https://github.com/r-devel/r-svn/blob/d62355fdacfd9a9a41639134ee9875e0c2502ca7/src/main/memory.c#L4146

Possibly the non-0x1 address is from (void*)1, though I admittedly still don't fully understand the issue, possibly we just have stricter implementations on by default.

[aitap]

I couldn't get it to crash. I also tried installing all the vctrs dependencies on R 4.4.1, then running R 4.5.0 --with-strict-barrier without rebuilding them. (The latter only got me two test failures of the form HASHTAB: argument of type SYMSXP is not an environment or NULL.) Any other information that could help reproduce the crash? My compiler is probably too old. A gdb backtrace could also be informative.

While the C standard allows making a pointer out of a number to perform very wild transformations, on our today's computers with MMUs and flat address spaces, (void*)1 stores a 1 in the register (or memory).

[aitap]

Are you compiling with -fsanitize-trap? This is what -fsanitize-trap looks like: r-hub/containers#84

CRAN special tests may be disabling the memcpy test, or running their tests in C23 mode, where memcpy(..., ..., 0) should no longer count as load/store (but I haven't verified either option)or using an older version of the compiler which doesn't have this check enabled.