Declare stack in headers, not define them, when building openssl #10669
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
richsalz
changed the title
levitte
reviewed
Dec 21, 2019
Unfortunately, they are also the main issue that we are battling. As long as they aren't moved in a similar way, the issue I demonstrated in #10666. An idea is to move those DEFINE calls to |
richsalz
changed the title
|
I fixed STACK_OF OPENSSL_STRING and OPENSSL_CSTRING. I think the latter should be removed, but that's a side note. I made this a WIP until I get around to fixing the build issues. I think it's in ParseC. |
|
I [ahem] went in that fixed the ParseC problem. Small rearrangement of safestack.h included. |
|
I had safestack changed around on purpose :) Not worth arguing about. |
well, defining some DEFINE_STACK_OF macros only sometimes seemed wrong to me |
|
I only wanted to define what was actually used (to make it easier to remove this stuff in a future release). But like I said, it's not a big deal :) |
|
Rebased to resolve conflict and pushed. |
richsalz
changed the title
|
CI's passing, out of WIP, ready for review. As a reminder, this keeps backward compatibility with defining "static inline" functions in safestack.h, but also does the right thing if no-deprecated is configured so only the source files that need them define the functions. It does this by using the macro defined in #10666, so that should be approved as part of this approval. |
|
Proposed solution does not look correct ( build with --api=3.0.0 ). Quote from relevant configure check: |
|
"relevant configure check" I don't know what that is, it's not part of openssl. |
levitte
reviewed
Dec 29, 2019
This was referenced Dec 29, 2019
|
Rebased on master since it needs #10753 |
|
You need to fix |
|
I think the better fix is to move some "typedef struct .... ASN_xxx"; to types.h, like all the other ASN1 struct typedefs. Additional commit pushed. |
|
Rich Salz wrote:
"relevant configure check" I don't know what that is, it's not part of openssl.
hmm ! "not part of openssl" !?!?!?
My comment includes very simple C program code that shows build failure!
I understand that you have no idea how autoconf macros test
functionalities. It particular it look like
AC_LINK_IFELSE(
[AC_LANG_PROGRAM([[
#include <openssl/evp.h>
]], [[
EVP_add_cipher(NULL);
]])],
[ac_CRYPTO_LINK=:])
For details see
https://www.gnu.org/software/autoconf/manual/autoconf-2.61/html_node/Generating-Sources.html
Regards,
Roumen Petrov
|
|
Can you post what the small sample program is? I see a command line and a configure argument, but no failing C code. Thanks. |
|
@richsalz, the failing program in question is there, at the end of #10669 (comment), which is what @petrovr is trying to tell you. I'll quote it again here: #include <openssl/evp.h>
int
main ()
{
EVP_add_cipher(NULL);
return 0;
} |
|
I missed that, sorry. And thanks @petrovr. So config "--with-api=3.0" and try to compile that small program? Got it. |
|
Also try config with |
|
Works for me. more suggestions? |
|
P.S. issue with unknown type name ‘STACK’ is because system cannot
detect that OpenSSL supports function sk_OPENSSL_STRING_new_null .
As result of this fail back to earlier OpenSSL "STACK" implementation
which is missing in master.
With master detection is fine but with this enhancement is not.
|
|
Okay, so here is what I was thinking happens. This line at https://github.com/msftguy/openssh-sc/blob/master/ssh-ocsp.c#L65 is the problem, exposed by the configuration flags. You have "--api=3.0.0 no-deprecated" which means that the earlier API's, in particular, that the STACK functions are declared always in the header file, does not happen. From INSTALL: |
|
Hmmm, sk_STRING_new_null isn't defined as a macro. It was, a long time ago (I think that was before 1.0.0), so whatever code that's guarded by that ifdef should be irrelevant today, i.e old cruft that would be cleaned away if it was me... |
|
Rich Salz wrote:
Okay, so here is what I was thinking happens. This line
```
#ifdef sk_STRING_new_null
```
No the difference between master and this enhancement is on line
https://gitlab.com/secsh/pkixssh/blob/master/ssh-ocsp.c#L45
HAVE_SK_OPENSSL_STRING_NEW_NULL is not defined in this as "# check for
function sk_OPENSSL_STRING_new_null inlined in openssl 1.1+ ..." (
https://gitlab.com/secsh/pkixssh/blob/master/configure.ac#L5528 ) fail.
Perhaps is the same issue as already reported "...implicit declaration
of function ‘sk_X509_new_null’ [-Wimplicit-function-declaration] ..." .
Regards,
Roumen
P.S. work-around for low level AES was published, so project should
build fine against current master, i.e. config.log will show more
details. In few days I could review log.
|
|
I still think you are (a) testing for old code and (b) configuring openssl to not include old code. @levitte do you agree with me? |
|
updated commit pushed to fix "no-deprecated" build. |
|
rebased to address conflicts and pushed. |
|
Rebased to fix conflict in apps/pkcs12.c |
|
Unfortunately there is one more conflict now. |
|
Please see #11263 (comment) |
... and only *define* them in the source files that need them.
Use DEFINE_OR_DECLARE which is set appropriately for internal builds
and not non-deprecated builds.
Deprecate stack-of-block
Better documentation
Move some ASN1 struct typedefs to types.h
Update ParseC to handle this. Most of all, ParseC needed to be more
consistent. The handlers are "recursive", in so far that they are called
again and again until they terminate, which depends entirely on what the
"massager" returns. There's a comment at the beginning of ParseC that
explains how that works. {Richard Levtte}
|
rebased. added some missing cases. i forgot about my promise to not rebase again. just like the project forgets to look at PR's :) |
t8m
approved these changes
Apr 23, 2020
beldmit
approved these changes
Apr 23, 2020
t8m
added
approval: done
|
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
Apr 24, 2020
... and only *define* them in the source files that need them.
Use DEFINE_OR_DECLARE which is set appropriately for internal builds
and not non-deprecated builds.
Deprecate stack-of-block
Better documentation
Move some ASN1 struct typedefs to types.h
Update ParseC to handle this. Most of all, ParseC needed to be more
consistent. The handlers are "recursive", in so far that they are called
again and again until they terminate, which depends entirely on what the
"massager" returns. There's a comment at the beginning of ParseC that
explains how that works. {Richard Levtte}
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #10669)
t8m
added
approval: ready to merge
|
Merged to master. Thank you, Rich, for your cleanup work. |
|
Thanks for not giving up on this. |
|
This seems to have broken stuff. See failing build log here: |
|
@mattcaswell, see #11655 |
I suspect #11655 is not sufficient. The oss-fuzz build log above is complaining about issues in srp.h. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This uses #10666 so that when building OpenSSL, no STACK_OF "static inline functions" are defined. This has a couple of benefits:
I did not change the STRING stacks, since they're too pervasive.