Skip to content

X509V3_EXT_print(): Return only 0 or 1 as the callers expect#29981

Open
t8m wants to merge 1 commit intoopenssl:masterfrom
t8m:x509v3_print-return
Open

X509V3_EXT_print(): Return only 0 or 1 as the callers expect#29981
t8m wants to merge 1 commit intoopenssl:masterfrom
t8m:x509v3_print-return

Conversation

@t8m
Copy link
Member

@t8m t8m commented Feb 11, 2026

Alternative to #29793

@t8m t8m added branch: master Applies to master branch approval: review pending This pull request needs review by a committer triaged: bug The issue/pr is/fixes a bug branch: 3.0 Applies to openssl-3.0 branch tests: exempted The PR is exempt from requirements for testing branch: 3.3 Applies to openssl-3.3 branch: 3.4 Applies to openssl-3.4 branch: 3.5 Applies to openssl-3.5 branch: 3.6 Applies to openssl-3.6 labels Feb 11, 2026
@t8m t8m moved this to Waiting Review in Development Board Feb 11, 2026
@fwh-dc
Copy link
Contributor

fwh-dc commented Feb 12, 2026

Either way the behavior should be documented to avoid future confusion.

@t8m
Copy link
Member Author

t8m commented Feb 12, 2026

Either way the behavior should be documented to avoid future confusion.

Sure, but that should be a separate PR IMO. I unfortunately do not have time to write that documentation currently.

@t8m t8m requested a review from a team February 12, 2026 08:51
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fwh-dc
Copy link
Contributor

fwh-dc commented Feb 13, 2026

Either way the behavior should be documented to avoid future confusion.

Sure, but that should be a separate PR IMO. I unfortunately do not have time to write that documentation currently.

Fair, I've submitted a PR for that. I like this alternative and it is consistent with documentation from openbsd.

@github-project-automation github-project-automation bot moved this from Waiting Review to Waiting Merge in Development Board Feb 13, 2026
Copy link
Contributor

@DDvO DDvO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this PR as the best solution because:

Looking at the semantics and the code of X509V3_EXT_print() and unknown_ext_print() I see not much reason to differentiate various reasons for non-success outcome and believe that the actual oversight was in unknown_ext_print() to use return BIO_dump_indent(out, (const char *)ext, extlen, indent) rather than return BIO_dump_indent(out, (const char *)ext, extlen, indent) > 0.

Having a closer look, I just found that ASN1_parse_dump(), which is also used by unknown_ext_print(), can even return 2!
So good that here return ASN1_parse_dump(out, ext, extlen, indent, -1) is changed to return ASN1_parse_dump(out, ext, extlen, indent, -1) > 0.

@DDvO DDvO added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Feb 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch branch: 3.0 Applies to openssl-3.0 branch branch: 3.3 Applies to openssl-3.3 branch: 3.4 Applies to openssl-3.4 branch: 3.5 Applies to openssl-3.5 branch: 3.6 Applies to openssl-3.6 tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug

Projects

Status: Waiting Merge

Development

Successfully merging this pull request may close these issues.

4 participants