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

OpenSSL 1.1.1a: Fixing static inline issue with SunCC #7721

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
9 participants
@makr
Copy link
Contributor

commented Nov 28, 2018

Work around a failing 'make test' with Oracle Developer Studio 12.5 and a configured OpenSSL debug build. Marks all mentioned symbols as weak.
Partly fixes #6912.

@kroeckx

This comment has been minimized.

Copy link
Member

commented Dec 8, 2018

Should we put that in safestack.h instead?

@makr

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2018

Hm, I see. At the end? The way I have it defined in rsa_complex.c now?
I'll try to get to it next week ...

makr added a commit to makr/openssl that referenced this pull request Jan 18, 2019

openssl#7721, openssl#6912 related: correction after comment from @kr…
…oeckx.

Moved "weak" declaration of symbols into the respective header file.

Signed-off-by: Matthias Kraft <Matthias.Kraft@softwareag.com>
@makr

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

sigh ... shouldn't have rebased the branch, it seems.

@levitte

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

You could do a git rebase -i OpenSSL_1_1_1-stable, which will get you a TODO list in an editor. Remove all the "pick" lines apart from the last (commit id 23dcef5), save and exit. Rebase will do the rest. Finish up with a git push -f.

If you want to be extra safe, you can preserve the current contents of openssl-6912 by creating a local branch, with git branch foo before doing the rebase described above. When you feel safe that the rebase did the right thing, drop this branch with git branch -D foo

@makr makr force-pushed the makr:openssl-6912 branch from 23dcef5 to 2a6c9dc Jan 24, 2019

makr added a commit to makr/openssl that referenced this pull request Jan 24, 2019

openssl#7721, openssl#6912 related: correction after comment from @kr…
…oeckx.

Moved "weak" declaration of symbols into the respective header file.

Signed-off-by: Matthias Kraft <Matthias.Kraft@softwareag.com>
@makr

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2019

@levitte Thanks heaps!
@blastwave Let me guess, the pragmas? Do you have some output from the compiler?

@levitte

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

So.... am I to assume that ODS 12.5 (and probably other versions too) simply ignores inline in some situations (such as debugging), or is there a deeper issue here? Note that ossl_inline may have an empty value as a last resort, which would lead to this kind of situation as well, I just want to double check that isn't the issue here, 'cause in that case, we might want to adjust include/openssl/e_os2.h.

I know there's been talk about this stuff elsewhere, and I haven't followed all of it, so it's possible this has already been answered...

Either way, I assume this will be an issue at any time ossl_inline is used in public headers. It would seem to me that if we're doing this in include/openssl/safestack.h, we should do the same thing in include/openssl/lhash.h, where ossl_inline is used as generously. And we will need to remember this for any future place where ossl_inline is used, right?
For completeness, I would like to see a similar change in include/openssl/lhash.h in this PR.

@makr

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2019

Some background is at #6912 .
I'll have a look into lhash.h, too. Hopefully tomorrow.
When I first stumbled over the issue I found a blog post, were the SunCC maintainers mused over static inline. Unfortunately I cannot find the link again and my Google foo seems to leave me, too. ... Ah, found it: https://blogs.oracle.com/d/inline-functions-in-c
In there is a reference to another blog-post, that is only accessible through the Wayback-Machine. The applicable paragraph says:

For static inline functions it is simple. Either a function defined with the inline function specifier is inlined at a reference or a call is made to the actual function. The compiler can choose which to do at each reference. The Sun C compiler decides if it is profitable to inline at -xO3 and above. When not profitable to inline, or at an optimization of less than -xO3 a reference to the actual function will be generated. If any reference to the actual function is generated the function definition will be generated in the object code. Note if the address of the function is taken, the actual function will be generated in the object code.

@levitte

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

Thanks for all the links, and especially the musing was a good read.

makr added a commit to makr/openssl that referenced this pull request Jan 25, 2019

makr added some commits Nov 28, 2018

Work around a failing 'make test' with Oracle Developer Studio 12.5 a…
…nd an

configured debug build. Marks all mentioned symbols as weak.
See #6912.
#7721, #6912 related: correction after comment from @kroeckx.
Moved "weak" declaration of symbols into the respective header file.

