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

Get rid of warn_binary #16367

Closed
wants to merge 1 commit into from
Closed

Get rid of warn_binary #16367

wants to merge 1 commit into from

Conversation

beldmit
Copy link
Member

@beldmit beldmit commented Aug 20, 2021

Current implementation of warn_binary introduces a regression
when the content is passed in /dev/stdin as an explicit file name
and reads the file to be processed twice otherwise.

I suggest to reimplement this functionality after 3.0 if necessary.

Fixes #16359

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

Current implementation of warn_binary introduces a regression
when the content is passed in /dev/stdin as an explicit file name
and reads the file to be processed twice otherwise.

I suggest to reimplement this functionality after 3.0 if necessary.

Fixes openssl#16359
@beldmit beldmit added approval: review pending This pull request needs review by a committer branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug labels Aug 20, 2021
@beldmit beldmit added this to the 3.0.0 milestone Aug 20, 2021
@beldmit beldmit requested a review from DDvO August 20, 2021 15:09
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 do not agree with entirely removing the warn_binary function.
Yet sure stdin should not be read twice.
Instead, the function can simply check for the input file being /dev/stdin and return immediately in this case.

@beldmit
Copy link
Member Author

beldmit commented Aug 21, 2021

I think no files should be read twice - and check for stdin is not the right way to fix the issue. If we have a really large file, why are we to read it twice?

The right way looks to me like set some flag about possible binary content during reading the file and report afterwards. But this also looks like a post-3.0 solution

@DDvO
Copy link
Contributor

DDvO commented Aug 21, 2021

I think no files should be read twice - and check for stdin is not the right way to fix the issue. If we have a really large file, why are we to read it twice?

Good point that reading a (large) file twice is not a good solution.

The right way looks to me like set some flag about possible binary content during reading the file and report afterwards.

Yes.

But this also looks like a post-3.0 solution

I agree. Please open an issue, pointing to this discussion, such that this point is not forgotten,
and then I'll be fine with removing the warn_binary() function.

@beldmit
Copy link
Member Author

beldmit commented Aug 21, 2021

@DDvO, please - #16372

@paulidale
Copy link
Contributor

Instead, the function can simply check for the input file being /dev/stdin and return immediately in this case.

This doesn't exist on some platforms.

Reading via a memory BIO and checking the first bytes for binary might be an option. Otherwise @beldmit's suggestion of a flag that is set when something is encountered (filter BIO perhaps?). Finally, the command does know if it has been told to read stdin and could skip the check in this case -- not ideal but possibly okay enough.

@beldmit beldmit 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 Aug 21, 2021
@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually.

openssl-machine pushed a commit that referenced this pull request Aug 22, 2021
Current implementation of warn_binary introduces a regression
when the content is passed in /dev/stdin as an explicit file name
and reads the file to be processed twice otherwise.

I suggest to reimplement this functionality after 3.0 if necessary.

Fixes #16359

Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #16367)
@beldmit
Copy link
Member Author

beldmit commented Aug 22, 2021

Merged. Thanks!

@beldmit beldmit closed this Aug 22, 2021
@beldmit beldmit deleted the fix_16359 branch September 21, 2021 12:20
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 triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CMS processing from stdin
4 participants