Repository navigation
feat(libressl3): implement EVP_PKEY_get0_RSA, EVP_PKEY_get0_DSA and EVP_PKEY_get0_DH - #12582
PrivacyOnion wants to merge 3 commits into
Conversation
Progress report (f7f03da → 9379436)📈 System libraries: 43.45% (+0.03%, +5 implemented functions) ✅ 5 implemented
Changed libraries (implemented exports)
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The implementation and focused accessor tests 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: #12582 conflicts with #12578 and #12580 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 accessors 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 accessor implementation and focused tests look internally consistent. This PR still needs an explicit order with #12578 and #12580: exact-head pairwise merge previews conflict in files both changes edit, but this PR currently says Depends on: none.
What I ran
- Compared #12582 head
dd47c78d6d72d3d88b2d21ccac72ffd8adb193c3with #12578 head00b24c75b4be4b3ee7ed97013e4ea4a7a21b3afeand #12580 heade5f35a2407c01477085fc0d9aa78ddbba669fda7. - Generated pairwise merge previews against current
main319621ba2e578bd053b0baba2877fe70520e0737. - Refreshed hosted checks.
Results
| Check | Result |
|---|---|
| #12582 + #12578 | conflict in shared implementation, test, and debt files |
| #12582 + #12580 | conflict in shared implementation, test, and debt files |
| Current #12582 hosted policy checks | pass |
| Current #12582 Windows workflow | in progress at latest check |
| 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 tests. 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.
dd47c78 to
8409429
Compare
|
Restacked on #12580, lands third. |
|
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.
|
# Conflicts: # docs/dev/debt/libSceLibreSsl3.md
8409429 to
9379436
Compare
|
Thanks @MarcZX10. Addressed points:
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
I reviewed exact head 937943669c354a2b630e12b34002db5d8d26ef7f; the declared order is now #12578 → #12580 → #12582, and the accessor/type checks and reference-count tests match the implementations in the stacked diff. The author reports guest_libressl3_key_reference passing on this exact head; hosted Windows, conventions, labels, and report checks remain pending, while Linux and specialized jobs are skipped. I did not build or run CTest locally, and console layouts/error-queue behavior remain unverified as recorded in the debt entry.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, changes requested
The declared dependency order is clear, but the live branch is not stacked on #12580 and the combined merge preview conflicts in three files.
What I ran
I checked PR head 937943669c354a2b630e12b34002db5d8d26ef7f against current main at f7f03daf52f11191174b1be089a56e268390a707. The individual PR merges cleanly onto main; the blocker below is the integration of the declared dependency stack. Environment: Linux, Git.
git merge-base --is-ancestor refs/review/pr-12580 refs/review/pr-12582
git merge-tree --write-tree --name-only --no-messages refs/review/pr-12580 refs/review/pr-12582
git diff --check refs/remotes/upstream/main...refs/review/pr-12582
python3 tools/check_conventions.py --base refs/remotes/upstream/main --head refs/review/pr-12582Results
| Check | Result |
|---|---|
| Dependency ancestry | Fails: #12580's live head is not an ancestor of #12582's live head. |
| Combined #12580 + #12582 merge preview | Conflicts in Key.cpp, GuestLibreSsl3KeyReference.cpp, and docs/dev/debt/libSceLibreSsl3.md. |
git diff --check against main |
Reports an added blank line at EOF in the debt file. |
| Repository conventions | 0 errors; one existing debt-length warning (1,115 characters). |
| Individual #12582 merge onto current main | Clean. |
| Build or CTest | Not run locally. The author reports the focused CTest on this head; I have not independently verified that run. Windows CI is still in progress; policy checks pass and unrelated product jobs are skipped. |
Unedited command output
$ git merge-base --is-ancestor refs/review/pr-12580 refs/review/pr-12582
(no output; exit status 1)
$ git merge-tree --write-tree --name-only --no-messages refs/review/pr-12580 refs/review/pr-12582
df1feee819a536e9b0425c174b74d00ade791ad4
core/libs/prx/libSceLibreSsl3/src/Key.cpp
core/libs/tests/GuestLibreSsl3KeyReference.cpp
docs/dev/debt/libSceLibreSsl3.md
(exit status 1)
$ git diff --check refs/remotes/upstream/main...refs/review/pr-12582
docs/dev/debt/libSceLibreSsl3.md:210: new blank line at EOF.
$ python3 tools/check_conventions.py --base refs/remotes/upstream/main --head refs/review/pr-12582
1 findings, 0 errors; GitHub shows at most 10 error annotations, the job summary lists all
Findings
Blocking
- The declared stack does not integrate as submitted. #12582 lists
Depends on: #12580, but its commit graph contains separate copies of the earlierSSL_CTX_up_refandX509_STORE_up_refcommits rather than #12580's live head as an ancestor. Combining the current #12580 and #12582 heads conflicts in the three files above, so the stated landing order cannot be applied cleanly. After #12580 is merged, restack #12582 on the resultingmain, retain the EVP accessor change, resolve any resulting conflicts, and rerun the focused test and checks on the new head.
Non-blocking
- Remove the extra blank line at the end of
docs/dev/debt/libSceLibreSsl3.mdsogit diff --checkis clean.
Not verified
- I did not build or run CTest locally. The author-reported focused CTest and the in-progress Windows CI are not independent local results.
- I did not verify behavior on Windows, macOS, or console hardware.
To get this to a good state
Please restack this PR after #12580 lands, keep the dependency field aligned with the resulting branch, remove the trailing blank line, and rerun the focused test. I will re-check the new head, dependency merge preview, and the same applicable checks.
Re-checked — dependency head changed, conflicts remainThe current #12580 head includes #12578, but #12582 still targets an older #12580 head and now conflicts with the updated dependency. What I ranChecked exact heads #12578 python3 /root/.codex/skills/anyps5-pr-review/scripts/check_pr_stack.py \
--base f7f03daf52f11191174b1be089a56e268390a707 \
--dependency 12578=ced92ec4eda54f95ccf42ef08408bb4aaec363a6 \
--dependent 12580=71e4f221692913c12f2424c2c4ae342c02543c6f
python3 /root/.codex/skills/anyps5-pr-review/scripts/check_pr_stack.py \
--base f7f03daf52f11191174b1be089a56e268390a707 \
--dependency 12580=71e4f221692913c12f2424c2c4ae342c02543c6f \
--dependent 12582=937943669c354a2b630e12b34002db5d8d26ef7fResults
Not verifiedI did not run the focused CTest locally. The local preview checks ancestry and merge conflicts only; it does not test runtime behavior. To get this to a good statePlease update #12582 on the current #12580 head |
What
NidRsaEncryption(6),NidDhKeyAgreement(28),NidDsa(116), andNidRsassaPss(912) incore/libs/prx/libSceLibreSsl3/include/Key.hpp.EVP_PKEY_get0_RSA,EVP_PKEY_get0_DSA, andEVP_PKEY_get0_DHincore/libs/prx/libSceLibreSsl3/src/Key.cpp, returning the underlying key pointer fromLibreSsl3EvpPkeywhen the key type matches ornullptron mismatch, following LibreSSL 4.0.0. Null key pointers throwstd::invalid_argument.core/libs/prx/libSceLibreSsl3/include/Api.hppand remove stubs fromcore/libs/prx/libSceLibreSsl3/Export.cpp.docs/dev/debt/libSceLibreSsl3.mdnoting unmodeledERRand omittedEVPerrorcodes.core/libs/tests/GuestLibreSsl3KeyReference.cppto verify valid key pointer retrieval, type filtering, and null key exception handling.Tested
Windows 11, verified
guest_libressl3_key_referencecovering key retrieval and null argument validation forEVP_PKEY_get0_RSA,EVP_PKEY_get0_DSA, andEVP_PKEY_get0_DH.Ran locally on
937943669c: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)