From 7ef302308f607e7d70de9884e7aafd88e10925ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dawid=20Budzy=C5=84ski?= Date: Tue, 8 Sep 2026 20:07:33 +0200 Subject: [PATCH 1/4] fix(share): make getData non-destructive for multi-reader sharing Reads no longer shm_unlink, so N processes (e.g. mirai_map daemons) can attach while the owner handle from shareData() stays alive. Lifetime is owned by clearData()/finalizer. Also strdup shm names (was dangling CHAR pointer), always ftruncate on re-share, and fix error-path leaks. No new dependencies. --- R/call.R | 11 +- man/shareData.Rd | 20 +++- src/share.c | 271 +++++++++++++++++++++++++++++++++-------------- tests/test_kit.R | 25 ++++- 4 files changed, 239 insertions(+), 88 deletions(-) diff --git a/R/call.R b/R/call.R index 779a6f7..c953991 100644 --- a/R/call.R +++ b/R/call.R @@ -65,22 +65,19 @@ shmName = function(map_name) sub("^/*", "/", map_name) shareData = function(data, map_name, verbose=FALSE) { conn = rawConnection(raw(0L), "w") + on.exit(close(conn), add = TRUE) serialize(data, conn) - seek(conn, 0L) map_name = shmName(map_name) - x = .Call( + .Call( "CcreateMappingObjectR", map_name, paste0(map_name,"_key"), rawConnectionValue(conn), verbose ) - close(conn) - x } getData = function(map_name, verbose=FALSE) { map_name = shmName(map_name) output = .Call("CgetMappingObjectR", map_name, paste0(map_name,"_key"), verbose) conn = rawConnection(output,"r") - obj = unserialize(conn) - close(conn) - obj + on.exit(close(conn), add = TRUE) + unserialize(conn) } diff --git a/man/shareData.Rd b/man/shareData.Rd index c2fbe8b..df26a19 100644 --- a/man/shareData.Rd +++ b/man/shareData.Rd @@ -5,6 +5,12 @@ \title{ Share Data between R Sessions} \description{ Experimental functions that enable the user to share a R object between 2 \R sessions. +The object is serialized once by \code{shareData} into POSIX shared memory +(or a Windows file mapping). It can then be read multiple times, from the +current session and/or from other processes running as the same user, via +\code{getData}. Reads are non-destructive: the segment lives until the owner +handle returned by \code{shareData} is cleared with \code{clearData} (or +garbage collected). Keep the owner object alive while readers are active. } \usage{ shareData(data, map_name, verbose=FALSE) @@ -19,7 +25,7 @@ clearData(x, verbose=FALSE) } \value{ \code{shareData} returns a external pointer. -\code{getData} returns an \R object stored in the memory location \code{map_name}. +\code{getData} returns an \R object stored in the memory location \code{map_name}. Reads do not consume the segment, so \code{getData} may be called repeatedly and from multiple processes. \code{clearData} returns \code{TRUE} or \code{FALSE} depending on whether the data have been cleared in memory. } \author{Morgan Jacob} @@ -27,9 +33,17 @@ clearData(x, verbose=FALSE) # In R session 1: share data in memory # > x = shareData(mtcars,"share1") # -# In R session 2: get data from session 1 +# In R session 2: get data from session 1 (repeatable, also in parallel) +# > getData("share1") # > getData("share1") # -# In R session 1: clear data in memory +# In R session 1: clear data in memory once all readers are done # > clearData(x) +# +# Sharing large immutable globals with background workers (owner stays alive): +# > x = shareData(list(graph = g, dist = D), "/mydata") +# > # workers only receive the short map name and attach via shared memory +# > # mirai::daemons(4) +# > # mirai::mirai_map(ids, function(i) fun(i, kit::getData("/mydata"))) +# > # mirai::daemons(0); clearData(x) } diff --git a/src/share.c b/src/share.c index 8a7d61c..0cb233c 100644 --- a/src/share.c +++ b/src/share.c @@ -17,6 +17,7 @@ */ #include "kit.h" +#include /* * Structure to hold Length and Address @@ -35,8 +36,8 @@ struct OBJECT { size_t STORAGE_SIZE; void *addr; void *length; - const char *STORAGE_ID; - const char *LENGTH_ID; + char *STORAGE_ID; + char *LENGTH_ID; #endif }; @@ -53,16 +54,26 @@ static void map_finalizer (SEXP ext) { } if (verbose_finalizer) Rprintf("* Clear external pointer...\n"); struct OBJECT *ptr = (struct OBJECT*) R_ExternalPtrAddr(ext); -#ifdef WIN32 - UnmapViewOfFile(ptr->lpMapAddress); - CloseHandle(ptr->hMapFile); - UnmapViewOfFile(ptr->lpMapLength); - CloseHandle(ptr->hMapLength); +#ifdef WIN32 + if (ptr->lpMapAddress != NULL) UnmapViewOfFile(ptr->lpMapAddress); + if (ptr->hMapFile != NULL && ptr->hMapFile != INVALID_HANDLE_VALUE) CloseHandle(ptr->hMapFile); + if (ptr->lpMapLength != NULL) UnmapViewOfFile(ptr->lpMapLength); + if (ptr->hMapLength != NULL && ptr->hMapLength != INVALID_HANDLE_VALUE) CloseHandle(ptr->hMapLength); #else - munmap(ptr->addr, ptr->STORAGE_SIZE); - shm_unlink(ptr->STORAGE_ID); - munmap(ptr->length, 256); - shm_unlink(ptr->LENGTH_ID); + if (ptr->addr != NULL && ptr->addr != MAP_FAILED && ptr->STORAGE_SIZE > 0) { + munmap(ptr->addr, ptr->STORAGE_SIZE); + } + if (ptr->STORAGE_ID != NULL) { + shm_unlink(ptr->STORAGE_ID); + free(ptr->STORAGE_ID); + } + if (ptr->length != NULL && ptr->length != MAP_FAILED) { + munmap(ptr->length, 256); + } + if (ptr->LENGTH_ID != NULL) { + shm_unlink(ptr->LENGTH_ID); + free(ptr->LENGTH_ID); + } #endif R_Free(ptr); R_ClearExternalPtr(ext); @@ -87,70 +98,140 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje if (verbose) Rprintf("* Data object size: %zu\n",len*sizeof(Rbyte)); if (verbose) Rprintf("* Start mapping object...OK\n"); struct OBJECT *foo = R_Calloc(1, struct OBJECT); +#ifdef WIN32 + foo->hMapFile = NULL; + foo->hMapLength = NULL; + foo->lpMapAddress = NULL; + foo->lpMapLength = NULL; +#else + foo->fd_addr = -1; + foo->fd_length = -1; + foo->addr = NULL; + foo->length = NULL; + foo->STORAGE_ID = NULL; + foo->LENGTH_ID = NULL; + foo->STORAGE_SIZE = BUF_SIZE; +#endif SEXP ext = PROTECT(R_MakeExternalPtr(foo, R_NilValue, R_NilValue)); - R_RegisterCFinalizerEx(ext, map_finalizer, TRUE); - if (verbose) Rprintf("* Register finalizer...OK\n"); -#ifdef WIN32 +#ifdef WIN32 LPSTR pMN = (LPSTR) CHAR(STRING_PTR_RO(MapObjectName)[0]); LPSTR pML = (LPSTR) CHAR(STRING_PTR_RO(MapLengthName)[0]); + if (BUF_SIZE == 0) { + R_ClearExternalPtr(ext); + R_Free(foo); + UNPROTECT(1); + error("* Data object is empty...ERROR"); + } foo->hMapFile = CreateFileMapping(INVALID_HANDLE_VALUE, NULL, PAGE_READWRITE, 0, BUF_SIZE, pMN); foo->hMapLength = CreateFileMapping(INVALID_HANDLE_VALUE, NULL, PAGE_READWRITE, 0, 256, pML); - if (foo->hMapFile == INVALID_HANDLE_VALUE || foo->hMapLength == INVALID_HANDLE_VALUE) { + if (foo->hMapFile == NULL || foo->hMapFile == INVALID_HANDLE_VALUE || + foo->hMapLength == NULL || foo->hMapLength == INVALID_HANDLE_VALUE) { + if (foo->hMapFile != NULL && foo->hMapFile != INVALID_HANDLE_VALUE) CloseHandle(foo->hMapFile); + if (foo->hMapLength != NULL && foo->hMapLength != INVALID_HANDLE_VALUE) CloseHandle(foo->hMapLength); + R_ClearExternalPtr(ext); + R_Free(foo); + UNPROTECT(1); + error("* Creating file mapping...ERROR"); + } + if (verbose) Rprintf("* Creating file maping...OK\n"); + foo->lpMapAddress = (LPCTSTR) MapViewOfFile (foo->hMapFile, FILE_MAP_ALL_ACCESS, 0, 0, BUF_SIZE); + foo->lpMapLength = (LPCTSTR) MapViewOfFile (foo->hMapLength, FILE_MAP_ALL_ACCESS, 0, 0, 256); + if (foo->lpMapAddress == NULL || foo->lpMapLength == NULL) { + if (foo->lpMapAddress != NULL) UnmapViewOfFile(foo->lpMapAddress); + if (foo->lpMapLength != NULL) UnmapViewOfFile(foo->lpMapLength); + CloseHandle(foo->hMapFile); + CloseHandle(foo->hMapLength); + R_ClearExternalPtr(ext); + R_Free(foo); + UNPROTECT(1); + error("* Map view file...ERROR"); + } + if (verbose) Rprintf("* Map view file...OK\n"); + CopyMemory((LPVOID)foo->lpMapAddress, RAW(DataObject), BUF_SIZE); + CopyMemory((LPVOID)foo->lpMapLength, &len, sizeof(size_t)); #else const char *pMN = CHAR(STRING_PTR_RO(MapObjectName)[0]); const char *pML = CHAR(STRING_PTR_RO(MapLengthName)[0]); - foo->STORAGE_ID = pMN; - foo->LENGTH_ID = pML; - foo->STORAGE_SIZE = BUF_SIZE; + foo->STORAGE_ID = strdup(pMN); + foo->LENGTH_ID = strdup(pML); + if (foo->STORAGE_ID == NULL || foo->LENGTH_ID == NULL) { + if (foo->STORAGE_ID != NULL) free(foo->STORAGE_ID); + if (foo->LENGTH_ID != NULL) free(foo->LENGTH_ID); + R_ClearExternalPtr(ext); + R_Free(foo); + UNPROTECT(1); + error("* Duplicating shared memory names...ERROR"); + } + if (BUF_SIZE == 0) { + free(foo->STORAGE_ID); + free(foo->LENGTH_ID); + R_ClearExternalPtr(ext); + R_Free(foo); + UNPROTECT(1); + error("* Data object is empty...ERROR"); + } foo->fd_addr = shm_open(foo->STORAGE_ID, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); foo->fd_length = shm_open(foo->LENGTH_ID, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); if (foo->fd_addr == -1 || foo->fd_length == -1) { Rprintf("shm_open error, errno(%d): %s\n", errno, strerror(errno)); -#endif + if (foo->fd_addr != -1) close(foo->fd_addr); + if (foo->fd_length != -1) close(foo->fd_length); + free(foo->STORAGE_ID); + free(foo->LENGTH_ID); + R_ClearExternalPtr(ext); + R_Free(foo); + UNPROTECT(1); error("* Creating file mapping...ERROR"); } if (verbose) Rprintf("* Creating file maping...OK\n"); -#ifdef WIN32 -#else - struct stat mapstat; - if (-1 != fstat(foo->fd_addr, &mapstat) && mapstat.st_size == 0) { - if(ftruncate(foo->fd_addr, BUF_SIZE) == -1) { - error("* Extend shared memory object (1)...ERROR"); - } - } - if (-1 != fstat(foo->fd_length, &mapstat) && mapstat.st_size == 0) { - if(ftruncate(foo->fd_length, 256) == -1) { - error("* Extend shared memory object (2)...ERROR"); - } + // Always size the objects: re-sharing the same name must reflect the new payload. + if (ftruncate(foo->fd_addr, BUF_SIZE) == -1 || ftruncate(foo->fd_length, 256) == -1) { + close(foo->fd_addr); + close(foo->fd_length); + free(foo->STORAGE_ID); + free(foo->LENGTH_ID); + R_ClearExternalPtr(ext); + R_Free(foo); + UNPROTECT(1); + error("* Extend shared memory object...ERROR"); } if (verbose) Rprintf("* Extend shared memory object...OK\n"); -#endif - -#ifdef WIN32 - foo->lpMapAddress = (LPCTSTR) MapViewOfFile (foo->hMapFile, FILE_MAP_ALL_ACCESS, 0, 0, BUF_SIZE); - foo->lpMapLength = (LPCTSTR) MapViewOfFile (foo->hMapLength, FILE_MAP_ALL_ACCESS, 0, 0, 256); - if (foo->lpMapAddress == NULL || foo->lpMapLength == NULL) { -#else - foo->addr = mmap(NULL, BUF_SIZE, PROT_WRITE, MAP_SHARED, foo->fd_addr, 0); - foo->length = mmap(NULL, 256, PROT_WRITE, MAP_SHARED, foo->fd_length, 0); - if (foo->addr == MAP_FAILED || foo->length == MAP_FAILED) { -#endif + foo->addr = mmap(NULL, BUF_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, foo->fd_addr, 0); + foo->length = mmap(NULL, 256, PROT_READ | PROT_WRITE, MAP_SHARED, foo->fd_length, 0); + if (foo->addr == MAP_FAILED || foo->length == MAP_FAILED) { + if (foo->addr != MAP_FAILED) munmap(foo->addr, BUF_SIZE); + if (foo->length != MAP_FAILED) munmap(foo->length, 256); + close(foo->fd_addr); + close(foo->fd_length); + // Do not unlink here: the name may be shared; owner cleanup happens via finalizer. + foo->addr = NULL; + foo->length = NULL; + free(foo->STORAGE_ID); + free(foo->LENGTH_ID); + R_ClearExternalPtr(ext); + R_Free(foo); + UNPROTECT(1); error("* Map view file...ERROR"); } if (verbose) Rprintf("* Map view file...OK\n"); -#ifndef WIN32 if (close(foo->fd_addr) == -1 || close(foo->fd_length) == -1) { + munmap(foo->addr, BUF_SIZE); + munmap(foo->length, 256); + free(foo->STORAGE_ID); + free(foo->LENGTH_ID); + R_ClearExternalPtr(ext); + R_Free(foo); + UNPROTECT(1); error("* Closing file descriptors...ERROR"); } -#endif -#ifdef WIN32 - CopyMemory((LPVOID)foo->lpMapAddress, RAW(DataObject), BUF_SIZE); - CopyMemory((LPVOID)foo->lpMapLength, &len, sizeof(size_t)); -#else + foo->fd_addr = -1; + foo->fd_length = -1; memcpy(foo->addr, RAW(DataObject), BUF_SIZE); memcpy(foo->length, &len, sizeof(size_t)); #endif if (verbose) Rprintf("* Copy memory...OK\n"); + R_RegisterCFinalizerEx(ext, map_finalizer, TRUE); + if (verbose) Rprintf("* Register finalizer...OK\n"); UNPROTECT(1); return ext; } @@ -172,13 +253,18 @@ SEXP getMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseArg) LPSTR pML = (LPSTR) CHAR(STRING_PTR_RO(MapLengthName)[0]); HANDLE hMapFile = OpenFileMapping(FILE_MAP_ALL_ACCESS, FALSE, pMN); HANDLE hMapLength = OpenFileMapping(FILE_MAP_ALL_ACCESS, FALSE, pML); - if (hMapFile == INVALID_HANDLE_VALUE || hMapLength == INVALID_HANDLE_VALUE) { + if (hMapFile == NULL || hMapFile == INVALID_HANDLE_VALUE || + hMapLength == NULL || hMapLength == INVALID_HANDLE_VALUE) { + if (hMapFile != NULL && hMapFile != INVALID_HANDLE_VALUE) CloseHandle(hMapFile); + if (hMapLength != NULL && hMapLength != INVALID_HANDLE_VALUE) CloseHandle(hMapLength); #else const char *pMN = CHAR(STRING_PTR_RO(MapObjectName)[0]); const char *pML = CHAR(STRING_PTR_RO(MapLengthName)[0]); int fd_addr = shm_open(pMN, O_RDONLY, S_IRUSR | S_IWUSR); int fd_length = shm_open(pML, O_RDONLY, S_IRUSR | S_IWUSR); if (fd_addr == -1 || fd_length == -1) { + if (fd_addr != -1) close(fd_addr); + if (fd_length != -1) close(fd_length); #endif error("* Creating file mapping...ERROR"); } @@ -186,32 +272,58 @@ SEXP getMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseArg) #ifdef WIN32 LPCTSTR lpMapLength = (LPCTSTR) MapViewOfFile (hMapLength, FILE_MAP_ALL_ACCESS, 0, 0, 256); if (lpMapLength == NULL) { + CloseHandle(hMapFile); CloseHandle(hMapLength); #else void *length = mmap(NULL, 256, PROT_READ, MAP_SHARED, fd_length, 0); if (length == MAP_FAILED) { - shm_unlink(pML); + close(fd_addr); + close(fd_length); #endif error("* Map view file (length)...ERROR"); } if (verbose) Rprintf("* Map view file (length)...OK\n"); #ifdef WIN32 size_t len = *(size_t*)lpMapLength; - LPCTSTR lpMapAddress = (LPCTSTR) MapViewOfFile (hMapFile, FILE_MAP_ALL_ACCESS, 0, 0, len*sizeof(Rbyte)); - if (lpMapAddress == NULL) { + LPCTSTR lpMapAddress = NULL; + if (len > 0) { + lpMapAddress = (LPCTSTR) MapViewOfFile (hMapFile, FILE_MAP_ALL_ACCESS, 0, 0, len*sizeof(Rbyte)); + } + if (len == 0 || lpMapAddress == NULL) { + UnmapViewOfFile(lpMapLength); CloseHandle(hMapFile); + CloseHandle(hMapLength); #else - size_t len = *(size_t*)length; - void *addr = mmap(NULL, len*sizeof(Rbyte), PROT_READ, MAP_SHARED, fd_addr, 0); - if (addr == MAP_FAILED) { - shm_unlink(pMN); + size_t len = *(size_t*)length; + // Detach the length mapping as soon as the size is known; data mapping stays. + if (munmap(length, 256) == -1) { + close(fd_addr); + close(fd_length); + error("* Closing mapping file (length)...ERROR"); + } + if (verbose) Rprintf("* Closing mapping file (length)...OK\n"); + if (close(fd_length) == -1) { + close(fd_addr); + error("* Closing file descriptor (length)...ERROR"); + } + if (verbose) Rprintf("* Closing mapping handle (length)...OK\n"); + void *addr = NULL; + if (len > 0) { + addr = mmap(NULL, len*sizeof(Rbyte), PROT_READ, MAP_SHARED, fd_addr, 0); + } + if (len == 0 || addr == MAP_FAILED) { + close(fd_addr); #endif error("* Map view file (address)...ERROR"); } if (verbose) Rprintf("* Map view file (address)...OK\n"); -#ifndef WIN32 - if (close(fd_addr) == -1 || close(fd_length) == -1) { - error("* Closing file descriptors...ERROR"); + // fds no longer needed once mappings are established. + // Keep fd_addr open until after mmap; fd_length already closed on POSIX. +#ifdef WIN32 +#else + if (close(fd_addr) == -1) { + munmap(addr, len*sizeof(Rbyte)); + error("* Closing file descriptor (address)...ERROR"); } #endif SEXP ans = PROTECT(allocVector(RAWSXP, len)); @@ -219,43 +331,48 @@ SEXP getMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseArg) #ifdef WIN32 CopyMemory(RAW(ans), (Rbyte*)lpMapAddress, len*sizeof(Rbyte)); #else - memcpy(RAW(ans), (Rbyte*)addr, len*sizeof(Rbyte)); // maybe need +1 + memcpy(RAW(ans), (Rbyte*)addr, len*sizeof(Rbyte)); #endif if (verbose) Rprintf("* Copy map memory...OK\n"); - + #ifdef WIN32 if (!UnmapViewOfFile(lpMapLength)) { -#else - if (munmap(length, 256) == -1) { -#endif + UnmapViewOfFile(lpMapAddress); + CloseHandle(hMapFile); + CloseHandle(hMapLength); + UNPROTECT(1); error("* Closing mapping file (length)...ERROR"); } if (verbose) Rprintf("* Closing mapping file (length)...OK\n"); -#ifdef WIN32 if (!CloseHandle(hMapLength)) { -#else - if (shm_unlink(pML) == -1) { -#endif + UnmapViewOfFile(lpMapAddress); + CloseHandle(hMapFile); + UNPROTECT(1); error("* Closing mapping handle (length)...ERROR"); } if (verbose) Rprintf("* Closing mapping handle (length)...OK\n"); - -#ifdef WIN32 + if (!UnmapViewOfFile(lpMapAddress)) { -#else - if (munmap(addr, len*sizeof(Rbyte)) == -1) { -#endif + CloseHandle(hMapFile); + UNPROTECT(1); error("* Closing mapping file (address)...ERROR"); } if (verbose) Rprintf("* Closing mapping file (address)...OK\n"); -#ifdef WIN32 if (!CloseHandle(hMapFile)) { -#else - if (shm_unlink(pMN) == -1) { -#endif + UNPROTECT(1); error("* Closing mapping handle (address)...ERROR"); } if (verbose) Rprintf("* Closing mapping handle (address)...OK\n"); +#else + // Non-destructive read: unmap + close only. Lifetime stays with the owner + // handle from shareData(); use clearData() to shm_unlink(). + if (munmap(addr, len*sizeof(Rbyte)) == -1) { + UNPROTECT(1); + error("* Closing mapping file (address)...ERROR"); + } + if (verbose) Rprintf("* Closing mapping file (address)...OK\n"); + if (verbose) Rprintf("* Closing mapping handle (address)...OK\n"); +#endif UNPROTECT(1); return ans; } diff --git a/tests/test_kit.R b/tests/test_kit.R index 21b6b71..5c9b54a 100644 --- a/tests/test_kit.R +++ b/tests/test_kit.R @@ -1766,11 +1766,34 @@ x = tryCatch(shareData(mtcars,"share1"), error=function(err) { if (!is.null(x)) { check("0022.001", getData("share1"), mtcars) - check("0022.002", clearData(x), TRUE) + # Reads are non-destructive (issue #43): repeated gets must all succeed + check("0022.002", getData("share1"), mtcars) + check("0022.003", getData("share1"), mtcars) + check("0022.004", clearData(x), TRUE) + # After owner clears, readers must fail + check("0022.005", tryCatch({getData("share1"); "unexpected-ok"}, error=function(e) "expected-error"), "expected-error") + # Clearing twice returns FALSE + check("0022.006", clearData(x), FALSE) } rm(x) +# Re-sharing the same name with a different payload size must work +x = tryCatch(shareData(1:10, "share-resize"), error=function(err) { + cat("Skipping shareData resize tests:", conditionMessage(err), "\n") + NULL +}) + +if (!is.null(x)) { + check("0022.007", getData("share-resize"), 1:10) + check("0022.008", clearData(x), TRUE) + rm(x) + x = shareData(1:1000, "share-resize") + check("0022.009", getData("share-resize"), 1:1000) + check("0022.010", getData("share-resize"), 1:1000) + check("0022.011", clearData(x), TRUE) +} + # -------------------------------------------------------------------------------------------------- # pcountNA # -------------------------------------------------------------------------------------------------- From 174a9a79579c6059d4c433f2ad1473784a66220d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dawid=20Budzy=C5=84ski?= Date: Tue, 8 Sep 2026 20:20:46 +0200 Subject: [PATCH 2/4] fix(share): seqlock torn-write detection, orphan reap, macOS re-share - Length segment now carries a {len, gen} seqlock header (len stays at offset 0 for back-compat). Readers fail loudly ('please retry') instead of risking torn data when racing a concurrent writer. - Add clearShared(map_name) unlink-by-name for orphan recovery (e.g. after a crashed session); no-op on Windows where mappings die with the last handle. - Same-size re-share republishes in place; size change recreates objects (macOS rejects ftruncate on re-opened shm fds). - fstat-clamped data mapping avoids SIGBUS on concurrent shrink. - No new dependencies. --- NAMESPACE | 2 +- R/call.R | 4 + man/shareData.Rd | 16 ++- src/init.c | 2 + src/kit.h | 1 + src/share.c | 266 +++++++++++++++++++++++++++++++++++++++++------ tests/test_kit.R | 21 +++- 7 files changed, 276 insertions(+), 36 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index 68fb537..b92b852 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -5,5 +5,5 @@ importFrom(utils, packageVersion) export( charToFact, count, countNA, countOccur, fduplicated, fpmin, fpmax, fpos, funique, iif, nif, nswitch, pall, pallNA, pallv, pany, panyNA, panyv, pcount, pcountNA, pfirst, plast, pmean, pprod, prange, psum, setlevels, topn, uniqLen, vswitch, psort, - getData, shareData, clearData + getData, shareData, clearData, clearShared ) diff --git a/R/call.R b/R/call.R index c953991..f4b38e3 100644 --- a/R/call.R +++ b/R/call.R @@ -1,6 +1,10 @@ # Function calls charToFact = function(x, decreasing=FALSE, addNA=TRUE, nThread=getOption("kit.nThread")) .Call(CcharToFactR, x, decreasing, nThread, NA, parent.frame(), addNA) clearData = function(x, verbose=FALSE) .Call("CclearMappingObjectR", x, verbose) +clearShared = function(map_name, verbose=FALSE) { + map_name = shmName(map_name) + .Call("CunlinkMappingObjectR", map_name, paste0(map_name,"_key"), verbose) +} count = function(x, value) .Call(CcountR, x, value) countNA = function(x) .Call(CcountNAR, x) countOccur = function(x) .Call(CcountOccurR, x) diff --git a/man/shareData.Rd b/man/shareData.Rd index df26a19..3471017 100644 --- a/man/shareData.Rd +++ b/man/shareData.Rd @@ -1,7 +1,8 @@ -\name{shareData/getData/clearData} +\name{shareData/getData/clearData/clearShared} \alias{shareData} \alias{getData} \alias{clearData} +\alias{clearShared} \title{ Share Data between R Sessions} \description{ Experimental functions that enable the user to share a R object between 2 \R sessions. @@ -10,23 +11,27 @@ The object is serialized once by \code{shareData} into POSIX shared memory current session and/or from other processes running as the same user, via \code{getData}. Reads are non-destructive: the segment lives until the owner handle returned by \code{shareData} is cleared with \code{clearData} (or -garbage collected). Keep the owner object alive while readers are active. +garbage collected). Keep the owner object alive while readers are active, +e.g. with \code{on.exit(clearData(x))}, and clear only after all readers +(including background workers) are done. } \usage{ shareData(data, map_name, verbose=FALSE) getData(map_name, verbose=FALSE) clearData(x, verbose=FALSE) +clearShared(map_name, verbose=FALSE) } \arguments{ \item{data}{ A \R object like a vector or a \code{data.frame}.} - \item{map_name}{ A character. A name for the memory map location where to store the data.} + \item{map_name}{ A character. A name for the memory map location where to store the data. Concurrent writers must use unique names: re-sharing a name while readers are active fails those reads loudly instead of returning mixed data.} \item{x}{ An external pointer like the one returned by function \code{shareData}.} \item{verbose}{ A logical value \code{TRUE} or \code{FALSE} to provide or not information to the user.} } \value{ \code{shareData} returns a external pointer. -\code{getData} returns an \R object stored in the memory location \code{map_name}. Reads do not consume the segment, so \code{getData} may be called repeatedly and from multiple processes. +\code{getData} returns an \R object stored in the memory location \code{map_name}. Reads do not consume the segment, so \code{getData} may be called repeatedly and from multiple processes. A read racing a concurrent write errors (\code{please retry}) instead of returning torn data. \code{clearData} returns \code{TRUE} or \code{FALSE} depending on whether the data have been cleared in memory. +\code{clearShared} unlinks a segment by name, without needing the owner handle, and returns \code{TRUE} if anything was removed. Use it to reap orphaned segments (e.g. after a crashed session); on Windows it is a harmless no-op because mappings vanish with the last open handle. } \author{Morgan Jacob} \examples{ @@ -46,4 +51,7 @@ clearData(x, verbose=FALSE) # > # mirai::daemons(4) # > # mirai::mirai_map(ids, function(i) fun(i, kit::getData("/mydata"))) # > # mirai::daemons(0); clearData(x) +# +# Reap a leftover segment by name (POSIX shared memory survives crashes): +# > clearShared("/mydata") } diff --git a/src/init.c b/src/init.c index 5244e65..b3adc0b 100644 --- a/src/init.c +++ b/src/init.c @@ -29,6 +29,7 @@ static const R_CallMethodDef CallEntries[] = { {"CvswitchR", (DL_FUNC) &vswitchR, -1}, {"CcreateMappingObjectR", (DL_FUNC) &createMappingObjectR, -1}, {"CgetMappingObjectR", (DL_FUNC) &getMappingObjectR, -1}, + {"CunlinkMappingObjectR", (DL_FUNC) &unlinkMappingObjectR, -1}, {"CclearMappingObjectR", (DL_FUNC) &clearMappingObjectR, -1}, {NULL, NULL, -1} }; @@ -63,5 +64,6 @@ void R_init_kit(DllInfo *dll) { R_RegisterCCallable("kit", "CvswitchR", (DL_FUNC) &vswitchR); R_RegisterCCallable("kit", "CcreateMappingObjectR", (DL_FUNC) &createMappingObjectR); R_RegisterCCallable("kit", "CgetMappingObjectR", (DL_FUNC) &getMappingObjectR); + R_RegisterCCallable("kit", "CunlinkMappingObjectR", (DL_FUNC) &unlinkMappingObjectR); R_RegisterCCallable("kit", "CclearMappingObjectR", (DL_FUNC) &clearMappingObjectR); } diff --git a/src/kit.h b/src/kit.h index 2c4b032..93f6fe5 100644 --- a/src/kit.h +++ b/src/kit.h @@ -117,6 +117,7 @@ extern SEXP vswitchR(SEXP x, SEXP values, SEXP outputs, SEXP na, SEXP nthreads, extern SEXP createMappingObjectR(SEXP MapName, SEXP MapLength, SEXP DataObject, SEXP verboseArg); extern SEXP getMappingObjectR(SEXP MapName, SEXP MapLength, SEXP verboseArg); +extern SEXP unlinkMappingObjectR(SEXP MapName, SEXP MapLength, SEXP verboseArg); extern SEXP clearMappingObjectR(SEXP ext, SEXP verboseArg); union uno { double d; unsigned int u[2]; }; diff --git a/src/share.c b/src/share.c index 0cb233c..16bb784 100644 --- a/src/share.c +++ b/src/share.c @@ -19,6 +19,41 @@ #include "kit.h" #include +/* + * Metadata stored at offset 0 of the 256-byte length segment. + * `len` stays first so segments written by older versions (which stored + * only `len`) still read correctly; their `gen` bytes are zero (even = ready). + * + * Single-writer (owner) / multi-reader seqlock: odd `gen` means a write is + * in progress. Concurrent misuse (read during write, re-share under active + * readers) fails loudly instead of silently corrupting. No mutexes, no + * dependencies. Correct use needs no lock: share() returns only after the + * payload is published, so readers starting afterwards never see odd `gen`. + */ + +#define SHM_META_SIZE 256 + +struct SHM_META { + size_t len; + uint64_t gen; +}; + +static void shm_release_fence(void) { +#ifdef WIN32 + MemoryBarrier(); +#elif defined(__GNUC__) || defined(__clang__) + __atomic_thread_fence(__ATOMIC_RELEASE); +#endif +} + +static void shm_acquire_fence(void) { +#ifdef WIN32 + MemoryBarrier(); +#elif defined(__GNUC__) || defined(__clang__) + __atomic_thread_fence(__ATOMIC_ACQUIRE); +#endif +} + /* * Structure to hold Length and Address * of data to be shared in memory segment @@ -147,8 +182,17 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje error("* Map view file...ERROR"); } if (verbose) Rprintf("* Map view file...OK\n"); + // Seqlock publish: mark writing, copy payload, then publish size + ready. + struct SHM_META *metaW = (struct SHM_META *) foo->lpMapLength; + uint64_t gW = metaW->gen; + if (gW & 1ULL) gW++; // recover from a torn previous write (e.g. crashed writer) + metaW->gen = gW + 1ULL; // odd: write in progress + shm_release_fence(); CopyMemory((LPVOID)foo->lpMapAddress, RAW(DataObject), BUF_SIZE); - CopyMemory((LPVOID)foo->lpMapLength, &len, sizeof(size_t)); + shm_release_fence(); + metaW->len = len; + shm_release_fence(); + metaW->gen = gW + 2ULL; // even: ready #else const char *pMN = CHAR(STRING_PTR_RO(MapObjectName)[0]); const char *pML = CHAR(STRING_PTR_RO(MapLengthName)[0]); @@ -184,8 +228,14 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje error("* Creating file mapping...ERROR"); } if (verbose) Rprintf("* Creating file maping...OK\n"); - // Always size the objects: re-sharing the same name must reflect the new payload. - if (ftruncate(foo->fd_addr, BUF_SIZE) == -1 || ftruncate(foo->fd_length, 256) == -1) { + // Size the objects. Same-size re-share reuses them (payload + seqlock + // generation are simply republished). A size change recreates them: on some + // platforms (e.g. macOS) ftruncate fails on fds from re-opening an existing + // object, so the stale objects are unlinked and fresh (truncatable) ones are + // created. In-flight readers of the old objects keep reading the old + // payload; only new opens see the new one. + struct stat st_addr, st_len; + if (fstat(foo->fd_addr, &st_addr) == -1 || fstat(foo->fd_length, &st_len) == -1) { close(foo->fd_addr); close(foo->fd_length); free(foo->STORAGE_ID); @@ -193,7 +243,38 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje R_ClearExternalPtr(ext); R_Free(foo); UNPROTECT(1); - error("* Extend shared memory object...ERROR"); + error("* Stat shared memory object...ERROR"); + } + if (st_addr.st_size < 0 || (size_t) st_addr.st_size != BUF_SIZE || + st_len.st_size < 0 || (size_t) st_len.st_size != SHM_META_SIZE) { + close(foo->fd_addr); + close(foo->fd_length); + shm_unlink(foo->STORAGE_ID); + shm_unlink(foo->LENGTH_ID); + foo->fd_addr = shm_open(foo->STORAGE_ID, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); + foo->fd_length = shm_open(foo->LENGTH_ID, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); + if (foo->fd_addr == -1 || foo->fd_length == -1) { + Rprintf("shm_open error, errno(%d): %s\n", errno, strerror(errno)); + if (foo->fd_addr != -1) close(foo->fd_addr); + if (foo->fd_length != -1) close(foo->fd_length); + free(foo->STORAGE_ID); + free(foo->LENGTH_ID); + R_ClearExternalPtr(ext); + R_Free(foo); + UNPROTECT(1); + error("* Recreating file mapping...ERROR"); + } + if (ftruncate(foo->fd_addr, BUF_SIZE) == -1 || ftruncate(foo->fd_length, SHM_META_SIZE) == -1) { + Rprintf("ftruncate error, errno(%d): %s\n", errno, strerror(errno)); + close(foo->fd_addr); + close(foo->fd_length); + free(foo->STORAGE_ID); + free(foo->LENGTH_ID); + R_ClearExternalPtr(ext); + R_Free(foo); + UNPROTECT(1); + error("* Extend shared memory object...ERROR"); + } } if (verbose) Rprintf("* Extend shared memory object...OK\n"); foo->addr = mmap(NULL, BUF_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, foo->fd_addr, 0); @@ -226,8 +307,17 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje } foo->fd_addr = -1; foo->fd_length = -1; + // Seqlock publish: mark writing, copy payload, then publish size + ready. + struct SHM_META *meta = (struct SHM_META *) foo->length; + uint64_t g = meta->gen; + if (g & 1ULL) g++; // recover from a torn previous write (e.g. crashed writer) + meta->gen = g + 1ULL; // odd: write in progress + shm_release_fence(); memcpy(foo->addr, RAW(DataObject), BUF_SIZE); - memcpy(foo->length, &len, sizeof(size_t)); + shm_release_fence(); + meta->len = len; + shm_release_fence(); + meta->gen = g + 2ULL; // even: ready #endif if (verbose) Rprintf("* Copy memory...OK\n"); R_RegisterCFinalizerEx(ext, map_finalizer, TRUE); @@ -269,13 +359,16 @@ SEXP getMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseArg) error("* Creating file mapping...ERROR"); } if (verbose) Rprintf("* Creating file maping...OK\n"); + size_t len_a = 0; + size_t map_len = 0; + uint64_t gen_a = 0; #ifdef WIN32 - LPCTSTR lpMapLength = (LPCTSTR) MapViewOfFile (hMapLength, FILE_MAP_ALL_ACCESS, 0, 0, 256); + LPCTSTR lpMapLength = (LPCTSTR) MapViewOfFile (hMapLength, FILE_MAP_ALL_ACCESS, 0, 0, SHM_META_SIZE); if (lpMapLength == NULL) { CloseHandle(hMapFile); CloseHandle(hMapLength); #else - void *length = mmap(NULL, 256, PROT_READ, MAP_SHARED, fd_length, 0); + void *length = mmap(NULL, SHM_META_SIZE, PROT_READ, MAP_SHARED, fd_length, 0); if (length == MAP_FAILED) { close(fd_addr); close(fd_length); @@ -283,59 +376,97 @@ SEXP getMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseArg) error("* Map view file (length)...ERROR"); } if (verbose) Rprintf("* Map view file (length)...OK\n"); + // Seqlock: observe the header before the payload copy. An odd generation + // means a writer is mid-publish, so fail fast instead of reading torn data. + struct SHM_META *metaR = NULL; +#ifdef WIN32 + metaR = (struct SHM_META *) lpMapLength; + len_a = metaR->len; + gen_a = metaR->gen; + shm_acquire_fence(); + if (gen_a & 1ULL) { + UnmapViewOfFile(lpMapLength); + CloseHandle(hMapFile); + CloseHandle(hMapLength); +#else + metaR = (struct SHM_META *) length; + len_a = metaR->len; + gen_a = metaR->gen; + shm_acquire_fence(); + if (gen_a & 1ULL) { + munmap(length, SHM_META_SIZE); + close(fd_addr); + close(fd_length); +#endif + error("* Shared data is being written, please retry...ERROR"); + } + if (verbose) Rprintf("* Generation check...OK\n"); #ifdef WIN32 - size_t len = *(size_t*)lpMapLength; LPCTSTR lpMapAddress = NULL; - if (len > 0) { - lpMapAddress = (LPCTSTR) MapViewOfFile (hMapFile, FILE_MAP_ALL_ACCESS, 0, 0, len*sizeof(Rbyte)); + if (len_a > 0) { + lpMapAddress = (LPCTSTR) MapViewOfFile (hMapFile, FILE_MAP_ALL_ACCESS, 0, 0, len_a*sizeof(Rbyte)); } - if (len == 0 || lpMapAddress == NULL) { + if (len_a == 0 || lpMapAddress == NULL) { UnmapViewOfFile(lpMapLength); CloseHandle(hMapFile); CloseHandle(hMapLength); #else - size_t len = *(size_t*)length; - // Detach the length mapping as soon as the size is known; data mapping stays. - if (munmap(length, 256) == -1) { + // Clamp the data mapping to the object size observed right now: if a writer + // concurrently re-shares a smaller payload, mapping the stale (larger) size + // could SIGBUS on access. The seqlock re-check after the copy turns any such + // race into a clean error. Concurrent writers must use unique map names. + map_len = len_a*sizeof(Rbyte); + struct stat addrstat; + if (len_a == 0 || fstat(fd_addr, &addrstat) == -1) { + munmap(length, SHM_META_SIZE); close(fd_addr); close(fd_length); - error("* Closing mapping file (length)...ERROR"); + error("* Map view file (address)...ERROR"); } - if (verbose) Rprintf("* Closing mapping file (length)...OK\n"); - if (close(fd_length) == -1) { + if ((size_t) addrstat.st_size < map_len) map_len = (size_t) addrstat.st_size; + // fd_length stays open: the header is re-read after the copy to validate. + if (munmap(length, SHM_META_SIZE) == -1) { close(fd_addr); - error("* Closing file descriptor (length)...ERROR"); + close(fd_length); + error("* Closing mapping file (length)...ERROR"); } - if (verbose) Rprintf("* Closing mapping handle (length)...OK\n"); + if (verbose) Rprintf("* Closing mapping file (length)...OK\n"); void *addr = NULL; - if (len > 0) { - addr = mmap(NULL, len*sizeof(Rbyte), PROT_READ, MAP_SHARED, fd_addr, 0); + if (map_len > 0) { + addr = mmap(NULL, map_len, PROT_READ, MAP_SHARED, fd_addr, 0); } - if (len == 0 || addr == MAP_FAILED) { + if (map_len == 0 || addr == MAP_FAILED) { close(fd_addr); + close(fd_length); #endif error("* Map view file (address)...ERROR"); } if (verbose) Rprintf("* Map view file (address)...OK\n"); - // fds no longer needed once mappings are established. - // Keep fd_addr open until after mmap; fd_length already closed on POSIX. + // The data mapping is established; its fd can go. fd_length is still needed + // for the post-copy header revalidation below. #ifdef WIN32 #else if (close(fd_addr) == -1) { - munmap(addr, len*sizeof(Rbyte)); + munmap(addr, map_len); + close(fd_length); error("* Closing file descriptor (address)...ERROR"); } #endif - SEXP ans = PROTECT(allocVector(RAWSXP, len)); + SEXP ans = PROTECT(allocVector(RAWSXP, len_a)); if (verbose) Rprintf("* Create RAW Vector...OK\n"); #ifdef WIN32 - CopyMemory(RAW(ans), (Rbyte*)lpMapAddress, len*sizeof(Rbyte)); + CopyMemory(RAW(ans), (Rbyte*)lpMapAddress, len_a*sizeof(Rbyte)); #else - memcpy(RAW(ans), (Rbyte*)addr, len*sizeof(Rbyte)); + memcpy(RAW(ans), (Rbyte*)addr, map_len); #endif if (verbose) Rprintf("* Copy map memory...OK\n"); #ifdef WIN32 + // Seqlock revalidation: the header must be unchanged since the pre-copy read. + shm_acquire_fence(); + size_t len_b = metaR->len; + uint64_t gen_b = metaR->gen; + bool torn_win = ((gen_a != gen_b) || (gen_a & 1ULL) || (len_a != len_b)); if (!UnmapViewOfFile(lpMapLength)) { UnmapViewOfFile(lpMapAddress); CloseHandle(hMapFile); @@ -351,6 +482,12 @@ SEXP getMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseArg) error("* Closing mapping handle (length)...ERROR"); } if (verbose) Rprintf("* Closing mapping handle (length)...OK\n"); + if (torn_win) { + UnmapViewOfFile(lpMapAddress); + CloseHandle(hMapFile); + UNPROTECT(1); + error("* Shared data changed during read, please retry...ERROR"); + } if (!UnmapViewOfFile(lpMapAddress)) { CloseHandle(hMapFile); @@ -366,17 +503,86 @@ SEXP getMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseArg) #else // Non-destructive read: unmap + close only. Lifetime stays with the owner // handle from shareData(); use clearData() to shm_unlink(). - if (munmap(addr, len*sizeof(Rbyte)) == -1) { + if (munmap(addr, map_len) == -1) { + close(fd_length); UNPROTECT(1); error("* Closing mapping file (address)...ERROR"); } if (verbose) Rprintf("* Closing mapping file (address)...OK\n"); - if (verbose) Rprintf("* Closing mapping handle (address)...OK\n"); + // Seqlock revalidation against the pre-copy header. + void *length2 = mmap(NULL, SHM_META_SIZE, PROT_READ, MAP_SHARED, fd_length, 0); + if (length2 == MAP_FAILED) { + close(fd_length); + UNPROTECT(1); + error("* Map view file (length)...ERROR"); + } + struct SHM_META *metaR2 = (struct SHM_META *) length2; + size_t len_b = metaR2->len; + uint64_t gen_b = metaR2->gen; + shm_acquire_fence(); + bool torn_posix = ((gen_a != gen_b) || (gen_a & 1ULL) || (len_a != len_b) || + (map_len != len_a*sizeof(Rbyte))); + if (munmap(length2, SHM_META_SIZE) == -1) { + close(fd_length); + UNPROTECT(1); + error("* Closing mapping file (length)...ERROR"); + } + if (close(fd_length) == -1) { + UNPROTECT(1); + error("* Closing mapping handle (length)...ERROR"); + } + if (verbose) Rprintf("* Closing mapping handle (length)...OK\n"); + if (torn_posix) { + UNPROTECT(1); + error("* Shared data changed during read, please retry...ERROR"); + } #endif UNPROTECT(1); return ans; } +/* + * Function to unlink a mapping by name (orphan recovery without owner handle) + */ + +SEXP unlinkMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseArg) { + if (TYPEOF(MapObjectName) != STRSXP || LENGTH(MapObjectName) != 1) { + error("Argument 'MapObjectName' must be of type character and length 1."); + } + if (!IS_BOOL(verboseArg)) { + error("Argument 'verbose' must be TRUE or FALSE."); + } + const bool verbose = asLogical(verboseArg); +#ifdef WIN32 + // Named file mappings vanish with the last open handle; there is no name + // to unlink. Probe only, so the call stays harmless cross-platform. + LPSTR pMN = (LPSTR) CHAR(STRING_PTR_RO(MapObjectName)[0]); + LPSTR pML = (LPSTR) CHAR(STRING_PTR_RO(MapLengthName)[0]); + HANDLE hMapFile = OpenFileMapping(FILE_MAP_ALL_ACCESS, FALSE, pMN); + HANDLE hMapLength = OpenFileMapping(FILE_MAP_ALL_ACCESS, FALSE, pML); + if (hMapFile != NULL && hMapFile != INVALID_HANDLE_VALUE) CloseHandle(hMapFile); + if (hMapLength != NULL && hMapLength != INVALID_HANDLE_VALUE) CloseHandle(hMapLength); + if (verbose) Rprintf("* Unlink by name is a no-op on Windows...OK\n"); + return ScalarLogical(FALSE); +#else + const char *pMN = CHAR(STRING_PTR_RO(MapObjectName)[0]); + const char *pML = CHAR(STRING_PTR_RO(MapLengthName)[0]); + bool unlinked = false; + if (shm_unlink(pMN) == 0) { + unlinked = true; + } else if (errno != ENOENT) { + error("* Unlink shared data...ERROR"); + } + if (shm_unlink(pML) == 0) { + unlinked = true; + } else if (errno != ENOENT) { + error("* Unlink shared data length...ERROR"); + } + if (verbose) Rprintf("* Unlink by name (removed=%d)...OK\n", unlinked ? 1 : 0); + return unlinked ? ScalarLogical(TRUE) : ScalarLogical(FALSE); +#endif +} + /* * Function to clear mapping object */ diff --git a/tests/test_kit.R b/tests/test_kit.R index 5c9b54a..6e07131 100644 --- a/tests/test_kit.R +++ b/tests/test_kit.R @@ -68,6 +68,7 @@ charToFact = kit::charToFact shareData = kit::shareData getData = kit::getData clearData = kit::clearData +clearShared = kit::clearShared # -------------------------------------------------------------------------------------------------- # topn @@ -1794,6 +1795,24 @@ if (!is.null(x)) { check("0022.011", clearData(x), TRUE) } +rm(x) + +# clearShared() reaps a live segment by name, without the owner handle +x = tryCatch(shareData(mtcars, "share-orphan"), error=function(err) { + cat("Skipping clearShared tests:", conditionMessage(err), "\n") + NULL +}) + +if (!is.null(x)) { + check("0022.012", getData("share-orphan"), mtcars) + check("0022.013", clearShared("share-orphan"), TRUE) + check("0022.014", tryCatch({getData("share-orphan"); "unexpected-ok"}, error=function(e) "expected-error"), "expected-error") + check("0022.015", clearShared("share-orphan"), FALSE) + check("0022.016", clearData(x), TRUE) +} + +rm(x) + # -------------------------------------------------------------------------------------------------- # pcountNA # -------------------------------------------------------------------------------------------------- @@ -1926,7 +1945,7 @@ rm(x, y, z, x1, y1, z1, base_pfirst, base_plast) # -------------------------------------------------------------------------------------------------- rm(check,count,countNA,countOccur,fduplicated,fpos,funique,iif,nswitch,nif,pall,pany,pcount,pcountNA, - pmean,pprod,psum,setlevels,topn,uniqLen,vswitch,psort,charToFact,shareData,getData,clearData, + pmean,pprod,psum,setlevels,topn,uniqLen,vswitch,psort,charToFact,shareData,getData,clearData,clearShared, pallNA, pallv, panyv, panyNA, pfirst, plast, fpmax, fpmin, prange) # -------------------------------------------------------------------------------------------------- From 39f50710d60073207bb9adc95de337af68e8dd62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dawid=20Budzy=C5=84ski?= Date: Tue, 8 Sep 2026 20:37:31 +0200 Subject: [PATCH 3/4] fix(share): review hardening - validation, atomics, single-writer, portable tests - Validate MapLengthName everywhere; EXTPTRSXP check in clearData; RAWSXP check + XLENGTH in creator; R-side map_name/verbose checks. - Ordered header access via __atomic load/store (acquire/release); 32-bit gen for single-copy atomicity; loud fallback warning. - Busy-name detection: odd generation fails loudly (dual-writer or crashed writer) instead of interleaving; recover via clearShared. - Reader opens header before payload so resize mixes are unreachable; fstat header before mapping (SIGBUS guard); early fail-fast check. - Unlink-on-error only for objects this call created (fresh-tracked). - Exact .Call arities; DWORD size split on Windows; errno saved before diagnostics; SHM_META_SIZE used consistently. - Tests hermetic via pre-clean; Windows branches for clearShared; POSIX-only no-clear re-share block; pkgdown topic updated. --- R/call.R | 24 +++- _pkgdown.yml | 2 +- man/shareData.Rd | 13 ++- src/init.c | 8 +- src/share.c | 288 ++++++++++++++++++++++++++++++++++------------- tests/test_kit.R | 42 ++++++- 6 files changed, 275 insertions(+), 102 deletions(-) diff --git a/R/call.R b/R/call.R index f4b38e3..399cbc4 100644 --- a/R/call.R +++ b/R/call.R @@ -1,9 +1,21 @@ # Function calls +checkMapName = function(map_name) { + if (!is.character(map_name) || length(map_name) != 1L || is.na(map_name) || !nzchar(map_name)) + stop("Argument 'map_name' must be a non-empty string of length 1.") + shmName(map_name) +} + +checkVerbose = function(verbose) { + if (!is.logical(verbose) || length(verbose) != 1L || is.na(verbose)) + stop("Argument 'verbose' must be TRUE or FALSE.") + verbose +} + charToFact = function(x, decreasing=FALSE, addNA=TRUE, nThread=getOption("kit.nThread")) .Call(CcharToFactR, x, decreasing, nThread, NA, parent.frame(), addNA) -clearData = function(x, verbose=FALSE) .Call("CclearMappingObjectR", x, verbose) +clearData = function(x, verbose=FALSE) .Call("CclearMappingObjectR", x, checkVerbose(verbose)) clearShared = function(map_name, verbose=FALSE) { - map_name = shmName(map_name) - .Call("CunlinkMappingObjectR", map_name, paste0(map_name,"_key"), verbose) + map_name = checkMapName(map_name) + .Call("CunlinkMappingObjectR", map_name, paste0(map_name,"_key"), checkVerbose(verbose)) } count = function(x, value) .Call(CcountR, x, value) countNA = function(x) .Call(CcountNAR, x) @@ -68,10 +80,11 @@ psort = function(x, decreasing = FALSE, na.last = NA, nThread=getOption("kit.nTh shmName = function(map_name) sub("^/*", "/", map_name) shareData = function(data, map_name, verbose=FALSE) { + map_name = checkMapName(map_name) + verbose = checkVerbose(verbose) conn = rawConnection(raw(0L), "w") on.exit(close(conn), add = TRUE) serialize(data, conn) - map_name = shmName(map_name) .Call( "CcreateMappingObjectR", map_name, paste0(map_name,"_key"), rawConnectionValue(conn), verbose @@ -79,7 +92,8 @@ shareData = function(data, map_name, verbose=FALSE) { } getData = function(map_name, verbose=FALSE) { - map_name = shmName(map_name) + map_name = checkMapName(map_name) + verbose = checkVerbose(verbose) output = .Call("CgetMappingObjectR", map_name, paste0(map_name,"_key"), verbose) conn = rawConnection(output,"r") on.exit(close(conn), add = TRUE) diff --git a/_pkgdown.yml b/_pkgdown.yml index 964026c..4353d36 100644 --- a/_pkgdown.yml +++ b/_pkgdown.yml @@ -73,7 +73,7 @@ reference: desc: "Find a matrix position inside a larger matrix and share data between R sessions." - contents: - fpos - - shareData/getData/clearData + - shareData/getData/clearData/clearShared articles: - title: "Introduction to kit" diff --git a/man/shareData.Rd b/man/shareData.Rd index 3471017..98a0a3f 100644 --- a/man/shareData.Rd +++ b/man/shareData.Rd @@ -3,9 +3,9 @@ \alias{getData} \alias{clearData} \alias{clearShared} -\title{ Share Data between R Sessions} +\title{Share Data between R Sessions} \description{ -Experimental functions that enable the user to share a R object between 2 \R sessions. +Experimental functions that enable the user to share an R object between R sessions. The object is serialized once by \code{shareData} into POSIX shared memory (or a Windows file mapping). It can then be read multiple times, from the current session and/or from other processes running as the same user, via @@ -13,7 +13,10 @@ current session and/or from other processes running as the same user, via handle returned by \code{shareData} is cleared with \code{clearData} (or garbage collected). Keep the owner object alive while readers are active, e.g. with \code{on.exit(clearData(x))}, and clear only after all readers -(including background workers) are done. +(including background workers) are done. Exactly one writer may publish a +name at a time; do not re-assign the owner variable without clearing first +(\code{clearData} the old handle before re-sharing its name), and do not +re-share a name at a different size while readers are active. } \usage{ shareData(data, map_name, verbose=FALSE) @@ -23,7 +26,7 @@ clearShared(map_name, verbose=FALSE) } \arguments{ \item{data}{ A \R object like a vector or a \code{data.frame}.} - \item{map_name}{ A character. A name for the memory map location where to store the data. Concurrent writers must use unique names: re-sharing a name while readers are active fails those reads loudly instead of returning mixed data.} + \item{map_name}{ A character. A name for the memory map location where to store the data. Concurrent writers must use unique names: re-sharing a name while readers are active fails those reads loudly instead of returning mixed data. On Windows a live name cannot grow: clear it before re-sharing a larger object.} \item{x}{ An external pointer like the one returned by function \code{shareData}.} \item{verbose}{ A logical value \code{TRUE} or \code{FALSE} to provide or not information to the user.} } @@ -31,7 +34,7 @@ clearShared(map_name, verbose=FALSE) \code{shareData} returns a external pointer. \code{getData} returns an \R object stored in the memory location \code{map_name}. Reads do not consume the segment, so \code{getData} may be called repeatedly and from multiple processes. A read racing a concurrent write errors (\code{please retry}) instead of returning torn data. \code{clearData} returns \code{TRUE} or \code{FALSE} depending on whether the data have been cleared in memory. -\code{clearShared} unlinks a segment by name, without needing the owner handle, and returns \code{TRUE} if anything was removed. Use it to reap orphaned segments (e.g. after a crashed session); on Windows it is a harmless no-op because mappings vanish with the last open handle. +\code{clearShared} unlinks a segment by name, without needing the owner handle, and returns \code{TRUE} if anything was removed. Use it to reap orphaned segments (e.g. after a crashed session); on Windows it is a harmless no-op because mappings vanish with the last open handle, and always returns \code{FALSE} there. } \author{Morgan Jacob} \examples{ diff --git a/src/init.c b/src/init.c index b3adc0b..fc55158 100644 --- a/src/init.c +++ b/src/init.c @@ -27,10 +27,10 @@ static const R_CallMethodDef CallEntries[] = { {"CsetlevelsR", (DL_FUNC) &setlevelsR, -1}, {"CtopnR", (DL_FUNC) &topnR, -1}, {"CvswitchR", (DL_FUNC) &vswitchR, -1}, - {"CcreateMappingObjectR", (DL_FUNC) &createMappingObjectR, -1}, - {"CgetMappingObjectR", (DL_FUNC) &getMappingObjectR, -1}, - {"CunlinkMappingObjectR", (DL_FUNC) &unlinkMappingObjectR, -1}, - {"CclearMappingObjectR", (DL_FUNC) &clearMappingObjectR, -1}, + {"CcreateMappingObjectR", (DL_FUNC) &createMappingObjectR, 4}, + {"CgetMappingObjectR", (DL_FUNC) &getMappingObjectR, 3}, + {"CunlinkMappingObjectR", (DL_FUNC) &unlinkMappingObjectR, 3}, + {"CclearMappingObjectR", (DL_FUNC) &clearMappingObjectR, 2}, {NULL, NULL, -1} }; diff --git a/src/share.c b/src/share.c index 16bb784..b533099 100644 --- a/src/share.c +++ b/src/share.c @@ -18,40 +18,73 @@ #include "kit.h" #include +#include // offsetof for the layout assertions below /* - * Metadata stored at offset 0 of the 256-byte length segment. + * Metadata stored at offset 0 of the SHM_META_SIZE-byte length segment. * `len` stays first so segments written by older versions (which stored * only `len`) still read correctly; their `gen` bytes are zero (even = ready). * * Single-writer (owner) / multi-reader seqlock: odd `gen` means a write is - * in progress. Concurrent misuse (read during write, re-share under active - * readers) fails loudly instead of silently corrupting. No mutexes, no - * dependencies. Correct use needs no lock: share() returns only after the - * payload is published, so readers starting afterwards never see odd `gen`. + * in progress. Readers validate the header before AND after the payload + * copy, so misuse fails loudly instead of silently corrupting. No mutexes, + * no dependencies. + * + * Contracts (also stated in ?shareData, enforced where cheap): + * - exactly one writer publishes a name at a time; a second writer seeing + * an odd generation fails loudly instead of interleaving payloads; + * - readers may run concurrently with one writer and with each other; + * - resizing a live name requires quiesced readers (same-size re-share is + * always safe); the owner handle must outlive all readers. + * Correct use needs no lock: share() returns only after the payload is + * published, so readers starting afterwards never see odd `gen`. + * + * `gen` is 32-bit so header loads/stores are single-copy-atomic even on + * 32-bit targets (mmap is page-aligned, hence naturally aligned). */ #define SHM_META_SIZE 256 struct SHM_META { size_t len; - uint64_t gen; + uint32_t gen; }; -static void shm_release_fence(void) { -#ifdef WIN32 - MemoryBarrier(); -#elif defined(__GNUC__) || defined(__clang__) - __atomic_thread_fence(__ATOMIC_RELEASE); -#endif -} +// Layout guarantees: header fits the mapping, len stays first for back-compat. +typedef char shm_meta_size_check[(sizeof(struct SHM_META) <= SHM_META_SIZE) ? 1 : -1]; +typedef char shm_meta_len_first_check[(offsetof(struct SHM_META, len) == 0) ? 1 : -1]; -static void shm_acquire_fence(void) { -#ifdef WIN32 - MemoryBarrier(); -#elif defined(__GNUC__) || defined(__clang__) - __atomic_thread_fence(__ATOMIC_ACQUIRE); +// Ordered header access. Every R toolchain (GCC, Clang, mingw) provides +// __atomic builtins; anything else falls back to plain accesses (still +// correct on strongly-ordered CPUs; the generation check fails loudly +// otherwise). The payload copy itself is never trusted, only validated. +#if defined(__GNUC__) || defined(__clang__) +static uint32_t shm_load_gen_acquire(const uint32_t *p) { + return __atomic_load_n(p, __ATOMIC_ACQUIRE); +} +static void shm_store_gen_release(uint32_t *p, uint32_t v) { + __atomic_store_n(p, v, __ATOMIC_RELEASE); +} +static size_t shm_load_len_acquire(const size_t *p) { + return __atomic_load_n(p, __ATOMIC_ACQUIRE); +} +static void shm_store_len_release(size_t *p, size_t v) { + __atomic_store_n(p, v, __ATOMIC_RELEASE); +} +#else +#warning "ordered shared-memory access unavailable: seqlock ordering best-effort only" +static uint32_t shm_load_gen_acquire(const uint32_t *p) { return *p; } +static void shm_store_gen_release(uint32_t *p, uint32_t v) { *p = v; } +static size_t shm_load_len_acquire(const size_t *p) { return *p; } +static void shm_store_len_release(size_t *p, size_t v) { *p = v; } #endif + +// strdup without feature-test-macro dependence (strict-ISO safe). +static char *shm_dup_string(const char *s) { + size_t n = strlen(s) + 1; + char *p = (char *) malloc(n); + if (p != NULL) memcpy(p, s, n); + return p; } /* @@ -95,6 +128,10 @@ static void map_finalizer (SEXP ext) { if (ptr->lpMapLength != NULL) UnmapViewOfFile(ptr->lpMapLength); if (ptr->hMapLength != NULL && ptr->hMapLength != INVALID_HANDLE_VALUE) CloseHandle(ptr->hMapLength); #else + // fds are normally already closed (set to -1); close defensively in case a + // future path registers the finalizer while still holding descriptors. + if (ptr->fd_addr >= 0) close(ptr->fd_addr); + if (ptr->fd_length >= 0) close(ptr->fd_length); if (ptr->addr != NULL && ptr->addr != MAP_FAILED && ptr->STORAGE_SIZE > 0) { munmap(ptr->addr, ptr->STORAGE_SIZE); } @@ -103,15 +140,15 @@ static void map_finalizer (SEXP ext) { free(ptr->STORAGE_ID); } if (ptr->length != NULL && ptr->length != MAP_FAILED) { - munmap(ptr->length, 256); + munmap(ptr->length, SHM_META_SIZE); } if (ptr->LENGTH_ID != NULL) { shm_unlink(ptr->LENGTH_ID); free(ptr->LENGTH_ID); } #endif + R_ClearExternalPtr(ext); // disarm first so GC re-entry is a safe no-op R_Free(ptr); - R_ClearExternalPtr(ext); if (verbose_finalizer) Rprintf("* Clear external pointer...OK\n"); } @@ -123,12 +160,22 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje if (TYPEOF(MapObjectName) != STRSXP || LENGTH(MapObjectName) != 1) { error("Argument 'MapObjectName' must be of type character and length 1."); } + if (TYPEOF(MapLengthName) != STRSXP || LENGTH(MapLengthName) != 1) { + error("Argument 'MapLengthName' must be of type character and length 1."); + } + if (TYPEOF(DataObject) != RAWSXP) { + error("Argument 'DataObject' must be a raw vector."); + } if (!IS_BOOL(verboseArg)) { error("Argument 'verbose' must be TRUE or FALSE."); } const bool verbose = asLogical(verboseArg); verbose_finalizer = verbose; - const size_t len = LENGTH(DataObject); + const R_xlen_t xlen = XLENGTH(DataObject); + if (xlen < 0 || (uint64_t) xlen > (uint64_t) SIZE_MAX) { + error("* Data object is too large...ERROR"); + } + const size_t len = (size_t) xlen; const size_t BUF_SIZE = len*sizeof(Rbyte); if (verbose) Rprintf("* Data object size: %zu\n",len*sizeof(Rbyte)); if (verbose) Rprintf("* Start mapping object...OK\n"); @@ -157,8 +204,9 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje UNPROTECT(1); error("* Data object is empty...ERROR"); } - foo->hMapFile = CreateFileMapping(INVALID_HANDLE_VALUE, NULL, PAGE_READWRITE, 0, BUF_SIZE, pMN); - foo->hMapLength = CreateFileMapping(INVALID_HANDLE_VALUE, NULL, PAGE_READWRITE, 0, 256, pML); + foo->hMapFile = CreateFileMapping(INVALID_HANDLE_VALUE, NULL, PAGE_READWRITE, + (DWORD) (BUF_SIZE >> 32), (DWORD) (BUF_SIZE & 0xFFFFFFFFu), pMN); + foo->hMapLength = CreateFileMapping(INVALID_HANDLE_VALUE, NULL, PAGE_READWRITE, 0, SHM_META_SIZE, pML); if (foo->hMapFile == NULL || foo->hMapFile == INVALID_HANDLE_VALUE || foo->hMapLength == NULL || foo->hMapLength == INVALID_HANDLE_VALUE) { if (foo->hMapFile != NULL && foo->hMapFile != INVALID_HANDLE_VALUE) CloseHandle(foo->hMapFile); @@ -170,7 +218,7 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje } if (verbose) Rprintf("* Creating file maping...OK\n"); foo->lpMapAddress = (LPCTSTR) MapViewOfFile (foo->hMapFile, FILE_MAP_ALL_ACCESS, 0, 0, BUF_SIZE); - foo->lpMapLength = (LPCTSTR) MapViewOfFile (foo->hMapLength, FILE_MAP_ALL_ACCESS, 0, 0, 256); + foo->lpMapLength = (LPCTSTR) MapViewOfFile (foo->hMapLength, FILE_MAP_ALL_ACCESS, 0, 0, SHM_META_SIZE); if (foo->lpMapAddress == NULL || foo->lpMapLength == NULL) { if (foo->lpMapAddress != NULL) UnmapViewOfFile(foo->lpMapAddress); if (foo->lpMapLength != NULL) UnmapViewOfFile(foo->lpMapLength); @@ -183,21 +231,30 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje } if (verbose) Rprintf("* Map view file...OK\n"); // Seqlock publish: mark writing, copy payload, then publish size + ready. - struct SHM_META *metaW = (struct SHM_META *) foo->lpMapLength; - uint64_t gW = metaW->gen; - if (gW & 1ULL) gW++; // recover from a torn previous write (e.g. crashed writer) - metaW->gen = gW + 1ULL; // odd: write in progress - shm_release_fence(); + // Exactly one writer per name: an odd generation means another writer is + // mid-publish (or a previous one crashed) — fail loudly and recover with + // clearShared() instead of interleaving two payloads silently. + struct SHM_META *metaW = (struct SHM_META *) (void *) foo->lpMapLength; + uint32_t gW = shm_load_gen_acquire(&metaW->gen); + if (gW & 1u) { + UnmapViewOfFile(foo->lpMapAddress); + UnmapViewOfFile(foo->lpMapLength); + CloseHandle(foo->hMapFile); + CloseHandle(foo->hMapLength); + R_ClearExternalPtr(ext); + R_Free(foo); + UNPROTECT(1); + error("* Shared name is busy (concurrent writer?) — clearShared() to recover...ERROR"); + } + shm_store_gen_release(&metaW->gen, gW + 1u); // odd: write in progress CopyMemory((LPVOID)foo->lpMapAddress, RAW(DataObject), BUF_SIZE); - shm_release_fence(); - metaW->len = len; - shm_release_fence(); - metaW->gen = gW + 2ULL; // even: ready + shm_store_len_release(&metaW->len, len); + shm_store_gen_release(&metaW->gen, gW + 2u); // even: ready #else const char *pMN = CHAR(STRING_PTR_RO(MapObjectName)[0]); const char *pML = CHAR(STRING_PTR_RO(MapLengthName)[0]); - foo->STORAGE_ID = strdup(pMN); - foo->LENGTH_ID = strdup(pML); + foo->STORAGE_ID = shm_dup_string(pMN); + foo->LENGTH_ID = shm_dup_string(pML); if (foo->STORAGE_ID == NULL || foo->LENGTH_ID == NULL) { if (foo->STORAGE_ID != NULL) free(foo->STORAGE_ID); if (foo->LENGTH_ID != NULL) free(foo->LENGTH_ID); @@ -217,9 +274,11 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje foo->fd_addr = shm_open(foo->STORAGE_ID, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); foo->fd_length = shm_open(foo->LENGTH_ID, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); if (foo->fd_addr == -1 || foo->fd_length == -1) { - Rprintf("shm_open error, errno(%d): %s\n", errno, strerror(errno)); + int open_errno = errno; + Rprintf("shm_open error, errno(%d): %s\n", open_errno, strerror(open_errno)); if (foo->fd_addr != -1) close(foo->fd_addr); if (foo->fd_length != -1) close(foo->fd_length); + // No unlink here: pre-existing names may belong to a live owner. free(foo->STORAGE_ID); free(foo->LENGTH_ID); R_ClearExternalPtr(ext); @@ -234,8 +293,14 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje // object, so the stale objects are unlinked and fresh (truncatable) ones are // created. In-flight readers of the old objects keep reading the old // payload; only new opens see the new one. + // + // Orphan discipline: only objects this call effectively created (empty/new) + // are unlinked again on later failures; pre-existing live objects are never + // unlinked by us. Recover leftovers with clearShared(). struct stat st_addr, st_len; if (fstat(foo->fd_addr, &st_addr) == -1 || fstat(foo->fd_length, &st_len) == -1) { + int st_errno = errno; + Rprintf("stat error, errno(%d): %s\n", st_errno, strerror(st_errno)); close(foo->fd_addr); close(foo->fd_length); free(foo->STORAGE_ID); @@ -245,6 +310,8 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje UNPROTECT(1); error("* Stat shared memory object...ERROR"); } + bool fresh_addr = (st_addr.st_size == 0); + bool fresh_len = (st_len.st_size == 0); if (st_addr.st_size < 0 || (size_t) st_addr.st_size != BUF_SIZE || st_len.st_size < 0 || (size_t) st_len.st_size != SHM_META_SIZE) { close(foo->fd_addr); @@ -254,9 +321,11 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje foo->fd_addr = shm_open(foo->STORAGE_ID, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); foo->fd_length = shm_open(foo->LENGTH_ID, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); if (foo->fd_addr == -1 || foo->fd_length == -1) { - Rprintf("shm_open error, errno(%d): %s\n", errno, strerror(errno)); - if (foo->fd_addr != -1) close(foo->fd_addr); - if (foo->fd_length != -1) close(foo->fd_length); + int reopen_errno = errno; + Rprintf("shm_open error, errno(%d): %s\n", reopen_errno, strerror(reopen_errno)); + // Both names were just unlinked by us, so anything opened here is ours. + if (foo->fd_addr != -1) { close(foo->fd_addr); shm_unlink(foo->STORAGE_ID); } + if (foo->fd_length != -1) { close(foo->fd_length); shm_unlink(foo->LENGTH_ID); } free(foo->STORAGE_ID); free(foo->LENGTH_ID); R_ClearExternalPtr(ext); @@ -264,10 +333,16 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje UNPROTECT(1); error("* Recreating file mapping...ERROR"); } + // Recreated objects are ours: unlink them again if sizing fails below. + fresh_addr = true; + fresh_len = true; if (ftruncate(foo->fd_addr, BUF_SIZE) == -1 || ftruncate(foo->fd_length, SHM_META_SIZE) == -1) { - Rprintf("ftruncate error, errno(%d): %s\n", errno, strerror(errno)); + int ft_errno = errno; + Rprintf("ftruncate error, errno(%d): %s\n", ft_errno, strerror(ft_errno)); close(foo->fd_addr); close(foo->fd_length); + shm_unlink(foo->STORAGE_ID); + shm_unlink(foo->LENGTH_ID); free(foo->STORAGE_ID); free(foo->LENGTH_ID); R_ClearExternalPtr(ext); @@ -278,13 +353,16 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje } if (verbose) Rprintf("* Extend shared memory object...OK\n"); foo->addr = mmap(NULL, BUF_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, foo->fd_addr, 0); - foo->length = mmap(NULL, 256, PROT_READ | PROT_WRITE, MAP_SHARED, foo->fd_length, 0); + foo->length = mmap(NULL, SHM_META_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, foo->fd_length, 0); if (foo->addr == MAP_FAILED || foo->length == MAP_FAILED) { - if (foo->addr != MAP_FAILED) munmap(foo->addr, BUF_SIZE); - if (foo->length != MAP_FAILED) munmap(foo->length, 256); + int mm_errno = errno; + Rprintf("mmap error, errno(%d): %s\n", mm_errno, strerror(mm_errno)); + if (foo->addr != MAP_FAILED && foo->addr != NULL) munmap(foo->addr, BUF_SIZE); + if (foo->length != MAP_FAILED && foo->length != NULL) munmap(foo->length, SHM_META_SIZE); close(foo->fd_addr); close(foo->fd_length); - // Do not unlink here: the name may be shared; owner cleanup happens via finalizer. + if (fresh_addr) shm_unlink(foo->STORAGE_ID); + if (fresh_len) shm_unlink(foo->LENGTH_ID); foo->addr = NULL; foo->length = NULL; free(foo->STORAGE_ID); @@ -295,9 +373,17 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje error("* Map view file...ERROR"); } if (verbose) Rprintf("* Map view file...OK\n"); - if (close(foo->fd_addr) == -1 || close(foo->fd_length) == -1) { + int cerr_addr = close(foo->fd_addr); + int cerr_len = close(foo->fd_length); + foo->fd_addr = -1; + foo->fd_length = -1; + if (cerr_addr == -1 || cerr_len == -1) { + int cl_errno = errno; + Rprintf("close error, errno(%d): %s\n", cl_errno, strerror(cl_errno)); munmap(foo->addr, BUF_SIZE); - munmap(foo->length, 256); + munmap(foo->length, SHM_META_SIZE); + if (fresh_addr) shm_unlink(foo->STORAGE_ID); + if (fresh_len) shm_unlink(foo->LENGTH_ID); free(foo->STORAGE_ID); free(foo->LENGTH_ID); R_ClearExternalPtr(ext); @@ -305,19 +391,27 @@ SEXP createMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP DataObje UNPROTECT(1); error("* Closing file descriptors...ERROR"); } - foo->fd_addr = -1; - foo->fd_length = -1; // Seqlock publish: mark writing, copy payload, then publish size + ready. + // Exactly one writer per name: an odd generation means another writer is + // mid-publish (or a previous one crashed) — fail loudly and recover with + // clearShared() instead of interleaving two payloads silently. No unlink + // here: the names may belong to that live writer. struct SHM_META *meta = (struct SHM_META *) foo->length; - uint64_t g = meta->gen; - if (g & 1ULL) g++; // recover from a torn previous write (e.g. crashed writer) - meta->gen = g + 1ULL; // odd: write in progress - shm_release_fence(); + uint32_t g = shm_load_gen_acquire(&meta->gen); + if (g & 1u) { + munmap(foo->addr, BUF_SIZE); + munmap(foo->length, SHM_META_SIZE); + free(foo->STORAGE_ID); + free(foo->LENGTH_ID); + R_ClearExternalPtr(ext); + R_Free(foo); + UNPROTECT(1); + error("* Shared name is busy (concurrent writer?) — clearShared() to recover...ERROR"); + } + shm_store_gen_release(&meta->gen, g + 1u); // odd: write in progress memcpy(foo->addr, RAW(DataObject), BUF_SIZE); - shm_release_fence(); - meta->len = len; - shm_release_fence(); - meta->gen = g + 2ULL; // even: ready + shm_store_len_release(&meta->len, len); + shm_store_gen_release(&meta->gen, g + 2u); // even: ready #endif if (verbose) Rprintf("* Copy memory...OK\n"); R_RegisterCFinalizerEx(ext, map_finalizer, TRUE); @@ -334,6 +428,9 @@ SEXP getMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseArg) if (TYPEOF(MapObjectName) != STRSXP || LENGTH(MapObjectName) != 1) { error("Argument 'MapObjectName' must be of type character and length 1."); } + if (TYPEOF(MapLengthName) != STRSXP || LENGTH(MapLengthName) != 1) { + error("Argument 'MapLengthName' must be of type character and length 1."); + } if (!IS_BOOL(verboseArg)) { error("Argument 'verbose' must be TRUE or FALSE."); } @@ -348,52 +445,68 @@ SEXP getMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseArg) if (hMapFile != NULL && hMapFile != INVALID_HANDLE_VALUE) CloseHandle(hMapFile); if (hMapLength != NULL && hMapLength != INVALID_HANDLE_VALUE) CloseHandle(hMapLength); #else + // Open the header first, then the payload: with the writer's + // unlink(data)-then-unlink(meta) / create(data)-then-create(meta) order, + // every outcome is then a consistent (old,old)/(new,new) pair or a clean + // ENOENT — a stale-payload/new-header mix is unreachable (see below). const char *pMN = CHAR(STRING_PTR_RO(MapObjectName)[0]); const char *pML = CHAR(STRING_PTR_RO(MapLengthName)[0]); - int fd_addr = shm_open(pMN, O_RDONLY, S_IRUSR | S_IWUSR); - int fd_length = shm_open(pML, O_RDONLY, S_IRUSR | S_IWUSR); - if (fd_addr == -1 || fd_length == -1) { - if (fd_addr != -1) close(fd_addr); + int fd_length = shm_open(pML, O_RDONLY); + int fd_addr = shm_open(pMN, O_RDONLY); + if (fd_length == -1 || fd_addr == -1) { if (fd_length != -1) close(fd_length); + if (fd_addr != -1) close(fd_addr); #endif error("* Creating file mapping...ERROR"); } if (verbose) Rprintf("* Creating file maping...OK\n"); size_t len_a = 0; +#ifndef WIN32 size_t map_len = 0; - uint64_t gen_a = 0; +#endif + uint32_t gen_a = 0; #ifdef WIN32 LPCTSTR lpMapLength = (LPCTSTR) MapViewOfFile (hMapLength, FILE_MAP_ALL_ACCESS, 0, 0, SHM_META_SIZE); if (lpMapLength == NULL) { CloseHandle(hMapFile); CloseHandle(hMapLength); #else + // The length object must already hold a full header: a writer that crashed + // between shm_open and ftruncate leaves size 0, whose mapping would SIGBUS + // on dereference below. + struct stat st_len0; + if (fstat(fd_length, &st_len0) == -1 || + st_len0.st_size < (off_t) sizeof(struct SHM_META)) { + close(fd_length); + close(fd_addr); + error("* Map view file (length)...ERROR"); + } void *length = mmap(NULL, SHM_META_SIZE, PROT_READ, MAP_SHARED, fd_length, 0); if (length == MAP_FAILED) { - close(fd_addr); close(fd_length); -#endif + close(fd_addr); error("* Map view file (length)...ERROR"); } +#endif if (verbose) Rprintf("* Map view file (length)...OK\n"); // Seqlock: observe the header before the payload copy. An odd generation // means a writer is mid-publish, so fail fast instead of reading torn data. + // Generation is loaded first: the writer publishes len before the final + // generation bump, so gen-first narrows the retry window. struct SHM_META *metaR = NULL; #ifdef WIN32 - metaR = (struct SHM_META *) lpMapLength; - len_a = metaR->len; - gen_a = metaR->gen; - shm_acquire_fence(); - if (gen_a & 1ULL) { + metaR = (struct SHM_META *) (void *) lpMapLength; + gen_a = shm_load_gen_acquire(&metaR->gen); + len_a = shm_load_len_acquire(&metaR->len); + if (gen_a & 1u) { UnmapViewOfFile(lpMapLength); CloseHandle(hMapFile); CloseHandle(hMapLength); #else metaR = (struct SHM_META *) length; - len_a = metaR->len; - gen_a = metaR->gen; - shm_acquire_fence(); - if (gen_a & 1ULL) { + gen_a = shm_load_gen_acquire(&metaR->gen); + len_a = shm_load_len_acquire(&metaR->len); + if (gen_a & 1u) { munmap(length, SHM_META_SIZE); close(fd_addr); close(fd_length); @@ -424,6 +537,14 @@ SEXP getMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseArg) error("* Map view file (address)...ERROR"); } if ((size_t) addrstat.st_size < map_len) map_len = (size_t) addrstat.st_size; + // Fail fast when the clamp already proves a concurrent size change: this + // also avoids attempting a huge allocation for a garbage/torn length. + if (map_len != len_a*sizeof(Rbyte)) { + munmap(length, SHM_META_SIZE); + close(fd_addr); + close(fd_length); + error("* Shared data changed during read, please retry...ERROR"); + } // fd_length stays open: the header is re-read after the copy to validate. if (munmap(length, SHM_META_SIZE) == -1) { close(fd_addr); @@ -462,11 +583,11 @@ SEXP getMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseArg) if (verbose) Rprintf("* Copy map memory...OK\n"); #ifdef WIN32 - // Seqlock revalidation: the header must be unchanged since the pre-copy read. - shm_acquire_fence(); - size_t len_b = metaR->len; - uint64_t gen_b = metaR->gen; - bool torn_win = ((gen_a != gen_b) || (gen_a & 1ULL) || (len_a != len_b)); + // Seqlock revalidation: the header must be unchanged since the pre-copy + // read. Acquire loads are ordered after the payload copy above. + uint32_t gen_b = shm_load_gen_acquire(&metaR->gen); + size_t len_b = shm_load_len_acquire(&metaR->len); + bool torn_win = ((gen_a != gen_b) || (gen_b & 1u) || (len_a != len_b)); if (!UnmapViewOfFile(lpMapLength)) { UnmapViewOfFile(lpMapAddress); CloseHandle(hMapFile); @@ -517,10 +638,9 @@ SEXP getMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseArg) error("* Map view file (length)...ERROR"); } struct SHM_META *metaR2 = (struct SHM_META *) length2; - size_t len_b = metaR2->len; - uint64_t gen_b = metaR2->gen; - shm_acquire_fence(); - bool torn_posix = ((gen_a != gen_b) || (gen_a & 1ULL) || (len_a != len_b) || + uint32_t gen_b = shm_load_gen_acquire(&metaR2->gen); + size_t len_b = shm_load_len_acquire(&metaR2->len); + bool torn_posix = ((gen_a != gen_b) || (gen_b & 1u) || (len_a != len_b) || (map_len != len_a*sizeof(Rbyte))); if (munmap(length2, SHM_META_SIZE) == -1) { close(fd_length); @@ -549,6 +669,9 @@ SEXP unlinkMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseA if (TYPEOF(MapObjectName) != STRSXP || LENGTH(MapObjectName) != 1) { error("Argument 'MapObjectName' must be of type character and length 1."); } + if (TYPEOF(MapLengthName) != STRSXP || LENGTH(MapLengthName) != 1) { + error("Argument 'MapLengthName' must be of type character and length 1."); + } if (!IS_BOOL(verboseArg)) { error("Argument 'verbose' must be TRUE or FALSE."); } @@ -588,6 +711,9 @@ SEXP unlinkMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseA */ SEXP clearMappingObjectR (SEXP ext, SEXP verboseArg) { + if (TYPEOF(ext) != EXTPTRSXP) { + error("Argument 'x' must be an external pointer like the one returned by shareData()."); + } if (!IS_BOOL(verboseArg)) { error("Argument 'verbose' must be TRUE or FALSE."); } diff --git a/tests/test_kit.R b/tests/test_kit.R index 6e07131..a5dd187 100644 --- a/tests/test_kit.R +++ b/tests/test_kit.R @@ -1760,6 +1760,13 @@ rm(x1) # shareData # -------------------------------------------------------------------------------------------------- +# Reap leftovers from a previous crashed run so fixed shm names stay hermetic +# (POSIX shm survives crashes; on Windows clearShared is a harmless no-op). +invisible(tryCatch(clearShared("share1"), error=function(e) FALSE)) +invisible(tryCatch(clearShared("share-resize"), error=function(e) FALSE)) +invisible(tryCatch(clearShared("share-orphan"), error=function(e) FALSE)) +invisible(tryCatch(clearShared("share-noclear"), error=function(e) FALSE)) + x = tryCatch(shareData(mtcars,"share1"), error=function(err) { cat("Skipping shareData tests:", conditionMessage(err), "\n") NULL @@ -1780,6 +1787,8 @@ if (!is.null(x)) { rm(x) # Re-sharing the same name with a different payload size must work +# (cleared between shares so it also holds on Windows, where a live +# mapping cannot grow). x = tryCatch(shareData(1:10, "share-resize"), error=function(err) { cat("Skipping shareData resize tests:", conditionMessage(err), "\n") NULL @@ -1793,9 +1802,20 @@ if (!is.null(x)) { check("0022.009", getData("share-resize"), 1:1000) check("0022.010", getData("share-resize"), 1:1000) check("0022.011", clearData(x), TRUE) + rm(x) } -rm(x) +if (.Platform$OS.type != "windows") { + # POSIX only: re-sharing without clearing recreates on size change. The old + # handle is kept alive so its finalizer cannot unlink the new names early. + yo = shareData(1:5, "share-noclear") + yn = shareData(1:2000, "share-noclear") + check("0022.017", getData("share-noclear"), 1:2000) + check("0022.018", getData("share-noclear"), 1:2000) + check("0022.019", clearData(yn), TRUE) + check("0022.020", clearData(yo), TRUE) + rm(yo, yn) +} # clearShared() reaps a live segment by name, without the owner handle x = tryCatch(shareData(mtcars, "share-orphan"), error=function(err) { @@ -1804,11 +1824,21 @@ x = tryCatch(shareData(mtcars, "share-orphan"), error=function(err) { }) if (!is.null(x)) { - check("0022.012", getData("share-orphan"), mtcars) - check("0022.013", clearShared("share-orphan"), TRUE) - check("0022.014", tryCatch({getData("share-orphan"); "unexpected-ok"}, error=function(e) "expected-error"), "expected-error") - check("0022.015", clearShared("share-orphan"), FALSE) - check("0022.016", clearData(x), TRUE) + if (.Platform$OS.type == "windows") { + # clearShared() is a documented no-op on Windows: nothing unlinked, + # reads keep working until the owner clears. + check("0022.012", getData("share-orphan"), mtcars) + check("0022.013", clearShared("share-orphan"), FALSE) + check("0022.014", getData("share-orphan"), mtcars) + check("0022.015", clearData(x), TRUE) + check("0022.016", clearShared("share-orphan"), FALSE) + } else { + check("0022.012", getData("share-orphan"), mtcars) + check("0022.013", clearShared("share-orphan"), TRUE) + check("0022.014", tryCatch({getData("share-orphan"); "unexpected-ok"}, error=function(e) "expected-error"), "expected-error") + check("0022.015", clearShared("share-orphan"), FALSE) + check("0022.016", clearData(x), TRUE) + } } rm(x) From fa075a8cdbe899f5c45c95d3cd1236677dc9bf94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dawid=20Budzy=C5=84ski?= Date: Tue, 8 Sep 2026 20:44:19 +0200 Subject: [PATCH 4/4] fix(share): restore shared ifdef tail (Windows brace fix), 3-arg shm_open - The fstat guard made the POSIX meta-map block self-contained, orphaning the shared closing brace of WIN32's NULL check, which broke the Windows build (nested-function cascade). Shared-tail structure restored. - shm_open() needs its mode argument even without O_CREAT: glibc declares three parameters (macOS tolerates two). Fixes Linux build. - Guard shm_dup_string to POSIX (unused-function warning on Windows would fail error_on=warning CI). --- src/share.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/share.c b/src/share.c index b533099..14a33e3 100644 --- a/src/share.c +++ b/src/share.c @@ -80,12 +80,15 @@ static void shm_store_len_release(size_t *p, size_t v) { *p = v; } #endif // strdup without feature-test-macro dependence (strict-ISO safe). +// POSIX-only helper (the Windows branch keeps no name copies). +#ifndef WIN32 static char *shm_dup_string(const char *s) { size_t n = strlen(s) + 1; char *p = (char *) malloc(n); if (p != NULL) memcpy(p, s, n); return p; } +#endif /* * Structure to hold Length and Address @@ -451,8 +454,10 @@ SEXP getMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseArg) // ENOENT — a stale-payload/new-header mix is unreachable (see below). const char *pMN = CHAR(STRING_PTR_RO(MapObjectName)[0]); const char *pML = CHAR(STRING_PTR_RO(MapLengthName)[0]); - int fd_length = shm_open(pML, O_RDONLY); - int fd_addr = shm_open(pMN, O_RDONLY); + // NOTE: the mode argument is required even without O_CREAT: glibc + // declares shm_open() with three parameters (macOS tolerates two). + int fd_length = shm_open(pML, O_RDONLY, S_IRUSR | S_IWUSR); + int fd_addr = shm_open(pMN, O_RDONLY, S_IRUSR | S_IWUSR); if (fd_length == -1 || fd_addr == -1) { if (fd_length != -1) close(fd_length); if (fd_addr != -1) close(fd_addr); @@ -485,9 +490,9 @@ SEXP getMappingObjectR (SEXP MapObjectName, SEXP MapLengthName, SEXP verboseArg) if (length == MAP_FAILED) { close(fd_length); close(fd_addr); +#endif error("* Map view file (length)...ERROR"); } -#endif if (verbose) Rprintf("* Map view file (length)...OK\n"); // Seqlock: observe the header before the payload copy. An odd generation // means a writer is mid-publish, so fail fast instead of reading torn data.