Fix race condition in new_certs_dir output path#3095
Merged
Conversation
new_certs_dir output path
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3095 +/- ##
==========================================
- Coverage 78.16% 78.14% -0.03%
==========================================
Files 689 689
Lines 121628 121695 +67
Branches 16982 16997 +15
==========================================
+ Hits 95074 95094 +20
- Misses 25670 25715 +45
- Partials 884 886 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
new_certs_dir output pathnew_certs_dir output path
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
WillChilds-Klein
approved these changes
Mar 12, 2026
Comment on lines
+124
to
+129
| unlink(file_path.c_str()); | ||
| } | ||
| } | ||
| closedir(d); | ||
| } | ||
| rmdir(dir_path); |
Contributor
There was a problem hiding this comment.
do we need to account for failure conditions here? it's a test, so probably not...
nebeid
approved these changes
Mar 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of changes:
outdirlenincaToolwas never set after building the directory prefix, so it stayed at 0. This caused the cert filename (e.g.01.pem) to be written to the current working directory instead of the configurednew_certs_dir. When parallel test processes shared a working directory, they'd stomp on each other's01.pem.The fix sets
outdirlen = strlen(new_cert)after constructing the directory prefix so certs land in the right place.Also cleaned up
TearDownto remove the files that now (correctly) end up insidenew_certs_dir, and the auxiliary.oldfiles the CA tool creates during serial/db rotation.Call-outs:
This was a bug in
ca.ccitself, not the test. Everyopenssl cainvocation was writing the new cert to the CWD — the test flakiness just happened to surface it.Testing:
All 59
CATesttests pass. Verified via verbose output that certs now go to the temp directory (/tmp/awslcTestDir.../01.pem) instead of the CWD.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.