Repository navigation
Conversation
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the implementation and the host-side behavior is consistent with the cited LibreSSL 4.0.0 source. The new regression covers OR semantics, sign extension, writes outside the flags field, and repeat calls. I also ran g++ -std=c++20 -Icore/libs -fsyntax-only on Export.cpp and GuestLibreSsl3CipherFlags.cpp, tools/check_conventions.py --base refs/review/upstream-main, and git diff --check; all passed. A full local CMake configure could not proceed because the SDL2 submodule is absent and SDL XRandR support is unavailable; hosted product jobs are skipped by path policy.
Two items still block approval:
-
LibreSsl3CipherContextand the write at offset 104 are inferred from LibreSSL 4.0.0, but the PR says the PS5libSceLibreSsl3signature and context layout are unverified. The guest passes an existing context created by the target library; host tests against the newly declared struct only prove the adapter against that struct, not that a PS5 context has 64-bit flags at offset 104. Please provide target ABI evidence (for example, a measured console context or disassembly establishing the argument contract and field offset/width). Without it, this write can modify the wrong field or corrupt the context; keep the export on the throwing path until the guest ABI is established. -
The description says merge order for the ten shared-file PRs is unresolved. I fetched their current heads and trial-merged them against this head: content conflicts remain with #9970, #10452, #10461, #10490, #10491, #10493, #10557, #10631, #10948 and #10960, in the stated CMake/header/debt files.
CONTRIBUTING.mdrequires author agreement on overlap order and a matchingDepends onchecklist entry when another PR must land first. Please coordinate the order and update the checklist before this is considered ready. No competing implementation of this exact function appeared in those diffs.
|
Thank you for the careful review and for identifying the missing target ABI evidence. I agree that the LibreSSL 4.0.0 source and host-side tests establish the behavior of the proposed adapter, but do not establish the PS5 context layout or justify writing at offset 104. I do not currently have a console measurement or module disassembly that closes that gap, so the export should remain on the existing throwing path. I prepared a local rollback of the implementation, inferred layout and associated test. After that rollback, the net change against current main is only an additional technical-debt entry, which does not provide enough value to continue pursuing this PR. That rollback has not been pushed. The shared-file merge order also remains unagreed; I am not treating the overlap concern as resolved. I am therefore closing this PR. I will retain the implementation and host-side validation as reference material and revisit a functional contribution once the PS5 argument contract, field offset/width and update semantics can be verified, with any remaining overlap coordinated at that point. Thank you again for the review and your time. |
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
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.
|
What
EVP_CIPHER_CTX_set_flagsin libSceLibreSsl3 currently throws. Implement itsvoid (EVP_CIPHER_CTX*, int)contract, OR-ing the signed mask into the 64-bit context flags while preserving every other byte. The canonical public declaration exposes the guest calling convention and an explicit LP64 context layout on Windows.The signature and behavior follow LibreSSL 4.0.0 evp_cipher.c and evp_local.h. A null context throws where the original dereferences it. Console signature/layout equivalence remains unverified and is recorded in the library's debt file.
There is no functional prerequisite. Shared edits to CMake, the public header and the pending-export paragraph overlap #9970, #10452, #10461, #10490, #10491, #10493, #10557, #10631, #10948 and #10960. Their landing order has not been agreed.
Depends on: nonerecords the absence of a functional prerequisite; shared-file landing order remains to be coordinated before merging. I can rebase after those changes land.Tested
Final local commit:
7de7ed5012a100a020bb4d214c7a1f634846b643; base:636a9c1eecce2e467dba12476e3a42a82e540c7f.-fsanitize=address,undefined, fail-fast checks and leak detection): 12/12 passed with no sanitizer report.longas 8 bytes, its context as 160 bytes and flags at offset 104. Full context bytes matched for 100,040 inputs, each called twice: 200,080 comparisons passed, including every mask bit, signed limits and deterministic mixed masks.uint32_t, and a write at offset 112. Each failed its behavioral assertion. Original source bytes were restored, rebuilt and the regression passed.PS5 hardware, games and upstream CI were not run.
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)