Skip to content

Conversation

@nak3
Copy link
Contributor

@nak3 nak3 commented Apr 15, 2025

Currently, the code asserts that the return value of BIO_write() does not exceed n bytes. However, other calls to BIO_write() do not perform such a check.

Since there is no special handling in b64_write()
that would require this assertion, it is safe to remove it.

Checklist
  • documentation is added or updated
  • tests are added or updated

P.S. I noticed this assertion while reading the code. Although it has not caused any issues, it led me to check whether there are any cases where BIO_write() might return i > n.

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Apr 15, 2025
@nak3 nak3 closed this Apr 16, 2025
@nak3 nak3 reopened this Apr 16, 2025
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

It would actually make sense to replace all the OPENSSL_assert() calls in this file with proper ossl_assert() calls guarded by ifs and returning errors.

@t8m t8m added branch: master Merge to master branch triaged: refactor The issue/pr requests/implements refactoring labels Apr 16, 2025
@nak3 nak3 closed this Apr 16, 2025
@nak3 nak3 reopened this Apr 16, 2025
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Apr 16, 2025
Currently, the code asserts that the return value of
BIO_write() does not exceed n bytes. However, other
calls to BIO_write() do not perform such a check.

Since there is no special handling in b64_write()
that would require this assertion, it is safe to remove it.
@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Apr 29, 2025
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Apr 29, 2025
@nak3
Copy link
Contributor Author

nak3 commented Apr 29, 2025

Thank you! Updated.

@t8m t8m added approval: review pending This pull request needs review by a committer tests: exempted The PR is exempt from requirements for testing labels Apr 29, 2025
t8m
t8m previously approved these changes Apr 29, 2025
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

CI failures aren't relevant. Commits to be squashed when merging.

@t8m t8m 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 Apr 29, 2025
@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 Apr 30, 2025
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Apr 30, 2025
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #27401)
@t8m
Copy link
Member

t8m commented Apr 30, 2025

Squashed and merged to the master branch. Thank you for your contribution.

@t8m t8m closed this Apr 30, 2025
DDvO pushed a commit to siemens/openssl that referenced this pull request Jun 16, 2025
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#27401)
MichaelA-Fireblocks pushed a commit to MichaelA-Fireblocks/openssl that referenced this pull request Jul 15, 2025
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#27401)
MichaelA-Fireblocks pushed a commit to MichaelA-Fireblocks/openssl that referenced this pull request Jul 15, 2025
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#27401)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch tests: exempted The PR is exempt from requirements for testing triaged: refactor The issue/pr requests/implements refactoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants