Repository navigation
feat(libressl3): retain X509 store references - #12580
PrivacyOnion wants to merge 2 commits into
Conversation
Progress report (f7f03da → 71e4f22)📈 System libraries: 43.43% (+0.01%, +2 implemented functions) ✅ 2 implemented
Changed libraries (implemented exports)
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The implementation and focused reference-count test look internally consistent, but the PR needs an explicit landing order with the other open libSceLibreSsl3 reference-count changes before approval. I fetched the exact current heads and checked pairwise merge previews: #12580 conflicts with #12578 and #12582 in Key.cpp, GuestLibreSsl3KeyReference.cpp, and docs/dev/debt/libSceLibreSsl3.md. All three PR descriptions currently say Depends on: none.
Please agree which PR lands first, restack the later changes as needed, and record the resulting order in each affected Depends on checklist. This is a repository coordination requirement; I am not reporting a functional defect in the accessor here. Hosted Windows is still running; Ubuntu/macOS and product test jobs are not complete or are skipped.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — changes requested
The ref-count change and its focused regression test look internally consistent. This PR still needs an explicit order with #12578 and #12582: exact-head pairwise merge previews conflict in the files both features edit, while the PR currently says Depends on: none.
What I ran
- Compared #12580 head
e5f35a2407c01477085fc0d9aa78ddbba669fda7with #12578 head00b24c75b4be4b3ee7ed97013e4ea4a7a21b3afeand #12582 headdd47c78d6d72d3d88b2d21ccac72ffd8adb193c3. - Generated pairwise merge previews against current
main319621ba2e578bd053b0baba2877fe70520e0737. - Refreshed hosted checks.
Results
| Check | Result |
|---|---|
| #12580 + #12578 | conflict in shared implementation, test, and debt files |
| #12580 + #12582 | conflict in shared implementation, test, and debt files |
| Current #12580 hosted policy checks | pass |
| Current #12580 Windows workflow | pass |
| Linux/product and specialized jobs | skipped |
Findings
Blocking
Please record an agreed dependency/landing order or restack this PR so its relationship to the two open sibling changes is explicit. The merge previews conflict in:
core/libs/prx/libSceLibreSsl3/src/Key.cppcore/libs/tests/GuestLibreSsl3KeyReference.cppdocs/dev/debt/libSceLibreSsl3.md
Non-blocking
I found no separate functional blocker in the implementation or focused test. This review is requesting changes for integration clarity only.
Sources
Not verified
I did not independently rerun the focused C++ tests. Hosted Linux/product checks were skipped.
To get this to a good state
Set the intended order in the PR body/checklist and declare the predecessor dependency, or restack and update the affected diffs so the pairwise merge previews are clean.
e5f35a2 to
e499110
Compare
|
Restacked on #12578, lands second. |
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
e499110 to
b1a7980
Compare
|
@MarcZX10 Thanks for the review.
|
Re-checked — dependency stack still conflictsI rechecked the live heads after your update; #12580 still does not descend from #12578, and combining the two heads conflicts in three files. What I ranThe live heads are #12578 git merge-base --is-ancestor refs/review/pr-12578 refs/review/pr-12580
git merge-tree --write-tree --name-only --no-messages refs/review/pr-12578 refs/review/pr-12580Results
Unedited command outputFindingsBlocking
Not verifiedI did not build or run CTest locally; I have not independently verified the author-reported focused test. To get this to a good statePlease update the branch and dependency metadata together. I will recheck the new head, the combined merge preview, and the focused test evidence. |
b1a7980 to
71e4f22
Compare
|
@MarcZX10 Thanks for the review.
|
|
struct x509_store_st {
STACK_OF(X509_OBJECT) *objs;
STACK_OF(X509_LOOKUP) *get_cert_methods;
X509_VERIFY_PARAM *param;
int (*verify)(X509_STORE_CTX *ctx);
int (*verify_cb)(int ok, X509_STORE_CTX *ctx);
int (*check_issued)(X509_STORE_CTX *ctx, X509 *x, X509 *issuer);
CRYPTO_EX_DATA ex_data; /* { void *sk; } */
int references;
};On LP64 that is 6 pointers plus The test doesn't catch it, because it builds the buffer from the same struct and checks offset 128:
AI-assisted: yes (Claude Code), the layout was read from the pinned LibreSSL 4.0.0 source above. |
What
Implement
X509_STORE_up_ref_nid_postfixinlibSceLibreSsl3.core/libs/prx/libSceLibreSsl3/src/Key.cpp, implementX509_STORE_up_ref_nid_postfix(LibreSslX509Store* store)to atomically incrementstore->referencesviaRetain_nid_no_patch, following LibreSSL 4.0.0 x509_lu.c#L240-L244.std::invalid_argument("X509_STORE_up_ref requires a store").LibreSslX509Storesize (136 bytes) andreferencesoffset (128 bytes) fromlibSceLibreSslwithstatic_assertinKey.cpp.X509_STORE_up_ref_nid_postfix(LibreSslX509Store* store)incore/libs/prx/libSceLibreSsl3/include/Api.hppand remove the stub fromcore/libs/prx/libSceLibreSsl3/Export.cpp.docs/dev/debt/libSceLibreSsl3.md.core/libs/tests/GuestLibreSsl3KeyReference.cppto coverLibreSslX509StorewithCheckKey<LibreSslX509Store>(X509_STORE_up_ref_nid_postfix, 128).Tested
Windows 11, verified reference count increments across initial values, sequential retain, and concurrent multithreaded retain via GuestLibreSsl3KeyReference.
Ran locally on
71e4f22169:Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)