fix(fileutils): prevent stack buffer overflow in AbsPath#16
Conversation
getcwd + path concatenation and the absolute-path strcpy wrote into fixed PATH_MAX buffers with no bounds check, allowing a stack overflow for long paths. Build the working buffer with bounded snprintf, bail out safely (empty result) when the path cannot fit, and pass the output buffer size explicitly. Add regression tests for overlong relative and absolute paths.
|
Hi @Rakdos8 Thank you for the PR and apologies for the late response. We are still in the process of sorting out some details around contribution guidelines and PR templates. This is a good catch! There might even be a possibility to simply use |
|
An AI Pull Request if I've ever seen one, looked over most of them from this user and they all smell of AI slop. @CCP-Aporia I hope Fenris are going to add in some strong PR standards. |
|
We care less about the tools used and more about the quality of code - low effort/ low quality PRs will always be rejected. We are working internally on the best way for us to voice our expectations when it comes to quality, and perhaps we are partially to blame if we have low effort PRs open at the moment. As for this specific pull request; Lets keep PR discussion focused on the contents of the diff going forward - fully unpacking those expectations here would be a can of worms. I will mention we are looking into ways for both Fenris and community developers to have proper strategic discussion on carbon as a whole. Please stay tuned |
Problem
AbsPath(POSIX path, used byCcpGetAbsolutePath) wrote intochar[PATH_MAX]buffers with no bounds checking.getcwd(src, …)followed bystrcat(src, path)overflows the stack whencwd + pathexceedsPATH_MAX(1024 onmacOS);
src[len] = '/'; src[len+1] = 0;writes out of bounds whenlen == PATH_MAX-1; andstrcpy(src, path)(absolute path) plus
strcpy(realPath, path)(ongetcwdfailure) are unbounded too. This is reachable on macOS (aprimary CI target) via
CcpGetAbsolutePath, so it is a high-severity stack overflow driven entirely by path length.Fix
AbsPathnow receives the output buffer size (size_t realPathSize) — there is a single internal caller.srcisbuilt with a bounded
snprintfand the function returns an empty string when the combined path does not fit. Thegetcwdfailure path usesstrncpy_s(…, _TRUNCATE)instead ofstrcpy, and there is an explicit guard beforewriting into
realPath.CcpGetAbsolutePathdrops a dead line (astrcpy_sthat was immediately overwritten) andits unused variable, and now passes
sizeof(buffer). Added#include <cstdio>in the POSIX block. Contract: when apath is too long to fit, the function returns an empty string instead of overflowing.
Tests
Added two regression tests in
tests/CcpFileUtils.cpp(GoogleTest,#if !_WIN32):CcpGetAbsolutePathHandlesOverlongRelativePathSafelyandCcpGetAbsolutePathHandlesOverlongAbsolutePathSafely, bothusing 64 KB paths and expecting an empty result with no crash. Existing path-resolution tests are unaffected. Reviewed
manually; not compiled locally as the vcpkg toolchain is unavailable in this environment.
Scope
POSIX branch only — the Windows path uses
GetFullPathNameWand is unchanged. No public API change.