Skip to content

Comments

Fix regression from the new base64 encoder and other related cleanups#29550

Closed
t8m wants to merge 4 commits intoopenssl:masterfrom
t8m:base64-zero-div
Closed

Fix regression from the new base64 encoder and other related cleanups#29550
t8m wants to merge 4 commits intoopenssl:masterfrom
t8m:base64-zero-div

Conversation

@t8m
Copy link
Member

@t8m t8m commented Jan 5, 2026 •
edited
Loading

Fixing a few things:

  • long-standing bug in b64 bio where it was calling EVP_EncodeFinal() on flush when decoding
  • ctx->length is a constant in EVP_ENCODING_CTX so make it as such

Fixes #29518

@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 triaged: refactor The issue/pr requests/implements refactoring severity: regression The issue/pr is a regression from previous released version tests: exempted The PR is exempt from requirements for testing labels Jan 5, 2026
@t8m t8m requested review from Sashan and nhorman January 5, 2026 18:07
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jan 5, 2026
nhorman
nhorman previously approved these changes Jan 5, 2026
View reviewed changes
@loci-dev loci-dev mentioned this pull request Jan 5, 2026
Open
@t8m
Copy link
Member Author

t8m commented Jan 6, 2026

ping @openssl/committers for second review

@t8m t8m requested a review from mattcaswell January 6, 2026 08:46
mattcaswell
mattcaswell requested changes Jan 6, 2026
View reviewed changes
@t8m t8m force-pushed the base64-zero-div branch from 804a116 to 809ee66 Compare January 8, 2026 10:40
@t8m t8m added this to the 4.0.0 milestone Jan 8, 2026
@t8m t8m requested review from mattcaswell and nhorman January 8, 2026 10:42
@t8m
Copy link
Member Author

t8m commented Jan 8, 2026

Rebased, applied some of the review suggestions from Matt. Added commit to rename test_base64_simdutf to base64_simdutf_test for consistency.

t8m added 3 commits January 15, 2026 13:42
@t8m
b64_ctrl(): Do not try to call EVP_Encode functions when not writing
6be1a64 
The BIO_CTRL_FLUSH should just forward the call to the underlying
BIOs when not writing.
@t8m
base64 encoder: Make ctx->length a constant
bed84d4 
It is never changed anywhere.

Fixes openssl#29518
@t8m
Rename test_base64_simdutf to base64_simdutf_test
6f7119e 
The new name is better for consistency with other tests.
@t8m t8m force-pushed the base64-zero-div branch from 809ee66 to 6f7119e Compare January 15, 2026 12:45
Sashan
Sashan requested changes Jan 15, 2026
View reviewed changes
Copy link
Contributor

@Sashan Sashan left a comment

Choose a reason for hiding this comment

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

As I've mentioned in my comment may be we can move those constant conditions to asserts. this is just a thought to consider. I'm also fine leaving the code in PR as is.

crypto/evp/encode.c

#include <stdio.h>
#include <limits.h>
#include <assert.h>
Copy link
Contributor

@Sashan Sashan Jan 15, 2026

Choose a reason for hiding this comment

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

NIT: wen in here would it make sense to remove BIN_PER_LINE CHUNKS_PER_LINE and CHAR_PER_LINE as they are unused (for a long time, so we can zap them in follow up PR).

crypto/evp/encode.c
return 0;
OPENSSL_assert(ctx->length <= (int)sizeof(ctx->enc_data));
if (ctx->length - ctx->num > inl) {
assert(EVP_ENCODE_B64_LENGTH <= (int)sizeof(ctx->enc_data));
Copy link
Contributor

@Sashan Sashan Jan 15, 2026

Choose a reason for hiding this comment

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

just a side note: perhaps we can introduce compile time asserts, can be done in follow up PR.

crypto/evp/evp_local.h

#define EVP_CTRL_RET_UNSUPPORTED -1

#define EVP_ENCODE_B64_LENGTH 48
Copy link
Contributor

@Sashan Sashan Jan 15, 2026

Choose a reason for hiding this comment

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

The comment saying EVP_ENCODE_B64_LENGTH should be divisible by 3 to keep CPU optimized code used does not hurt.

Copy link
Member Author

@t8m t8m Jan 15, 2026

Choose a reason for hiding this comment

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

Comment added.

@t8m
Copy link
Member Author

t8m commented Jan 15, 2026

I prefer not adding the asserts.

@t8m t8m requested a review from Sashan January 15, 2026 13:50
Sashan
Sashan approved these changes Jan 15, 2026
View reviewed changes
Copy link
Contributor

@Sashan Sashan left a comment

Choose a reason for hiding this comment

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

I think this can go in. Looks good to me.

@paulidale paulidale 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 Jan 15, 2026
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 16, 2026
@openssl-machine
Copy link
Collaborator

openssl-machine commented Jan 16, 2026

This pull request is ready to merge

@mattcaswell
Copy link
Member

mattcaswell commented Jan 19, 2026

Pushed to master. Thank you!

openssl-machine pushed a commit that referenced this pull request Jan 19, 2026
@t8m @mattcaswell
b64_ctrl(): Do not try to call EVP_Encode functions when not writing
84eb1d3 
The BIO_CTRL_FLUSH should just forward the call to the underlying
BIOs when not writing.

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Matt Caswell <[email protected]>
MergeDate: Mon Jan 19 14:20:35 2026
(Merged from #29550)
openssl-machine pushed a commit that referenced this pull request Jan 19, 2026
@t8m @mattcaswell
base64 encoder: Make ctx->length a constant
a27b2ca 
It is never changed anywhere.

Fixes #29518

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Matt Caswell <[email protected]>
MergeDate: Mon Jan 19 14:20:35 2026
(Merged from #29550)
openssl-machine pushed a commit that referenced this pull request Jan 19, 2026
@t8m @mattcaswell
Rename test_base64_simdutf to base64_simdutf_test
bcc33df 
The new name is better for consistency with other tests.

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Matt Caswell <[email protected]>
MergeDate: Mon Jan 19 14:20:35 2026
(Merged from #29550)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@esyr esyr esyr left review comments

@Sashan Sashan Sashan approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

@paulidale paulidale paulidale approved these changes

@nhorman nhorman Awaiting requested review from nhorman

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch severity: fips change The pull request changes FIPS provider sources severity: regression The issue/pr is a regression from previous released version tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug triaged: refactor The issue/pr requests/implements refactoring

Projects

None yet

Milestone

4.0.0

Development

Successfully merging this pull request may close these issues.

Regressions from the new Base64 encoder

7 participants

@t8m @openssl-machine @mattcaswell @Sashan @esyr @nhorman @paulidale