Signed-off-by: Matthias Kraft <Matthias.Kraft@softwareag.com>
#7721, #6912: Declared OPENSSL_LH-symbols as weak as well.
And reformated change in safestack.h.

@makr makr force-pushed the makr:openssl-6912 branch from efe7871 to 730899f Jan 25, 2019

@mspncp

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

When I first stumbled over the issue I found a blog post, were the SunCC maintainers mused over static inline. Unfortunately I cannot find the link again and my Google foo seems to leave me, too. ... Ah, found it: https://blogs.oracle.com/d/inline-functions-in-c

Sigh! In C, even simple things can be so complicated and unintuitive that you need a blog post to fully understand it ;-). For example, the following is highly unintuitive to me and I would consider it a bug, not a feature:

The code provides an inline definition of foo(), so if the compiler chooses to inline the function it will use this definition. However, if it chooses not to inline the function, you will get a link time error when the linker is unable to find a suitable definition for the symbol.

Thanks for the link, @makr.

makr added a commit to makr/openssl that referenced this pull request Jan 25, 2019

openssl#7721, openssl#6912 related: correction after comment from @kr…
…oeckx.

Moved "weak" declaration of symbols into the respective header file.

Signed-off-by: Matthias Kraft <Matthias.Kraft@softwareag.com>
(cherry picked from commit f6f695a)

makr added a commit to makr/openssl that referenced this pull request Jan 25, 2019

openssl#7721, openssl#6912: Declared OPENSSL_LH-symbols as weak as well.
And reformated change in safestack.h.

(cherry picked from commit 730899f)

@makr makr changed the title OpenSSL 1.1.1a: Work around a failing 'make test' with Oracle Developer Studio 12.5 OpenSSL 1.1.1a: Fixing static inline issue with SunCC Jan 25, 2019

@levitte

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

Actually, considering inline is just a hint, and that one external version of a inline function must exist (for the purpose of being able to get the function address, for example), what SunPro CC does makes perfect sense. I believe we got away without noticing this issue because GCC and derivates of it remove dead code, so an unused inline "disappears".
(I'm actually surprised the VMS compiler hasn't complained, it usually does in cases like this)

@kroeckx

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

@t-j-h

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

I think inline functions in public header files simply shouldn't be present.

@mattcaswell

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

I think inline functions in public header files simply shouldn't be present.

I agree

@levitte

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

I'm ambivalent... in the case of stack and lhash, the generated functions we made static inline expressly to get better C type safety, and to get away from the mkstack.pl horror.

411abf2

I guess there are pros and cons, question is what outweighs what...

@levitte

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

#8087 has now been merged and cherry-picked to OpenSSL_1_1_1-stable as well. Of this PR, only the fixup of test/rsa_complex.c remains, so this PR is still relevant.

@levitte

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

Never mind that, I was still in the middle of a rebase when I looked... so it turns out that this PR isn't relevant any more. Closing it.

@levitte levitte closed this Jan 27, 2019

@vdukhovni

This comment has been minimized.

Copy link

commented Jan 27, 2019

I think that static inline functions in headers are fine used judiciously, and users can use sufficiently modern compilers. As we discover compilers that don't play along, if it is possible to add a work-around via e_os2.h, we should do that, and if not, https://dilbert.com/strip/1995-06-24 (s/computer/compiler/).

@vdukhovni

This comment has been minimized.

Copy link

commented Jan 27, 2019

For the specific issue of wanting to dynamically load libcrypto, in code that does not in fact use the STACK and LHASH functions, we could provide a macro that such applications can define that conditionally skips defining the inline functions for users who don't need them, and consume the header files without linking with libcrypto at compile-time.

@blastwave

This comment has been minimized.

Copy link

commented Jan 27, 2019

My grey beard and suspenders here are feeling that "inline" should be "do this fast" along with a hint or gentle sugestion to the compiler to avoid stack push call pop Nastia Liukin function "nastics". However even the so called "C" standard claims that "inline" and "extern" stuff may just be "unspecified behavior" and let's just agree that it would be nice if it all happened inside registers and never a cache line miss is heard from. I dunno. The Oracle Studio 12.6 compiler is very very new and very very standards compliant. Says so right on the tin. Maybe we should just do these things in Fortran 77 and then copy and paste the opcodes around. Tempest in a tea cup is what I am thinking.

