Skip to content

Commit 23d4122

Browse files
authored
Fix intermittent CA test failure on Windows CI when TEMP is unset (#3161)
### Description of changes: CA tests (`CATest.DatabaseWithRevokedEntryBasicTimestamp`, `CATest.CustomEndDate`, etc.) fail intermittently on the `windows-ltsc2022_sde-x86_64` CI job due to two issues with `WIN32_rename`: 1. **Temp files landing in `C:\Windows\`**: `GetTempPathA` falls back to the Windows directory when `TMP`, `TEMP`, and `USERPROFILE` are all unset — which is the case when running as SYSTEM in our Docker-based CI. This introduces a `GetSafeTempPathA` wrapper that detects the fallback and redirects to `C:\Windows\Temp\` instead. 2. **NTFS pending-delete race in `WIN32_rename`**: The `DeleteFile` + `MoveFile` workaround for `ERROR_ALREADY_EXISTS` is racy. NTFS defers file removal until all handles are closed, so `DeleteFile` can return success while the file is still visible. The subsequent `MoveFile` fails with `ERROR_ALREADY_EXISTS` (183), which wasn't in the retryable error set, so the retry budget was never used. Replaced with `MoveFileEx` using `MOVEFILE_REPLACE_EXISTING`, which atomically replaces the target — matching POSIX `rename()` semantics. ### Call-outs: `CATest::TearDown` was missing cleanup of `.new` and `.attr.new` files. These get left behind when a test fails between `SaveIndex` and `RotateIndex`. Added while in here. See failure [here](https://github.com/aws/aws-lc/actions/runs/24354082397/job/71116230692?pr=2934#step:4:3018). ### Testing: Existing CA tests cover this path. The fix should be verified by the `windows-ltsc2022_sde-x86_64` job no longer failing intermittently. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
1 parent be77a8a commit 23d4122

3 files changed

Lines changed: 46 additions & 10 deletions

File tree

crypto/test/test_util.cc

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -182,11 +182,42 @@ bool PEM_to_DER(const char *pem_str, uint8_t **out_der, long *out_der_len) {
182182
}
183183

184184
#if defined(OPENSSL_WINDOWS)
185+
// GetTempPathA falls back to the Windows directory (e.g. C:\Windows\) when the
186+
// TMP, TEMP, and USERPROFILE environment variables are all unset. This commonly
187+
// happens when running as SYSTEM in Docker containers or CI agents. The Windows
188+
// directory has special protections that cause file rename operations to fail
189+
// intermittently. Detect this case and redirect to C:\Windows\Temp\ instead.
190+
static DWORD GetSafeTempPathA(DWORD nBufferLength, LPSTR lpBuffer) {
191+
DWORD ret = GetTempPathA(nBufferLength, lpBuffer);
192+
if (ret == 0 || ret >= nBufferLength) {
193+
return ret;
194+
}
195+
char win_dir[PATH_MAX];
196+
UINT win_len = GetWindowsDirectoryA(win_dir, sizeof(win_dir));
197+
if (win_len == 0 || win_len >= sizeof(win_dir)) {
198+
return ret;
199+
}
200+
// Append trailing backslash to match GetTempPathA's format for comparison.
201+
if (win_len + 1 >= sizeof(win_dir)) {
202+
return ret;
203+
}
204+
win_dir[win_len] = '\\';
205+
win_dir[win_len + 1] = '\0';
206+
if (_stricmp(lpBuffer, win_dir) == 0) {
207+
int written = snprintf(lpBuffer, nBufferLength, "%sTemp\\", win_dir);
208+
if (written < 0 || (DWORD)written >= nBufferLength) {
209+
return 0;
210+
}
211+
ret = (DWORD)written;
212+
}
213+
return ret;
214+
}
215+
185216
size_t createTempFILEpath(char buffer[PATH_MAX]) {
186217
// On Windows, tmpfile() may attempt to create temp files in the root directory
187218
// of the drive, which requires Admin privileges, resulting in test failure.
188219
char pathname[PATH_MAX];
189-
if(0 == GetTempPathA(PATH_MAX, pathname)) {
220+
if(0 == GetSafeTempPathA(PATH_MAX, pathname)) {
190221
return 0;
191222
}
192223
return GetTempFileNameA(pathname, "awslctest", 0, buffer);
@@ -200,7 +231,7 @@ size_t createTempDirPath(char buffer[PATH_MAX]) {
200231
} random_bytes;
201232

202233
// Get the temporary path
203-
if (0 == GetTempPathA(PATH_MAX, temp_path)) {
234+
if (0 == GetSafeTempPathA(PATH_MAX, temp_path)) {
204235
return 0;
205236
}
206237

tool-openssl/ca.cc

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -126,16 +126,15 @@ static int WIN32_rename(const char *from, const char *to) {
126126
if (attempt > 0) {
127127
Sleep(kRetryDelayMs);
128128
}
129-
if (MoveFile(tfrom, tto)) {
129+
// Use MoveFileEx with MOVEFILE_REPLACE_EXISTING to match POSIX rename()
130+
// semantics: atomically replace the target if it already exists. Plain
131+
// MoveFile fails with ERROR_ALREADY_EXISTS when the target exists, and the
132+
// previous DeleteFile+MoveFile workaround was racy because NTFS defers
133+
// file removal until all handles are closed ("pending delete" state).
134+
if (MoveFileEx(tfrom, tto, MOVEFILE_REPLACE_EXISTING)) {
130135
goto ok;
131136
}
132137
err = GetLastError();
133-
if (err == ERROR_ALREADY_EXISTS || err == ERROR_FILE_EXISTS) {
134-
if (DeleteFile(tto) && MoveFile(tfrom, tto)) {
135-
goto ok;
136-
}
137-
err = GetLastError();
138-
}
139138
if (err != ERROR_ACCESS_DENIED && err != ERROR_SHARING_VIOLATION &&
140139
err != ERROR_LOCK_VIOLATION) {
141140
break;
@@ -149,7 +148,7 @@ static int WIN32_rename(const char *from, const char *to) {
149148
errno = EACCES;
150149
} else {
151150
errno = EINVAL;
152-
fprintf(stderr, "WIN32_rename: MoveFile('%s', '%s') failed with Windows "
151+
fprintf(stderr, "WIN32_rename: MoveFileEx('%s', '%s') failed with Windows "
153152
"error code %lu\n", from, to, (unsigned long)err);
154153
}
155154
err:

tool-openssl/ca_test.cc

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,14 @@ class CATest : public ::testing::Test {
8484
RemoveFile(db_old_path.c_str());
8585
std::string db_attr_old_path = std::string(db_path) + ".attr.old";
8686
RemoveFile(db_attr_old_path.c_str());
87+
std::string db_new_path = std::string(db_path) + ".new";
88+
RemoveFile(db_new_path.c_str());
89+
std::string db_attr_new_path = std::string(db_path) + ".attr.new";
90+
RemoveFile(db_attr_new_path.c_str());
8791
std::string serial_old_path = std::string(serial_path) + ".old";
8892
RemoveFile(serial_old_path.c_str());
93+
std::string serial_new_path = std::string(serial_path) + ".new";
94+
RemoveFile(serial_new_path.c_str());
8995

9096
// Clean up temp directory and its contents
9197
CleanupTempDir(new_certs_dir);

0 commit comments

Comments
 (0)