Merged
Conversation
c326fc0 to
dab618b
Compare
andrewhop
reviewed
Feb 15, 2023
dab618b to
15d051e
Compare
andrewhop
previously approved these changes
Feb 15, 2023
Our NCONF_get_section doesn't silently handle NULLs, so add a check to the caller. Change-Id: I133d10c7a5dec22469a80b78cb45f479f128ada2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56106 Reviewed-by: Bob Beck <bbe@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit 871704b873e356b9eee354b70bf0f11cf234a1f7)
Functions typically just push their own error codes. Change-Id: Iac83bfdf56ce436bd3a9b8af5c43ca67fb4b9b15 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56107 Reviewed-by: Bob Beck <bbe@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit db38fc55939095dd8082e0a89b0c9d187ac164f6)
Squat fewer unprefixed macros. Update-Note: CTX_TEST appears to be unused. If affected, switch to using X509V3_set_ctx_test instead. Change-Id: I43b86c0b6f147bbca85b8bc6b43602fc4f6697c1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56108 Reviewed-by: Bob Beck <bbe@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit 65ad925f51d879f8babd22ecb2ca4dc779b2cc24)
This relaxes two caller requirements: First, although one needs to initialize X509V3_CTX in two parts, some callers forget to this. This works some of the time on accident, because most codepaths read ctx->db. But if one were to read it, it'd be uninitialized. Since all the entrypoints take a CONF anyway, and always match them, just implicitly initialize the CONF half of the X509V3_CTX with the provided one. Second, allow X509V3_CTX to be NULL. Some codepaths in the library check for NULL (or don't use it) and some do not. Enough codepaths don't check that it really cannot be considered to work, but enough do that a caller could mistakenly pass in NULL and have it mostly work. I've seen one caller mistakenly do this. Since we have to copy the X509V3_CTX for the first relaxation anyway, allow it to be NULL and fill in an empty one when omitted. Update-Note: If using different CONFs in the X509V3_CTX and the function parameter, the function parameter is now always used. No callers do this, and it's somewhat arbitrary which is used. (The generic code always uses the one in ctx. The @section syntax uses the parameter. Then the per-extension callbacks use the ctx.) Change-Id: I9fc15a581ea375ea06c4b082dcf0d6360be8144f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56109 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> (cherry picked from commit 114fa727b7281bf532d037036356359619c730be)
This is purely cosmetic, but makes it consistent with the DW, etc., lines in the block above. The SEH unwind code support will emit a mix of DW and DB directives and this makes them look more consistent. Bug: 259 Change-Id: Ia16166ab2495aa813d6076d55af5b62511933c28 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56125 Reviewed-by: Adam Langley <agl@google.com> Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit aa18fe24cdbe4d0b353cd3d82a6a5017af88ccb1)
This reimplements policy handling using a similar DAG structure as in https://chromium-review.googlesource.com/c/chromium/src/+/4111415. The main difference is that, being C, we don't have std::set or std::map easily available. But the algorithm can be implemented purely with sorted lists, while remaining subquadratic. This implementation relies on two assumptions: 1. We do not return the policy tree. This was removed in https://boringssl-review.googlesource.com/c/boringssl/+/53327 2. We do not return the final set of certificate policies. I.e., certificate policy checking is only used for evaluating policy constraints and X509_V_FLAG_EXPLICIT_POLICY. The second assumption is not very important. It mostly simplifies has_explicit_policy slightly. In addition, this new implementation removes the per-certificate policy cache. Instead, we just process the policy extensions anew on certificate verification. This avoids a mess of threading complexity, including a race condition in the old logic. See https://boringssl-review.googlesource.com/c/boringssl/+/55762 for a description of the race condition. Change-Id: Ifba9037588ecff5eb6ed3c34c8bd7611f60013a6 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56036 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> (cherry picked from commit 029d0e77fb64625469cc02c8df26767c72081dfd)
This implements similar directives as MASM, so we do not need to build all the structures by hand. It does not provide any help to abstract between Win64 and SysV differences, however. This is pulled together from some old draft CLs I had, one of which actually synthesized CFI directives from SEH, so it should be possible. I've intentionally omitted that however, as it also brings in questions about how to handle the calling convention differences (the existing machinery won't *quite* work). I've uploaded just this for now, so review can focus on the basic mechanism. I've also preserved perlasm's weird mixed tabs and spaces indentation convention for now, though it is a bit tedious. Bug: 259 Change-Id: Ib3f46a27751a5319b758d12c462c660cf9f3e632 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56126 Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: Adam Langley <agl@google.com> (cherry picked from commit c556ee91ff7fc106baadb41ddaaae951ac9b3ba2)
The ABI test already uses CHECK_ABI_SEH, so this is covered under tests. As part of the conversion, we no longer rely on the assembler generating a specific encoding and can just write the code normally. Bug: 259 Change-Id: I47cbf81073237f2b95971a782848b85d230b6bf6 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56127 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: Adam Langley <agl@google.com> (cherry picked from commit ae1546b6f3bf1ad7eb24b491c914eb202b5547d3)
https://boringssl-review.googlesource.com/c/boringssl/+/56109 tried to simplify the X509V3_CTX story by automatically handling the second half of initialization, but it turns out not all callers specify both values. Instead, align with OpenSSL 3.0's behavior. Now X509V3_set_ctx implicitly zeros the other fields, so it is the only mandatory init function. This does mean callers which call X509V3_set_nconf before X509V3_set_ctx will break, but that's true in OpenSSL 3.0 too. I've retained the allowance for ctx being NULL, because whether functions tolerate that or not is still a bit inconsistent. Also added some TODOs about how strange this behavior is, but it's probably not worth spending much more time on this code. Change-Id: Ia04cf11eb5158374ca186795b7e579575e80666f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56265 Reviewed-by: Adam Langley <agl@google.com> Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: Adam Langley <agl@google.com> (cherry picked from commit 1c9d18307bdf75bee7f3ec7f7d2e421a734831f1)
Some profiling systems cannot unwind with CFI and benefit from having a frame pointer. Since this code doesn't have enough register pressure to actually need to use rbp as a general register, this change tweaks things so that a frame pointer is preserved. As this would invalidate the SEH handler, just replace it with proper unwind codes, which are more profiler-friendly and supportable by our unwind tests. Some notes on this: - We don't currently support the automatic calling convention conversion with unwind codes, but this file already puts all arguments in registers, so I just renamed the arguments and put the last two arguments in RDI and RSI. Those I stashed into the parameter stack area because it's free storage. - It is tedious to write the same directives in both CFI and SEH. We really could do with an abstraction. Although since most of our functions need a Windows variation anyway. - I restored the original file's use of PUSH to save the registers. This matches what Clang likes to output anyway, and push is probably smaller than the corresponding move with offset. (And it reduces how much thinking about offsets I need to do.) - Although it's an extra instruction, I restored the original file's separate fixed stack allocation and alloca for the sake of clarity. - The epilog is constrained by Windows being extremely picky about epilogs. (Windows doesn't annotate epilogs and instead simulates forward.) I think other options are possible, but using LEA with an offset to realign the stack for the POPs both matches the examples in Windows and what Clang seems to like to output. The original file used MOV with offset, but it seems to be related to the funny SEH handler. - The offsets in SEH directives may be surprising to someone used to CFI directives or a SysV RBP frame pointer. All three use slightly different baselines: CFI's canonical frame address (CFA) is RSP just before a CALL (so before the saved RIP in stack order). It is 16-byte aligned by ABI. A SysV RBP frame pointer is 16 bytes after that, after a saved RIP and saved RBP. It is also 16-byte aligned. Windows' baseline is the top of the fixed stack allocation, so potentially some bytes after that (all pushreg and allocstack directives). This too is required to be 16-byte aligned. Windows, however, doesn't require the frame register actually contain the fixed stack allocation. You can specify an offset from the value in the register to the actual top. But all the offsets in savereg, etc., directives use this baseline. Performance difference is within measurement noise. This does not create a stack frame for internal functions so frame-pointer unwinding may miss a function or two, but the broad attribution will be correct. Change originally by Clemens Fruhwirth. Then reworked from Adam Langley's https://boringssl-review.googlesource.com/c/boringssl/+/55945 by me to work on Windows and fix up some issues with the RBP setup. Bug: b/33072965, 259 Change-Id: I52302635a8ad3d9272404feac125e2a4a4a5d14c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56128 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit 0d5b6086143d19f86cc5d01b8944a1c13f99be24)
There was a typo and inhibitPolicyMapping updated the wrong value. With this fixed, we pass the PKITS tests (as imported into Chromium). NOTE: Imported the fix only, skipped the new tests because they depend on some previously skipped commits. Added action items to the backlog. Change-Id: I3b80eb56561ae5ae88023fa639d697a9f1757b21 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56205 Commit-Queue: David Benjamin <davidben@google.com> Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> Commit-Queue: Bob Beck <bbe@google.com> (cherry picked from commit ba68ca070ca939bc2d6b8f07bd64909bd90b25a5)
15d051e to
3eb17d0
Compare
samuel40791765
approved these changes
Feb 15, 2023
andrewhop
approved these changes
Feb 15, 2023
R3hankhan123
pushed a commit
to R3hankhan123/aws-lc
that referenced
this pull request
Aug 29, 2025
* Disallow external bindgen when AWS_LC_SYS_EXTERNAL_BINDGEN=0 * Fix s390x-unknown-linux-gnu CI test
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
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.
Issues:
N/A
Description of changes:
Describe AWS-LC’s current behavior and how your code changes that behavior. If there are no issues this pr is resolving,
explain why this change is necessary.
Call-outs:
Point out areas that need special attention or support during the review process. Discuss architecture or design changes.
Testing:
How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?
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.