Conversation
This change introduces a wrapper for the `prctl` system call to support naming Virtual Memory Areas (VMAs) using `PR_SET_VMA_ANON_NAME`. When the underlying kernel does not support this feature, the wrapper provides a fallback mechanism that writes the VMA information to a temporary file (`/tmp/cobalt_vma_tags_<pid>.txt`). This allows for memory debugging and attribution even on systems without the latest kernel features. A new NPLB test, `PosixPrctlTest.SetVmaAnonName`, has been added to verify both the direct `prctl` call and the fallback mechanism. The `prctl` wrapper is now used in various parts of the codebase, including the partition allocator and persistent memory allocator, under the `IS_STARBOARD` build flag. Bug: 443798603
This change introduces a wrapper for the `prctl` system call to support naming Virtual Memory Areas (VMAs) using `PR_SET_VMA_ANON_NAME`. When the underlying kernel does not support this feature, the wrapper provides a fallback mechanism that writes the VMA information to a temporary file (`/tmp/cobalt_vma_tags_<pid>.txt`). This allows for memory debugging and attribution even on systems without the latest kernel features. A new NPLB test, `PosixPrctlTest.SetVmaAnonName`, has been added to verify both the direct `prctl` call and the fallback mechanism. The `prctl` wrapper is now used in various parts of the codebase, including the partition allocator and persistent memory allocator, under the `IS_STARBOARD` build flag. Bug: 443798603
…temps, and respect TMPDIR.
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements support for PR_SET_VMA and PR_SET_VMA_ANON_NAME on the Starboard platform, updating memory management components and the prctl ABI wrapper to include a file-based fallback mechanism for VMA naming. Feedback identifies a potential undefined behavior risk when handling NULL pointers in snprintf, suggests improved error checking for system calls like snprintf and write, and recommends using mmap instead of malloc in tests to ensure reliable VMA tagging verification.
| int len = snprintf(buf, sizeof(buf), "0x%lx 0x%lx %s\n", arg3, | ||
| arg3 + arg4, reinterpret_cast<const char*>(arg5)); |
There was a problem hiding this comment.
The arg5 parameter for PR_SET_VMA_ANON_NAME can be NULL to clear a previously set name. Passing a NULL pointer to snprintf with the %s format specifier is undefined behavior and can lead to crashes on some platforms. A check should be added to handle this case safely.
int len = snprintf(buf, sizeof(buf), "0x%lx 0x%lx %s\n", arg3,
arg3 + arg4, arg5 ? reinterpret_cast<const char*>(arg5) : "");| snprintf(path_template, sizeof(path_template), "%s/%s_XXXXXX.txt", | ||
| GetTempDir(), kVmaTagsFileNamePrefix); |
There was a problem hiding this comment.
The return value of snprintf should be checked to ensure the path was not truncated. If truncation occurs, the XXXXXX placeholder required by mkstemps might be incomplete, causing the file creation to fail.
int written = snprintf(path_template, sizeof(path_template), "%s/%s_XXXXXX.txt",
GetTempDir(), kVmaTagsFileNamePrefix);
if (written < 0 || written >= static_cast<int>(sizeof(path_template))) {
return -1;
}| if (len > 0) { | ||
| size_t write_len = | ||
| std::min(static_cast<size_t>(len), sizeof(buf) - 1); | ||
| write(fd, buf, write_len); |
There was a problem hiding this comment.
The return value of write is ignored. It is recommended to check the return value to handle potential errors and to avoid compiler warnings about unused results.
if (write(fd, buf, write_len) < 0) {
close(fd);
return -1;
}| #include <string.h> | ||
| #include <sys/prctl.h> | ||
| #include <sys/resource.h> | ||
| #include <sys/types.h> |
There was a problem hiding this comment.
Include <sys/mman.h> to provide definitions for mmap and munmap, which are better suited for testing PR_SET_VMA than malloc.
#include <sys/mman.h>
#include <sys/types.h>| // or by using the fallback mechanism of writing to a file. | ||
| TEST(PosixPrctlTest, SetVmaAnonName) { | ||
| const size_t kMapSize = 4096; | ||
| void* p = malloc(kMapSize); |
There was a problem hiding this comment.
PR_SET_VMA is designed to work with anonymous memory mappings. Memory from malloc might not be a distinct anonymous mapping (e.g., it could be part of the heap's brk segment), which can lead to prctl returning EINVAL. Using mmap ensures a valid target for the test.
void* p = mmap(nullptr, kMapSize, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
ASSERT_NE(p, MAP_FAILED);| "the kernel, fallback was used."; | ||
| } | ||
|
|
||
| free(p); |
There was a problem hiding this comment.
Use munmap to release memory allocated with mmap.
munmap(p, kMapSize);… bugs. - Fix unsafe variadic implementation in prctl() by using a switch to safely extract arguments from va_list. - Integrate robust VMA tags fallback logic into __abi_wrap_prctl. - Use a self-contained fallback in posix_prctl_test.cc to ensure correctness across all build configurations. - Use std::mutex for thread safety in the VMA tags fallback. - Modernize code by replacing all C-style casts with static_cast and reinterpret_cast, adhering to the Chromium C++ Style Guide. - Add PosixPrctlVariadicTest to verify variadic handling and thread safety.
Awallky
force-pushed
the
wrap_prtcl_sandbox
branch
from
aceec3f to
2653447
Compare
2 participants
Bug: 443798603