Improve secure CRT portability by detecting and implementing missing definitions#10442
Improve secure CRT portability by detecting and implementing missing definitions#10442quesswho wants to merge 12 commits intoshader-slang:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a CMake module that probes for Microsoft-style secure CRT functions and exposes successful probes as Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/core/slang-secure-crt.h (1)
82-95:⚠️ Potential issue | 🟠 Major
__attribute__((format))is not portable to MSVC.The
__attribute__((format(printf, ...)))syntax is GCC/Clang-specific and will cause compilation errors on MSVC.Consider using a compiler-detection macro:
🛠️ Suggested fix
`#ifndef` HAVE_SPRINTF_S +#if defined(__GNUC__) || defined(__clang__) __attribute__((format(printf, 3, 4))) inline int sprintf_s( +#else +inline int sprintf_s( +#endif char* buffer, size_t sizeOfBuffer, const char* format,Apply the same pattern to
swprintf_s(lines 97-109), though GCC'swprintfformat checking is noted as incomplete in the code comment.
🧹 Nitpick comments (2)
source/core/slang-secure-crt.h (1)
64-80: Inconsistent null-check between Cygwin and non-Cygwin paths.The Cygwin path (lines 68-75) checks
if (str)for null before dereferencing, but the non-Cygwin path (line 77) passesstrdirectly tostrnlenwithout a null check. Whilestrnlentypically handles null gracefully on most platforms, adding a consistent null check would be safer.cmake/CheckSecureCRT.cmake (1)
15-47: Consider adding a probe for_stricmpor a platform guard.The probes cover all the secure CRT functions with
HAVE_*guards inslang-secure-crt.h, but there's no probe for_stricmp. This creates a gap where the#ifndef _stricmpguard in the header (lines 24-26) doesn't have a corresponding CMake-detected macro.If the
_stricmpmapping inslang-secure-crt.his fixed with a platform guard (e.g.,#if !defined(_MSC_VER)), then no probe is needed here. However, if you prefer consistency with the other functions, you could add:slang_probe_symbol_call(HAVE_STRICMP "_stricmp(\"a\", \"b\")" "string.h")This is a coordination note with the fix needed in
slang-secure-crt.h.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a51d9aed-e3d8-4a13-b2cf-c5f9a666a1f6
📒 Files selected for processing (3)
CMakeLists.txtcmake/CheckSecureCRT.cmakesource/core/slang-secure-crt.h
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
bot
dismissed
their stale review
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
bot
dismissed
their stale review
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/core/slang-secure-crt.h (1)
26-39:⚠️ Potential issue | 🟡 MinorInitialize the output pointer on every
fopen_sfailure path.When
fileNameormodeis null, the function returnsEINVALwithout clearing*f, allowing callers to observe a staleFILE*value in this failure case. Initialize*fbefore the parameter checks to ensure all failure paths properly clear the pointer.Proposed fix
inline int fopen_s(FILE** f, const char* fileName, const char* mode) { - if (f == nullptr || fileName == nullptr || mode == nullptr) + if (f == nullptr) + { + return EINVAL; + } + *f = nullptr; + if (fileName == nullptr || mode == nullptr) { return EINVAL; } *f = fopen(fileName, mode);
♻️ Duplicate comments (1)
source/core/slang-secure-crt.h (1)
55-76:⚠️ Potential issue | 🔴 CriticalRestore
_ssemantics for the bounded-length helpers.Line 58 ignores the caller-provided bound entirely, and Line 75 still forwards to
strnlen(...), which does not acceptnullptr. That meanswcsnlen_scan read past an unterminated buffer, whilestrnlen_scan hit UB/crash on null input instead of returning0.Proposed fix
`#ifndef` HAVE_WCSNLEN_S -inline size_t wcsnlen_s(const wchar_t* str, size_t /*numberofElements*/) +inline size_t wcsnlen_s(const wchar_t* str, size_t numberOfElements) { - return wcslen(str); + if (!str) + return 0; + size_t len = 0; + while (len < numberOfElements && str[len] != L'\0') + ++len; + return len; } `#endif` // HAVE_WCSNLEN_S `#ifndef` HAVE_STRNLEN_S inline size_t strnlen_s(const char* str, size_t numberOfElements) { -#if defined(__CYGWIN__) - const char* cur = str; - if (str) - { - const char* const end = str + numberOfElements; - while (*cur && cur < end) - cur++; - } - return size_t(cur - str); -#else - return strnlen(str, numberOfElements); -#endif + if (!str) + return 0; + size_t len = 0; + while (len < numberOfElements && str[len] != '\0') + ++len; + return len; } `#endif` // HAVE_STRNLEN_S
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 93ad447f-9b78-4454-8df5-0d22cf78003c
📒 Files selected for processing (1)
source/core/slang-secure-crt.h
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmake/CheckSecureCRT.cmake (1)
46-48: Add trailing newline at end of file.The file is missing a trailing newline. Many tools and
git diffwill flag this.Proposed fix
slang_probe_symbol_call(HAVE_STRNCPY_S "char b[1]; strncpy_s(b, 1, \"x\", 1)" "string.h") +source/core/slang-secure-crt.h (2)
109-140: Copy function fallbacks ignore destination buffer size.The
wcscpy_s,strcpy_s,wcsncpy_s, andstrncpy_sfallbacks comment outnumberOfElementsand use the unsafe underlying functions without bounds checking. This creates a contract mismatch where callers expect bounds validation but receive none.Similarly,
memcpy_s(line 16) andfread_s(line 50) useassert()for bounds checking which is disabled in release builds.Consider adding explicit bounds checks (not just assert) for defense-in-depth, or at minimum document that these fallbacks assume caller-validated sizes.
142-143: Add trailing newline at end of file.Proposed fix
`#endif` // SLANG_CORE_SECURE_CRT_H `#endif` // _MSC_VER +
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a1afb5e9-a29e-4297-8ebf-697d8e622dff
📒 Files selected for processing (3)
cmake/CheckSecureCRT.cmakecmake/SlangTarget.cmakesource/core/slang-secure-crt.h
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
bot
dismissed
their stale review
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
bot
dismissed
their stale review
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
bot
dismissed
their stale review
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
jkwak-work
left a comment
jkwak-work
left a comment
There was a problem hiding this comment.
I am not familiar with cmake syntax used in thie PR
But it seems harmless and helpful for MingW
quesswho
force-pushed
the
feature/checkcrt
branch
from
2eb7ea0 to
d74e522
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/core/slang-secure-crt.h (1)
37-48:⚠️ Potential issue | 🔴 CriticalInitialize
*fto nullptr on all return paths, including parameter validation failures.When parameter validation fails at line 39, the function returns EINVAL without setting
*f, leaving the output parameter uninitialized. This breaks the fopen_s contract that callers rely on:*f == nullptrshould be guaranteed on any non-zero return. Some callers in the codebase (e.g.,tools/slang-generate/main.cpp) do not pre-initialize their FILE* pointer, making them vulnerable to reading garbage values on validation failures.The fix is straightforward: set
*f = nullptr;before the parameter validation check (when f is valid).Proposed fix
inline int fopen_s(FILE** f, const char* fileName, const char* mode) { + if (f) + *f = nullptr; if (f == nullptr || fileName == nullptr || mode == nullptr) { return EINVAL; } *f = fopen(fileName, mode);
♻️ Duplicate comments (1)
source/core/slang-secure-crt.h (1)
1-16:⚠️ Potential issue | 🔴 CriticalMinGW still takes the wrong compatibility path.
Line 1 makes this whole block active on MinGW, and Lines 14-16 then redefine
_TRUNCATE/_stricmpeven though MinGW already provides Windows-flavored CRT declarations there. That can reintroduce the same redefinition/shadowing failures this PR is trying to remove. Please separate the “enable fallbacks on MinGW” guard from the POSIX alias section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e93edaac-241c-4639-93fd-d41044ed1145
📒 Files selected for processing (1)
source/core/slang-secure-crt.h
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
quesswho
changed the title
github-actions
bot
dismissed
their stale review
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
|
I've pushed additional commits that improves the |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
bot
dismissed
their stale review
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
bot
dismissed
their stale review
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
bot
dismissed
their stale review
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
bot
dismissed
their stale review
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… removed unnecessary check
| @@ -0,0 +1,48 @@ | |||
| # Probe for secure CRT functions that may not be available on all platforms, | |||
There was a problem hiding this comment.
nit: Trailing whitespace at end of lines 1 and 5 (after the commas). The project's formatting script may catch this, but worth cleaning up.
| # Probe for secure CRT functions that may not be available on all platforms, | |
| # Probe for secure CRT functions that may not be available on all platforms, |
| # | ||
| if(SLANG_SECURE_CRT_DEFINES) | ||
| target_compile_definitions( | ||
| ${target} | ||
| PRIVATE ${SLANG_SECURE_CRT_DEFINES} | ||
| ) | ||
| endif() | ||
|
|
||
| # | ||
| # Precompiled headers (PCH) |
There was a problem hiding this comment.
This propagates the HAVE_* defines to every target built through slang_add_target, even those that never include slang-secure-crt.h. That's harmless (the defines are simply unused), but an alternative approach would be to create a small interface library (e.g., add_library(slang-secure-crt INTERFACE) with the defines) and link it only from the targets that actually include the header.
Not blocking — the current approach is simple and correct. Just a potential future cleanup.
| @@ -1,4 +1,4 @@ | |||
| #ifndef _WIN32 | |||
| #if !defined(_WIN32) || defined(__MINGW32__) || defined(__CYGWIN__) | |||
There was a problem hiding this comment.
Good change. On MinGW, _WIN32 is defined but the secure CRT functions may be missing, so the old #ifndef _WIN32 guard was too broad.
One consideration: for non-CMake builds on MinGW/Cygwin, none of the HAVE_* macros will be defined, so all fallback implementations will be provided. If MinGW's CRT headers already declare some of these functions (with potentially different signatures like errno_t vs int), this could cause compilation errors.
It might be worth adding a brief comment near the top explaining the dependency on the CMake build system for the HAVE_* macros, e.g.:
// NOTE: The HAVE_<func> macros below are set by cmake/CheckSecureCRT.cmake.
// When building without CMake, all fallback implementations are provided.
| #endif | ||
| #if !defined(_stricmp) && !defined(__MINGW32__) | ||
| #define _stricmp strcasecmp |
There was a problem hiding this comment.
Sensible guard — MinGW provides _stricmp natively through its CRT headers. The !defined(_stricmp) check is a nice defensive touch for other compilers that might define it as a macro.
Verified: the only usage of _stricmp in the codebase is in slang-string.h inside an #ifdef _MSC_VER block, so this change has no impact on existing behavior.
|
|
||
| #ifndef HAVE_MEMCPY_S | ||
| // https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/memcpy-s-wmemcpy-s | ||
| inline int memcpy_s(void* dest, size_t destSize, const void* src, size_t count) | ||
| { | ||
| assert(destSize >= count); | ||
| if (count == 0) | ||
| return 0; | ||
| if (dest == nullptr) | ||
| { | ||
| errno = EINVAL; | ||
| return EINVAL; | ||
| } | ||
| if (src == nullptr || destSize < count) | ||
| { | ||
| memset(dest, 0, destSize); | ||
| if (src == nullptr) | ||
| { | ||
| errno = EINVAL; | ||
| return EINVAL; | ||
| } |
There was a problem hiding this comment.
The return type changed from void to int, and the implementation now properly matches MSVC semantics (null checks, zeroing dest on failure, returning appropriate error codes). I verified that the only call site (memcpy_s is currently unused in .cpp files) won't be affected, and memcpy_s isn't called from any .cpp files in the codebase.
One minor note: MSVC's memcpy_s also validates for overlapping buffers and invokes the invalid parameter handler rather than simply returning an error code. The comment references the MSVC docs which is good — readers can understand the semantic differences.
| // https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fread-s | ||
| inline size_t fread_s( | ||
| void* buffer, | ||
| [[maybe_unused]] size_t bufferSize, | ||
| size_t bufferSize, | ||
| size_t elementSize, | ||
| size_t count, | ||
| FILE* stream) | ||
| { | ||
| assert(bufferSize >= elementSize * count); | ||
| if (elementSize == 0 || count == 0) | ||
| return 0; | ||
| if (buffer == nullptr || stream == nullptr) | ||
| { | ||
| errno = EINVAL; | ||
| return 0; | ||
| } | ||
| if (count > bufferSize / elementSize) | ||
| { |
There was a problem hiding this comment.
The bounds check count > bufferSize / elementSize is a nice pattern — it avoids the elementSize * count multiplication that could overflow on 32-bit platforms.
One edge case worth considering: MSVC's fread_s documentation says the buffer contents are indeterminate on error (some implementations zero it). This implementation just returns 0 without touching the buffer, which is fine and matches the documented "returns 0" behavior. Good pragmatic choice.
| #ifndef HAVE_STRNLEN_S | ||
| // https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/strnlen-strnlen-s | ||
| inline size_t strnlen_s(const char* str, size_t numberOfElements) | ||
| { | ||
| #if defined(__CYGWIN__) | ||
| const char* cur = str; | ||
| if (str) | ||
| { | ||
| const char* const end = str + numberOfElements; | ||
| while (*cur && cur < end) | ||
| cur++; | ||
| } | ||
| return size_t(cur - str); | ||
| #else | ||
| return strnlen(str, numberOfElements); | ||
| #endif | ||
| if (str == nullptr) | ||
| return 0; | ||
| size_t count = 0; | ||
| while (count < numberOfElements && str[count] != '\0') | ||
| count++; | ||
| return count; | ||
| } | ||
| #endif // HAVE_STRNLEN_S | ||
|
|
There was a problem hiding this comment.
The sprintf_s comment about diverging from MSVC behavior (truncation via vsnprintf vs. invalid parameter handler) is excellent documentation. This is a pragmatic and correct choice for the fallback — callers in the codebase don't rely on the buffer being invalidated on overflow.
Good that the __GNUC__ guard was added for the format attribute — this makes it correct for MSVC builds (where this code path won't be reached anyway, but it's good hygiene).
| return EINVAL; | ||
| } | ||
| if (dest_size == 0) | ||
| { | ||
| errno = ERANGE; | ||
| return ERANGE; | ||
| } | ||
| if (src == nullptr) | ||
| { | ||
| dest[0] = '\0'; | ||
| errno = EINVAL; | ||
| return EINVAL; | ||
| } | ||
| size_t i = 0; | ||
| while (i < dest_size - 1 && src[i] != '\0') | ||
| { | ||
| dest[i] = src[i]; | ||
| i++; | ||
| } | ||
| dest[i] = '\0'; | ||
| if (src[i] != '\0') | ||
| { | ||
| dest[0] = '\0'; | ||
| errno = ERANGE; | ||
| return ERANGE; | ||
| } | ||
| return 0; | ||
| } | ||
| #endif // HAVE_STRCPY_S | ||
|
|
||
| inline void wcsncpy_s( | ||
| wchar_t* strDestination, | ||
| size_t /*numberOfElements*/, | ||
| #ifndef HAVE_WCSNCPY_S | ||
| // https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/strncpy-s-strncpy-s-l-wcsncpy-s-wcsncpy-s-l-mbsncpy-s-mbsncpy-s-l?view=msvc-170 | ||
| inline int wcsncpy_s( | ||
| wchar_t* strDest, | ||
| size_t numberOfElements, | ||
| const wchar_t* strSource, | ||
| size_t count) |
There was a problem hiding this comment.
I traced through the logic for several edge cases and it's correct:
count == _TRUNCATE: copies up tonumberOfElements-1, returnsSTRUNCATEif truncated ✓count < numberOfElements-1and source longer thancount: copies exactlycountchars, null-terminates, returns 0 ✓count >= numberOfElementsand source is long: detects overflow, zeros dest, returnsERANGE✓count == 0: null-terminates dest immediately, returns 0 ✓strSource == nullptr: null-terminates dest, returnsEINVAL✓
Well done on the correctness here — the original wcsncpy/strncpy fallback didn't null-terminate and had no error handling at all.
There was a problem hiding this comment.
PR Review: Improve secure CRT portability. Overall: Looks good. Well-structured improvement to MinGW/Cygwin portability with better secure CRT fallback implementations. What this PR does: (1) CMake probing via check_source_compiles to detect available secure CRT functions. (2) Per-function HAVE_* guards replacing the single ifndef _WIN32 guard. (3) Improved implementations matching MSVC semantics. Correctness verified for return type changes, strncpy_s/wcsncpy_s edge cases, CMake PARENT_SCOPE propagation, and stricmp guard. Suggestions (non-blocking, see inline comments): trailing whitespace in cmake file, add comment about CMake dependency for HAVE* macros, consider interface library for scoping defines. Missing: PR needs a pr: non-breaking label.
2 participants
Partially fixes #5843
Problem
slang-secure-crt.hpreviously defined implementations of secure CRT functions (e.g.fread_s,sprintf_s,strncpy_s) only on non-Windows platforms using a single#ifndef _WIN32guard. This caused missing declaration errors on Windows compilers that do not fully implement the secure CRT functions, such as MinGW or Cygwin.Solution
Added
cmake/CheckSecureCRT.cmake, which probes for each secure CRT function individually by attempting a real call with matching argument types usingcheck_source_compiles. This correctly detects inline and attributed declarations thatcheck_symbol_exists/check_function_existsmiss, without requiring platform-specific macro checks for each function.Each function in
slang-secure-crt.his now guarded by its ownHAVE_<FUNC>macro rather than the single_WIN32guard, so fallbacks are only defined when the platform lacks them.Improvements
The
_simplementations have been updated to properly match MSVC semantics,including null pointer checks, bounds checking, correct error codes, and null termination. Each implementation references the corresponding Microsoft documentation.
Testing
I verified a clean build on MSVC, GCC/G++. Previous redefinition/missing errors on MinGW (MSYS2 mingw64) are now gone. On Mingw there are still some compile errors related to uuids and the dxc lib, hence #5843 is not fully resolved.
As for the
_simplementations, I wrote a separate program that verifies the functionality and tests the error output against the MSVC documentation.