@levitte

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

An option is to move away these lines from safestack.h and lhash.h, as they are the actual culprit:

DEFINE_SPECIAL_STACK_OF(OPENSSL_STRING, char)
DEFINE_SPECIAL_STACK_OF_CONST(OPENSSL_CSTRING, char)
DEFINE_SPECIAL_STACK_OF(OPENSSL_BLOCK, void)

DEFINE_LHASH_OF(OPENSSL_STRING);
DEFINE_LHASH_OF(OPENSSL_CSTRING);
@vdukhovni

This comment has been minimized.

Copy link

commented Jan 27, 2019

No objection to moving the specific culprits out of the way, if that is not expected to break user code.

@blastwave

This comment has been minimized.

Copy link

commented Jan 27, 2019

Isn't this a "closed" issue with a bad title?

@levitte

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

Well, if anyone uses those specific STACK and LHASH types, they will have to include an extra header file. So there's the choice to make, either a controlled breakage for the few that can be documented, or the surprise breakage for those using compilers that do not aggressively remove dead code.

@levitte

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

Isn't this a "closed" issue with a bad title?

Uhmmm, the title is fine.
Yeah, it's closed, but it turns out there's more to say about the affected header files... We should really raise a new issue

@kroeckx

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

@blastwave

This comment has been minimized.

Copy link

commented Jan 27, 2019

I was thinking the same thing .. however if it makes reference to a modern commercial compiler then let's call it "Oracle Studio 12.6".

@paulidale

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

We shouldn't need to include an extra header file (which breaks the API). We can resolve this without by losing the inline definitions and making them out of line.

@vdukhovni

This comment has been minimized.

Copy link

commented Jan 27, 2019

The ability to generate (as opposed to declare prototypes for) inline functions for user-defined stacks and hashes has to stay, that's also part of the API, since some users may have their own header files that make use of the macros in question. What we're perhaps free to do is switch our pre-defined OpenSSL stacks and hashes from static inline to exported functions in the library. With the proviso that code built against 1.1.1b headers and libraries won't run against 1.1.1a and earlier, but we already have that for other reasons.

@paulidale

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

Yeah, that's what I meant. Keep the DEFINE macros in the headers but use them only in .c files.

This would break ABI compatibility but not API.

@vdukhovni

This comment has been minimized.

Copy link

commented Jan 27, 2019

Well, it would break ABI only when moving back from 1.1.1b to 1.1.1a, right? Or do you also see a break going from 1.1.1a to 1.1.1b?

@blastwave

This comment has been minimized.

Copy link

commented Jan 27, 2019

Someone reasonable open a new issue and follow along there. This one is status closed.

@paulidale

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

I can't think of how there would be a break going from 1.1.1a to 1.1.1b but I've been stung by weird shared library behaviour in the past. Paranoia is like a comfort pillow :)

@paulidale

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

Thinking further...

Changing away from static functions will break the API. A program that uses a DEFINE macro in one of its headers will need to be changed so that only one instance of the functions is created.

Sigh.

@vdukhovni

This comment has been minimized.

Copy link

commented Jan 28, 2019

No, the DEFINE macros must stay the same, producing static inline functions suitable for inclusion in headers. For generating actual non-inline macros in C source files, we'd need a different form of the macros to generate exportable non-inline C functions.

@paulidale

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

Which means duplicating their definitions or playing preprocessor games. I favour the latter but don't really like it.

@vdukhovni

This comment has been minimized.

Copy link

commented Jan 28, 2019

Yes, with some pre-processor games we can continue to export the macros that generate custom user lhashes and stacks, while exporting only extern declarations for the built-in stacks and lhashes for the OpenSSL library, and using alternative forms of the macros to create new public functions compiled into libcrypto.

The API remains the same. The ABI expands from 1.1.1a to 1.1.1b, which means you can't go back, but we have that already to some degree.

@levitte

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

The definition of the DEFINE macros should stay in place, for sure. But the calls of those macros in those same header files? I remain dubious. Declaring that those functions exist is a possibility, but in that case, they must be made non-static.

Actually, that may solve the issue quite well...

@levitte

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

I'm raising an issue on this

@levitte

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

See #8102

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.