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

Remove receiving of unused return value #23276

Closed
wants to merge 1 commit into from

Conversation

JohnnySavages
Copy link
Contributor

Found by Linux Verification Center (linuxtesting.org) with SVACE.

CLA: trivial

@mattcaswell mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: cleanup The issue/pr deals with cleanup of comments/docs not altering code significantly cla: trivial One of the commits is marked as 'CLA: trivial' labels Jan 12, 2024
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

Agree trivial

@mattcaswell mattcaswell added the tests: exempted The PR is exempt from requirements for testing label Jan 12, 2024
@tom-cosgrove-arm
Copy link
Contributor

Wouldn't a better fix be to actually check the result of BIO_snprintf() and behave (abort?) accordingly, rather than just ignore the error? We're going to perform file operations on the output of BIO_snprintf(), so if the buffer's aren't filled there could be some weird filenames!

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Jan 12, 2024
@JohnnySavages
Copy link
Contributor Author

Wouldn't a better fix be to actually check the result of BIO_snprintf() and behave (abort?) accordingly, rather than just ignore the error? We're going to perform file operations on the output of BIO_snprintf(), so if the buffer's aren't filled there could be some weird filenames!

Actually function cannot cause an "insufficient buffer" failure due to check higher. In the case of an incorrect string literal specifying the format, the use of the information specified in the buffer will fail, for which there is a check below.

Nevertheless, it might still be better to add a check for return codes. idk.
And if so, should I use assert everywhere?

@shahsb
Copy link
Contributor

shahsb commented Feb 12, 2024

Wouldn't a better fix be to actually check the result of BIO_snprintf() and behave (abort?) accordingly, rather than just ignore the error? We're going to perform file operations on the output of BIO_snprintf(), so if the buffer's aren't filled there could be some weird filenames!

I agree with @tom-cosgrove-arm

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

While it would definitely be better to add checks, rather than just omit them, the rest of the code in apps/ appears to be no better. Even in tests I see a TEST_true(BIO_snprintf(...)) (and that's insane, since BIO_snprintf() returns number of chars written or -1 for error, so is presumably almost never going to return zero).

So I'm now approving this PR as-is - any further fixing would need to have a wider scope

@tom-cosgrove-arm tom-cosgrove-arm 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 Mar 14, 2024
@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 Mar 15, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented Apr 4, 2024

Merged to the master branch. Thank you for your contribution.

@t8m t8m closed this Apr 4, 2024
openssl-machine pushed a commit that referenced this pull request Apr 4, 2024
CLA: trivial

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
(Merged from #23276)
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 cla: trivial One of the commits is marked as 'CLA: trivial' tests: exempted The PR is exempt from requirements for testing triaged: cleanup The issue/pr deals with cleanup of comments/docs not altering code significantly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants