Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect return check of BN_bn2nativepad #16943

Closed
wants to merge 2 commits into from

Conversation

PeiweiHu
Copy link
Contributor

@PeiweiHu PeiweiHu commented Nov 1, 2021

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

@kroeckx
Copy link
Member

kroeckx commented Nov 1, 2021

Could you update the documentation to say that BN_bn2lebinpad and BN_bn2nativepad also return -1 when the buffer is too small?

@PeiweiHu
Copy link
Contributor Author

PeiweiHu commented Nov 1, 2021

Could you update the documentation to say that BN_bn2lebinpad and BN_bn2nativepad also return -1 when the buffer is too small?

I put them together with BN_bn2binpad. It seems they invoke the same inner interface.

@kroeckx kroeckx added approval: review pending This pull request needs review by a committer branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch branch: 3.0 Merge to openssl-3.0 branch branch: master Merge to master branch labels Nov 1, 2021
@t8m t8m removed the branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch label Nov 1, 2021
@t8m t8m added the triaged: bug The issue/pr is/fixes a bug label Nov 1, 2021
@t8m
Copy link
Member

t8m commented Nov 1, 2021

This won't apply to 1.1.1. IMO it is also not worth backporting to 1.1.1 as the code fix is only for 3.0.

@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 Nov 1, 2021
@kroeckx
Copy link
Member

kroeckx commented Nov 1, 2021 via email

@t8m
Copy link
Member

t8m commented Nov 1, 2021

The documentation will apply to 1.1.1 too, I think

Yes, but IMO we are not aiming to fix all documentation deficiencies in 1.1.1.

@kroeckx
Copy link
Member

kroeckx commented Nov 1, 2021 via email

@t8m
Copy link
Member

t8m commented Nov 1, 2021

Anyway if someone submits a 1.1.1 PR, no problem.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

openssl-machine pushed a commit that referenced this pull request Nov 2, 2021
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #16943)
openssl-machine pushed a commit that referenced this pull request Nov 2, 2021
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #16943)
openssl-machine pushed a commit that referenced this pull request Nov 2, 2021
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #16943)

(cherry picked from commit 944fcfc)
openssl-machine pushed a commit that referenced this pull request Nov 2, 2021
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #16943)

(cherry picked from commit aedc5a8)
@t8m
Copy link
Member

t8m commented Nov 2, 2021

Merged to master and 3.0 branches. Thank you for the contribution.

@t8m t8m closed this Nov 2, 2021
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 Merge to master branch branch: 3.0 Merge to openssl-3.0 branch triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants