Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the ASN.1 OBJECT IDENTIFIER (OID) string encoding logic in lib/vtls/x509asn1.c and adds a new unit test suite to validate edge cases and limits around OID decoding/formatting.
Changes:
- Reworks
encodeOID()to better handle large (32-bit) OID components, add stricter validation, and propagate dynbuf errors (OOM/too large output). - Simplifies
OID2str()by removing an always-TRUE “symbolic” argument and always performing the symbolic lookup via dotted form. - Adds new unit test
unit1666plus build-system wiring to run it.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
lib/vtls/x509asn1.c |
Updates OID decoding (encodeOID) and simplifies OID2str call sites. |
tests/unit/unit1666.c |
New unit test coverage for encodeOID() behavior and edge cases. |
tests/unit/Makefile.inc |
Registers unit1666.c in the unit test bundle (Autotools). |
tests/unit/Makefile.am |
Adjusts unit test include paths (Autotools). |
tests/unit/CMakeLists.txt |
Adds include path for unit build (CMake). |
tests/data/test1666 |
New test case definition for running unit1666 via the test harness. |
tests/data/Makefile.am |
Registers test1666 in the test data list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
vszakats
pushed a commit
to vszakats/curl
that referenced
this pull request
Mar 19, 2026
- return error on OOM or doing too large output - fix full 32-bit number support - fix the broken handling of the first and second numbers - support up to 32-bit minus 80 for the second number - a field with a leading 0x80 is now considered an error, since it only works as padding and is then no longer the shortest possible version Add unit tests in 1666 Bonus: removed the last argument to OID2str() as it was always set TRUE. Closes curl#21003
bagder
force-pushed
the
bagder/encodeoid
branch
from
1d4b75e to
5eaa1f2
Compare
Member
Author
|
augment review |
🤖 Augment PR SummarySummary: Improves ASN.1 OID decoding/formatting in the X.509 parser and adds a dedicated unit test. Changes:
Technical Notes: The second arc is effectively limited to 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
vszakats
reviewed
Mar 19, 2026
- return error on OOM or doing too large output - fix full 32-bit number support - fix the broken handling of the first and second numbers - support up to 32-bit minus 80 for the second number - a field with a leading 0x80 is now considered an error, since it only works as padding and is then no longer the shortest possible version Add unit tests in 1666 Bonus: removed the last argument to OID2str() as it was always set TRUE.
bagder
force-pushed
the
bagder/encodeoid
branch
from
2b4dce5 to
6434f2c
Compare
Member
Author
🔒 Aisle Security Analysis✅ We scanned this PR and did not find any security vulnerabilities. Aisle supplements but does not replace security review. Analyzed PR: #21003 at commit Last updated on: 2026-03-19T12:24:11Z |
vszakats
added a commit
that referenced
this pull request
Mar 19, 2026
Fixing clang-tidy warning:
```
tests/unit/unit1666.c:50:12: error: call to undeclared function 'encodeOID'; ISO C99 and later do not support implicit function declarations [clang-diagnostic-implicit-function-declaration]
50 | result = encodeOID(dbuf, oid, oid + spec->size);
| ^
```
Ref: https://github.com/curl/curl/actions/runs/23297585235/job/67749144361?pr=21008#step:46:736
Follow-up to 673e14c #21003
Closes #21010
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.
Add unit tests in 1666
Bonus: removed the last argument to OID2str() as it was always set TRUE.