Repository navigation
feat(libSceLibreSsl): implement ASN.1 integer and string helpers - #10005
Conversation
|
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.
|
|
I reran the overlap scan on this head. It confirms the three disclosed debt-row overlaps (#9529, #9942 and #9949) and reports that comparisons with #9033, #9041, #9047, #9446, #9448, #9571, #9745, #9870 and #9970 could not complete because those branches conflict with main in CMakeLists.txt, Export.cpp and/or the debt file. Your description says the five exports are distinct, and the scan found no same-export or new-file overlap. Could you add the agreed integration order for the shared CMake/debt hunks (and a Depends on entry if another PR must land first), or clarify how you plan to reconcile them when rebasing? This is a merge-coordination question, not a request to close the PR. |
644ec92 to
ef8d0ce
Compare
|
Updated the PR description under Overlap and rebase coordination with the concrete reconciliation plan. No other PR must land first, so If one of the shared PRs lands first, I will rebase this branch, preserve the union of source files/test targets and Windows PATH entries, retain independent debt entries, and recount the remaining throwing exports from the resulting source. I will remove only the five throwing declarations replaced here, rerun the focused build/tests, conventions and overlap scan, and update this PR with I rebased onto current |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I found a mismatch between the implementation and its own new regression. In core/libs/prx/libSceLibreSsl/src/Asn1.cpp, ASN1_INTEGER_get_nid_postfix rejects length > 8 but does not reject negative lengths. For the test case String value{-1, 2, data, 0}, the loop executes zero times, magnitude stays 0, and the function returns 0; Integers() asserts that this input returns -1. LibreSSL 2.5.4 also explicitly rejects aint->length < 0 in ASN1_INTEGER_get_int64 before decoding. Please add the negative-length check and rerun guest_libressl_asn1_tests and the focused test set; the reported 5/5 result should then be reconciled with the exact current source/head.
|
I verified the exact implementation outside the project test harness with a small ABI-compatible call using |
|
I checked the exact reviewed head I added an explicit nonpositive-length guard, expanded tests across INT32_MIN/-8/-1/0, both INTEGER signs, null/valid/inaccessible data pointers and input preservation, and documented the version difference. The guard keeps the verified 2.5.4 result of 0. Pushed |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The updated exact head 1ba3d6defa37205b5149c33fd01ac0d0b47f7c47 resolves my earlier negative-length finding. The new length <= 0 guard avoids reading invalid data for nonpositive lengths, and the expanded regression covers INT32_MIN, -8, -1 and zero with both integer tags and null/valid/inaccessible data pointers. I also verified the version distinction in the cited source: LibreSSL 2.5.4 ASN1_INTEGER_get loops zero times for a negative length and returns 0, while the later ASN1_INTEGER_get_int64 explicitly rejects negative lengths. The implementation and updated tests now match the 2.5.4 contract the PR documents.
The author reports the exact-head ASN.1 tests and focused suite passing 5/5, plus conventions and whitespace checks; the hosted conventions check now passes. I did not run the C++ test executable locally in this session. My earlier request for changes applied to the superseded head and is resolved by this update.
|
@oneandonlydean @adriadam10 — could either of you take a maintainer pass on this PR when you have time? MarcZX10 has approved the current head, Please let me know if there are any remaining merge blockers. Thank you! |
|
@AZLisme the valid-input coverage and allocator-ownership checks look useful. Please drop the assertions that require the unimplemented error-queue paths to throw: |
1ba3d6d to
d5e392e
Compare
|
Addressed in The ASN.1 production implementation and documented error-queue debt are unchanged. I also rebased onto current main, preserved its DES implementation/test and reconciled the shared debt count. Rebuilt the actual library and focused executables; |
d5e392e to
f99296c
Compare
|
Rebased onto Rebuilt the focused executables: 7/7 tests pass, including the current DES and X509 error-string regressions. Conventions and whitespace checks pass. GitHub now reports this head as MERGEABLE, with successful checks; the |
github-actions
Bot
added
needs rebase
|
Rebased again and pushed Rebuilt the runtime libraries and eight focused executables: 9/9 tests pass, including AES, DES, X509, RAND/CRYPTO and both ASN.1 registrations. Local conventions, whitespace checks and hosted conventions/labels checks pass. Refreshed the overlap audit and manually checked shared/uncompared diffs by helper names and NIDs; no duplicate implementations were found. GitHub reports this head as MERGEABLE against current main, including the subsequent USB-storage merge. The @MarcZX10 Could you re-review the signed-boundary fix on this head? @oneandonlydean Please run the required full CI and merge when you are satisfied with review and validation. The proposed sequence remains #10005 first, followed by rebasing draft #10418. |
|
@AZLisme I checked 025bed3. The sign-dependent limit and overflow-safe negative conversion handle the requested INT64_MIN bytes, and the regression preserves the input. My standalone extraction rejects that case on the old head and passes it plus 20,000 representable signed-magnitude checks under UBSan on this head. I also counted 352 remaining throwing exports, matching the debt entry; main’s AES and runtime entries are retained without duplicate debt. This addresses the source finding from my side. MarcZX10’s change request still needs their re-review, and full Linux/Windows CI must bind this exact head; the standalone check does not replace runtime qualification. |
|
@AZLisme The local declarations differ from main’s public headers: |
025bed3 to
ce45830
Compare
|
@oneandonlydean Addressed in The old Both implementation and tests now include Validation on Arch Linux x86-64: 65/65 public-header tests pass, verifying 368 libraries and 12,171 exports plus all header compilation batches; 9/9 focused runtime tests pass after rebuilding the libraries and eight executables. Conventions and whitespace checks pass. The refreshed overlap scan and manual shared/uncompared diff audit found no other implementation of these five helpers. Please re-review this header integration and the earlier signed-boundary fix. A maintainer full Linux/Windows CI run must qualify this new head before merge; my local checks do not replace that qualification. |
ce45830 to
454f662
Compare
|
Rebased onto current main Rebuilt the runtime libraries and focused executables: 9/9 runtime tests and 65/65 public-header tests pass. Conventions and whitespace checks pass. Refreshed the overlap scan and inspected all 33 other shared/uncompared diffs by helper names and NIDs; no duplicate implementations were found. GitHub reports MERGEABLE, with successful hosted conventions and labels checks. The header integration and |
|
Full Linux/Windows CI run 40151824506 is now successful for exact head
The PR description now includes the hosted validation and its limits. The reviewed code and public declarations are unchanged, and the PR remains mergeable. @MarcZX10 Could you re-review the signed-boundary and canonical-header fixes on this qualified head? @oneandonlydean CI qualification is complete; please merge when satisfied with review. The agreed order remains #10005 before draft #10418. |
454f662 to
6098cf2
Compare
|
Main merged #9529 after the previous full CI run, creating a conflict in the shared LibreSSL debt file. Rebased onto The conflict resolution preserves the upstream error-queue implementation, public declarations, test target and debt entry. The ASN.1 source, tests, shared record and five public signatures are byte-identical to the previously qualified head. Exactly five stubs are replaced; 348 unknown exports remain. On the new head, 10/10 focused runtime tests pass, including the new error-queue regression, and 65/65 public-header checks pass. Conventions and whitespace checks pass. The overlap audit inspected 33 other shared/uncompared PR diffs and found no duplicate helper implementation. GitHub reports the new head as mergeable. Run 40151824506 remains successful evidence for the previous head |
|
The fresh CI run 40155070561 is not fully green: Windows passed, while Linux reported one failure in I reproduced that same assertion locally on both this PR and an isolated build of the exact CI base
I used All 10 LibreSSL/crypto tests and 65 public-header checks passed on both platforms. Windows finished with 1,516 passed, 789 expected skips and zero failures. Linux finished with 2,123 passed, 89 skips and this single failure; its workflow skip-check step was not run after CTest failed. @oneandonlydean Could you retry the Linux job on unchanged head |
|
CI run 40155070561, attempt 2 is now successful on unchanged head I checked the completed Linux retry log: it freshly checked out main
The PR description now records this qualification. The earlier floating-point failure reproduced on the base remains documented above; a passing retry does not resolve its underlying cause. No source changes were needed, and GitHub currently reports the PR as mergeable. @MarcZX10 Could you re-review the signed-boundary and canonical-header fixes? @oneandonlydean Thanks for retrying Linux; please merge when final review is satisfactory. The agreed order remains #10005 before draft #10418. |
What
Implement five missing
libSceLibreSslexports:ASN1_INTEGER_get,ASN1_STRING_data,ASN1_STRING_length,ASN1_STRING_type, andASN1_STRING_to_UTF8. The integer accessor uses a 64-bit guestlong; string accessors preserve the supplied pointer, length and tag. Conversion supports every string tag accepted by the pinned implementation, including embedded NULs, and returns a separately allocated, terminated UTF-8 buffer through guest libc and its application allocator.Signatures, the 24-byte guest string layout and behavior follow LibreSSL 2.5.4 ASN.1 sources, including
asn1.h,a_int.c,asn1_lib.c,a_strex.c,a_mbstr.canda_utf8.c. Firmware 13.40 reports this version in discussion #8981; console behavior was not independently measured. Nonpositive integer lengths return zero for valid INTEGER tags, as 2.5.4 does.Malformed encodings and allocation failure still throw until ASN.1 error-queue reporting is implemented; these are recorded in
docs/dev/debt/libSceLibreSsl.md. The regression suite keeps successful conversions, argument validation and successful allocator/free ownership coverage without requiring those unfinished error paths to throw. The integer accessor handles the full representable signed range, including negative magnitude2^63, without signed overflow; larger unrepresentable values remain recorded as debt.The five exports now have their sole declarations in
include/Api.hpp, reached throughlibSceLibreSsl.hpp. The implementation and tests share itsAsn1Stringrecord; local export declarations and duplicate layouts are removed. The formerint function(void)declarations were unknown-signature placeholders for throwing stubs. The implemented API requires the pinned source’s arguments, pointer returns and 64-bit guestlong, so those public declarations are corrected rather than retained. Guest libc allocation/free declarations also come from its canonical header.Tested
Local Arch Linux x86-64, GCC 16.2.1, CMake 4.4.4, Ninja, Release with
BUILD_TESTING=ON: exact head6098cf2e6463f0a1726c121b5e3616d450c38246, rebased ontodc15ed65375228a0f69c41d7770c4c118adb38f2. Rebuilt the actual runtime libraries and nine focused executables.ctest --test-dir build -R 'libressl|crypto_memcmp' --output-on-failure: 10/10 passed, including both ASN.1 registrations and main's error-queue regression.ctest --test-dir build -R '^public_headers' -j2 --output-on-failure: 65/65 passed. Conventions and whitespace checks pass.Coverage includes supported conversion tags and Unicode boundaries, embedded NULs, input preservation, accessors, signed integer limits including
INT64_MIN, legacy nonpositive lengths, argument validation and successful application allocation followed by guestfree. The overlap scanner and manual name/NID audits found no duplicate implementation. The latest refresh checked all 18 listed PR heads, including new ENGINE/init PR #10878 and the updated EC-helper PR #10395; neither duplicates these helpers or introduces a prerequisite.Full Linux/Windows run 40155070561, attempt 2 is successful for exact head
6098cf2e6463f0a1726c121b5e3616d450c38246merged onto main52a8bed576e5c54a7f443728cdf663a080f8f9ed. The fresh Linux checkout/merge log independently verifies both commits.guest_floating_pointpassed.The first Linux attempt failed the intermittent
guest_floating_pointassertion, which also reproduced on an isolated build of the exact base without this PR's changes. The base-reproduction evidence remains recorded; the successful retry does not establish or fix its underlying cause. The ASN.1 patch is unchanged.Earlier run 40151824506 passed both configured platform suites for previous head
454f66248dd919868c706aa599af34fe039b5f1c; it does not qualify this rebase. macOS, PS5 and titles were not tested. No GPU code changed.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)Overlap and rebase coordination
No prerequisite is required. As proposed by its author, draft #10418 will follow #10005 and rebase afterward. If a shared branch lands first, rebase while preserving the union of library sources, focused tests and Windows PATH entries; remove only the five throwing declarations replaced here. Keep independent debt entries and recount remaining throwing exports from the resulting source. Rerun relevant builds, focused tests, conventions and overlap checks before updating with
--force-with-lease. This rebase preserved main's AES, DES, X509 error-string, RAND/CRYPTO runtime and ERR queue implementations, regression targets and debt entries; the reconciled pending-export count is 348.