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.

Sign up for GitHub

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

Jump to bottom

Move definition of BIO_FLAGS_UPLINK completely to bio.h #7307

Closed
wants to merge 4 commits into from
Closed

Move definition of BIO_FLAGS_UPLINK completely to bio.h #7307

wants to merge 4 commits into from

Conversation

bernd-edlinger
Copy link
Member

Move definition of BIO_FLAGS_UPLINK to bio.h

@bernd-edlinger bernd-edlinger added branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch labels Sep 24, 2018
@levitte
Copy link
Member

levitte commented Sep 24, 2018

This is really an internal macro. The right move is to make it completely internal, which is the contrary of what you do here.

@bernd-edlinger
Copy link
Member Author

Hmm...
You want to keep the definition of this macro dependent on which header file is included in which
sequence, for the next years to come?

@mspncp
Copy link
Contributor

mspncp commented Sep 24, 2018

I think it should have a fixed value, not a variable one.

Grepping the source, I see the following in bio_lcl.h:

#if BIO_FLAGS_UPLINK==0
/* Shortcut UPLINK calls on most platforms... */
...

So it looks like the only obstacle for making that constant really constant is that it is (mis-)used for short-circuiting the applink calls. I recall that @dot-asm made some comment recently about short-circuiting in certain circumstances and that it doesn't work correctly anymore. But I don't recall the details and didn't find the location. So maybe we should consult @dot-asm, he seems to be the only one who fully understands the applink beast...

@mspncp
Copy link
Contributor

mspncp commented Sep 24, 2018

I think it should have a fixed value, not a variable one.

And I agree with @levitte that it should not appear in public headers.

@mspncp
Copy link
Contributor

mspncp commented Sep 24, 2018 •
edited

I recall that @dot-asm made some comment recently about short-circuiting in certain circumstances ...

I found it: #7054 (comment)

@bernd-edlinger
Copy link
Member Author

If that helps, the public header could define BIO_FLAGS_UPLINK = 0,
which is used nowhere.
And internally we could rename BIO_FLAGS_UPLINK to BIO_FLAGS_UPLINK_INTERNAL.
And in we could remove BIO_FLAGS_UPLINK in the next major version,
whenever that will be.
Is there a way to deprecate a macro?

@bernd-edlinger
Keep the externally visible macro BIO_FLAGS_UPLINK in bio.h
dcbae40 
and rename the internally used macro to BIO_FLAGS_UPLINK_INTERNAL.

[extended tests]
bernd-edlinger
bernd-edlinger commented Sep 24, 2018
View reviewed changes
crypto/bio/bss_file.c Outdated
ret = UP_fread(out, 1, (int)outl, b->ptr);
else
ret = fread(out, 1, (int)outl, (FILE *)b->ptr);
if (ret == 0
&& (b->flags & BIO_FLAGS_UPLINK) ? UP_ferror((FILE *)b->ptr) :
ferror((FILE *)b->ptr)) {
&& (b->flags & BIO_FLAGS_UPLINK_INTERNAL)
Copy link
Member Author

Choose a reason for hiding this comment

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

Funny, I think the brackets are misplaced here.
isn't this effectively if ( (ret == 0 && b->flags & BIO_FLAGS_UPLINK) ? UP_ferror (b->ptr) : ferror(b->ptr) ) {

@mspncp
Copy link
Contributor

mspncp commented Sep 24, 2018

Is there a way to deprecate a macro?

That's what Pauli told me to do in a similar situation:

c402e94#diff-57bbffc095bd596a230ff36b8d197d4aR27

@levitte
Copy link
Member

levitte commented Sep 24, 2018

I see no reason to rename it internally, just deprecate it publically.

@bernd-edlinger
Copy link
Member Author

The reason is that BIO_FLAGS_UPLINK is defined as 0 when only bio.h is included
but it is defined differently if cryptlib.h and bio.h is included,
in 1.0.2 the defined value depends even on the order of the include statements.

@levitte
Copy link
Member

levitte commented Sep 24, 2018

Ah, so it's to ensure that none of our code uses the bio.h value... Ok gotcha.

bernd-edlinger added a commit to bernd-edlinger/openssl that referenced this pull request Sep 24, 2018
@bernd-edlinger
Keep the externally visible macro BIO_FLAGS_UPLINK in bio.h
4d5b500 
and rename the internally used macro to BIO_FLAGS_UPLINK_INTERNAL.
Deprecate the externally visible definition of BIO_FLAGS_UPLINK.

Backport of openssl#7307 to 110

[extended tests]
@bernd-edlinger bernd-edlinger mentioned this pull request Sep 24, 2018
Closed
bernd-edlinger added a commit to bernd-edlinger/openssl that referenced this pull request Sep 24, 2018
@bernd-edlinger
Keep the externally visible macro BIO_FLAGS_UPLINK in bio.h
cd0a425 
and rename the internally used macro to BIO_FLAGS_UPLINK_INTERNAL.

Backport of openssl#7307 to 110

[extended tests]
t8m
t8m approved these changes Jun 19, 2019
View reviewed changes
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.

Looks ok. Needs second review from @openssl/omc

@t8m t8m added the approval: review pending This pull request needs review by a committer label Jun 19, 2019
mattcaswell
mattcaswell reviewed Jun 19, 2019
View reviewed changes
include/openssl/bio.h Outdated Show resolved Hide resolved
@bernd-edlinger bernd-edlinger removed the branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch label Jun 19, 2019
@bernd-edlinger
Review comments
4d9f637 
[extended tests]
bernd-edlinger
bernd-edlinger commented Jun 19, 2019
View reviewed changes
crypto/bio/bss_file.c
&& (b->flags & BIO_FLAGS_UPLINK) ? UP_ferror((FILE *)b->ptr) :
ferror((FILE *)b->ptr)) {
&& (b->flags & BIO_FLAGS_UPLINK_INTERNAL
? UP_ferror((FILE *)b->ptr) : ferror((FILE *)b->ptr))) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, actually, this is a bug-fix.

@t8m
Copy link
Member

t8m commented Jun 26, 2019

ping?

@bernd-edlinger bernd-edlinger 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 Jun 26, 2019
@bernd-edlinger
Copy link
Member Author

Merged to master as b113279. Thanks!

levitte pushed a commit that referenced this pull request Jun 26, 2019
@bernd-edlinger
Keep the externally visible macro BIO_FLAGS_UPLINK in bio.h
b113279 
and rename the internally used macro to BIO_FLAGS_UPLINK_INTERNAL.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #7307)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@bernd-edlinger @levitte @mspncp @t8m @mattcaswell