Re: [openssl-project] inline functions

2019-01-28 Thread Benjamin Kaduk
On Mon, Jan 28, 2019 at 07:10:55AM +0100, Richard Levitte wrote:
> On Mon, 28 Jan 2019 06:17:35 +0100,
> Dr Paul Dale wrote:
> > Richard wrote:
> > 
> > Not really, since they are static inline. This is by design, that for 
> > any file you want to use
> > a safestack in, you just start with a DEFINE_ line. The mistake we did 
> > was to leave a few
> > common ones in the safestack header file. (same thing for lhash) 
> > 
> > Which means we’ve a compatibility issue.  The functions are in a public 
> > header, they can be used
> > by any application.  We need to continue supporting such use.
> > Asking a user to add a DEFINE_ line is API breaking.
> > 
> > I would be pro making such a change but we’d need to accept the 
> > consequences.
> 
> We have to accept consequences either way, either:
> 
> 1. the surprise breakage if someone includes  but
>doesn't link with libcrypto, while compiling with
>-fkeep-inline-functions (explicitly or implicitly, depending on the
>compiler)

This one is only "surprising and new" the first time a user/project tries
to turn on -fkeep-inline-functions.

> 2. The controlled and documented change / breakage that they will have
>to either add those DEFINE lines where they need the functionality,
>or include another header file with common stack / lhash type
>implementations (with the caveat that they MUST link with libcrypto
>if they use those headers)

This one is "surprising and new" to everyone using the stuff (i.e., more
people).

-Ben
___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project

Re: [openssl-project] inline functions

2019-01-27 Thread Richard Levitte
On Mon, 28 Jan 2019 06:17:35 +0100,
Dr Paul Dale wrote:
> Richard wrote:
> 
> Not really, since they are static inline. This is by design, that for any 
> file you want to use
> a safestack in, you just start with a DEFINE_ line. The mistake we did 
> was to leave a few
> common ones in the safestack header file. (same thing for lhash) 
> 
> Which means we’ve a compatibility issue.  The functions are in a public 
> header, they can be used
> by any application.  We need to continue supporting such use.
> Asking a user to add a DEFINE_ line is API breaking.
> 
> I would be pro making such a change but we’d need to accept the consequences.

We have to accept consequences either way, either:

1. the surprise breakage if someone includes  but
   doesn't link with libcrypto, while compiling with
   -fkeep-inline-functions (explicitly or implicitly, depending on the
   compiler)

2. The controlled and documented change / breakage that they will have
   to either add those DEFINE lines where they need the functionality,
   or include another header file with common stack / lhash type
   implementations (with the caveat that they MUST link with libcrypto
   if they use those headers)

Cheers,
Richard

-- 
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/
___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project

Re: [openssl-project] inline functions

2019-01-27 Thread Dr Paul Dale
Richard wrote:

> Not really, since they are static inline. This is by design, that for any 
> file you want to use a safestack in, you just start with a DEFINE_ line. The 
> mistake we did was to leave a few common ones in the safestack header file. 
> (same thing for lhash) 

Which means we’ve a compatibility issue.  The functions are in a public header, 
they can be used by any application.  We need to continue supporting such use.
Asking a user to add a DEFINE_ line is API breaking.

I would be pro making such a change but we’d need to accept the consequences.


Pauli
-- 
Dr Paul Dale | Cryptographer | Network Security & Encryption 
Phone +61 7 3031 7217
Oracle Australia

___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project

Re: [openssl-project] inline functions

2019-01-27 Thread Richard Levitte


Dr Paul Dale  skrev: (27 januari 2019 23:18:06 CET)
>Isn’t this going to run into the same issues?

No. It's the expansion of the macros that causes the problem, not the macros 
themselves. So if you move the expansions to the .c that actually use stacks / 
lhashes, they should obviously be linked with libcrypto, problem solved. 

>The DEFINE_ expansions need to be in their own .c file (one file with
>all or one file per, I’m not fussed much but the latter seems nicer).

Not really, since they are static inline. This is by design, that for any file 
you want to use a safestack in, you just start with a DEFINE_ line. The mistake 
we did was to leave a few common ones in the safestack header file. (same thing 
for lhash) 

>The DEFINE_ macros need to stay in the header for compatibility.

Yup. 

>To handle the external linkage, a new macro that declares the function
>prototypes should be introduced.  I.e.:

Again, no need because of the static nature of the resulting inline functions. 

>
># define DECLARE_LHASH_OF(type) \
>LHASH_OF(type) * ln_##type##_new(unsigned long (*)(const type *), int
>(*)(const type *, const type *)); \
>void lh_##type##_free(LHASH_OF(type) *); \
>…
>
># define DEFINE_LHASH_OF(type) \
>DECLARE LHASH_OF(type); \
>LHASH_OF(type) { union lh_##type##_dummy { void* d1; unsigned long d2;
>int d3; } dummy; }; \
>static ossl_inline LHASH_OF(type) * \
>lh_##type##_new(unsigned long (*hfn)(const type *), \
>int (*cfn)(const type *, const type *)) \
>{ \
>return (LHASH_OF(type) *) \
>  OPENSSL_LH_new((OPENSSL_LH_HASHFUNC)hfn, (OPENSSL_LH_COMPFUNC)cfn); \
>} \
>static ossl_inline void lh_##type##_free(LHASH_OF(type) *lh) \
>{ \
>OPENSSL_LH_free((OPENSSL_LHASH *)lh); \
>} \
>…
>
>The headers can then use the DECLARE_LHASH_OF macro to prototype the
>functions.  The .c file uses the DEFINE_LHASH_OF macro to create them.
>
>I chose lhash here because it is the simpler of the two, safestack has
>more options and is a bit more convoluted.  I’m willing to make a stab
>at a PR for this.
>
>
>Pauli

-- 
Skickat från min Android-enhet med K-9 Mail. Ursäkta min fåordighet.
___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project

Re: [openssl-project] inline functions

2019-01-27 Thread Dr Paul Dale
Isn’t this going to run into the same issues?

The DEFINE_ expansions need to be in their own .c file (one file with all or 
one file per, I’m not fussed much but the latter seems nicer).

The DEFINE_ macros need to stay in the header for compatibility.  To handle the 
external linkage, a new macro that declares the function prototypes should be 
introduced.  I.e.:

# define DECLARE_LHASH_OF(type) \
LHASH_OF(type) * ln_##type##_new(unsigned long (*)(const type *), int 
(*)(const type *, const type *)); \
void lh_##type##_free(LHASH_OF(type) *); \
…

# define DEFINE_LHASH_OF(type) \
DECLARE LHASH_OF(type); \
LHASH_OF(type) { union lh_##type##_dummy { void* d1; unsigned long d2; int 
d3; } dummy; }; \
static ossl_inline LHASH_OF(type) * \
lh_##type##_new(unsigned long (*hfn)(const type *), \
int (*cfn)(const type *, const type *)) \
{ \
return (LHASH_OF(type) *) \
OPENSSL_LH_new((OPENSSL_LH_HASHFUNC)hfn, (OPENSSL_LH_COMPFUNC)cfn); 
\
} \
static ossl_inline void lh_##type##_free(LHASH_OF(type) *lh) \
{ \
OPENSSL_LH_free((OPENSSL_LHASH *)lh); \
} \
…

The headers can then use the DECLARE_LHASH_OF macro to prototype the functions. 
 The .c file uses the DEFINE_LHASH_OF macro to create them.

I chose lhash here because it is the simpler of the two, safestack has more 
options and is a bit more convoluted.  I’m willing to make a stab at a PR for 
this.


Pauli
-- 
Dr Paul Dale | Cryptographer | Network Security & Encryption 
Phone +61 7 3031 7217
Oracle Australia



> On 28 Jan 2019, at 4:56 am, Richard Levitte  wrote:
> 
> On Sun, 27 Jan 2019 18:51:58 +0100,
> Viktor Dukhovni wrote:
>> 
>>> On Jan 27, 2019, at 5:33 AM, Tim Hudson  wrote:
>>> 
>>> Tim - I think inline functions in public header files simply shouldn't be 
>>> present.
>> 
>> I think they have their place, and we should try to make them more portable
>> to less capable toolchains as needed.
> 
> A simple way to handle it is to move the DEFINE_ expansions to another
> header file.  I suggest common_stacks.h and common_lhashes.h
> 
> At the very least for 3.0.0.  For 1.1.1, I have no idea...
> 
> Cheers,
> Richard
> 
> -- 
> Richard Levitte levi...@openssl.org
> OpenSSL Project http://www.openssl.org/~levitte/
> ___
> openssl-project mailing list
> openssl-project@openssl.org
> https://mta.openssl.org/mailman/listinfo/openssl-project

___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project

Re: [openssl-project] inline functions

2019-01-27 Thread Richard Levitte
On Sun, 27 Jan 2019 18:51:58 +0100,
Viktor Dukhovni wrote:
> 
> > On Jan 27, 2019, at 5:33 AM, Tim Hudson  wrote:
> > 
> > Tim - I think inline functions in public header files simply shouldn't be 
> > present.
> 
> I think they have their place, and we should try to make them more portable
> to less capable toolchains as needed.

A simple way to handle it is to move the DEFINE_ expansions to another
header file.  I suggest common_stacks.h and common_lhashes.h

At the very least for 3.0.0.  For 1.1.1, I have no idea...

Cheers,
Richard

-- 
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/
___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project


Re: [openssl-project] inline functions

2019-01-27 Thread Viktor Dukhovni
> On Jan 27, 2019, at 5:33 AM, Tim Hudson  wrote:
> 
> Tim - I think inline functions in public header files simply shouldn't be 
> present.

I think they have their place, and we should try to make them more portable
to less capable toolchains as needed.

-- 
Viktor.

___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project


Re: [openssl-project] inline functions

2019-01-27 Thread Bernd Edlinger
./config -fkeep-inline-functions && make
-> build fails with unresolved externals in test/rsa_complex and 
test/shlibloadtest

On 1/27/19 2:23 PM, Dr Paul Dale wrote:
> Yes, those are the problematic cases.  I think that making the symbols weak 
> is “good enough” for the moment.  Longer term, we could do with a better 
> solution.  Moving the implementations into another file is one option.  There 
> is another longer term alternative: migrate OpenSSL away from lhash and 
> safestack by introducing new internal functionality that replaces them.  We 
> can’t remove either in case users user them but we could stop using them 
> ourselves.
> 
> Lhash is based on a clever hash table that dynamically resizes (both 
> increasing and decreasing) and amortises the rehashing costs over time.  If 
> the OpenSSL source code is looked through, there are relatively few removals 
> from the hash table.  I.e. the size decrease isn’t used much, if at all.  
> Likewise, the rehashing checks one bit in the hash for each item and moves it 
> into one of two lists based based on the result.  I.e. rehashing should be a 
> fast operation and amortising it doesn’t feel like a win.  On the down side, 
> lhash runs with a fairly high level of loading (many entries relative to 
> table size) and its collision resolution is a linked list (i.e. O(n) worst 
> case instead of O(1)).  There have also been improvements in hash technology 
> since the lhash algorithm was created — cache coherent algorithms and lock 
> free ones spring to mind.
> 
> Safestack isn’t a stack anymore.  It is used as a vector, an array 
> substitute, a queue and more.  I don’t think I’ve seen it used as a stack but 
> it probably is.  We should have separate data structures for the different 
> uses, each optimised for its specific usage.
> 
> 
> This would be a long path (and I’m hijacking this thread a bit), but it is 
> something I’ve been wanting to do for a while now.
> 
> 
> Pauli
> 
> 
> ___
> openssl-project mailing list
> openssl-project@openssl.org
> https://mta.openssl.org/mailman/listinfo/openssl-project
> 
___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project

Re: [openssl-project] inline functions

2019-01-27 Thread Dr Paul Dale
Yes, those are the problematic cases.  I think that making the symbols weak is 
“good enough” for the moment.  Longer term, we could do with a better solution. 
 Moving the implementations into another file is one option.  There is another 
longer term alternative: migrate OpenSSL away from lhash and safestack by 
introducing new internal functionality that replaces them.  We can’t remove 
either in case users user them but we could stop using them ourselves.

Lhash is based on a clever hash table that dynamically resizes (both increasing 
and decreasing) and amortises the rehashing costs over time.  If the OpenSSL 
source code is looked through, there are relatively few removals from the hash 
table.  I.e. the size decrease isn’t used much, if at all.  Likewise, the 
rehashing checks one bit in the hash for each item and moves it into one of two 
lists based based on the result.  I.e. rehashing should be a fast operation and 
amortising it doesn’t feel like a win.  On the down side, lhash runs with a 
fairly high level of loading (many entries relative to table size) and its 
collision resolution is a linked list (i.e. O(n) worst case instead of O(1)).  
There have also been improvements in hash technology since the lhash algorithm 
was created — cache coherent algorithms and lock free ones spring to mind.

Safestack isn’t a stack anymore.  It is used as a vector, an array substitute, 
a queue and more.  I don’t think I’ve seen it used as a stack but it probably 
is.  We should have separate data structures for the different uses, each 
optimised for its specific usage.


This would be a long path (and I’m hijacking this thread a bit), but it is 
something I’ve been wanting to do for a while now.


Pauli
-- 
Dr Paul Dale | Cryptographer | Network Security & Encryption 
Phone +61 7 3031 7217
Oracle Australia



> On 27 Jan 2019, at 10:58 pm, Richard Levitte  wrote:
> 
> You're talking about these lines from safestack.h:
> 
>DEFINE_SPECIAL_STACK_OF(OPENSSL_STRING, char)
>DEFINE_SPECIAL_STACK_OF_CONST(OPENSSL_CSTRING, char)
>DEFINE_SPECIAL_STACK_OF(OPENSSL_BLOCK, void)
> 
> and these from lhash.h:
> 
>DEFINE_LHASH_OF(OPENSSL_STRING);
>DEFINE_LHASH_OF(OPENSSL_CSTRING);
> 
> I didn't think of those when looking at the PR, but you're entirely
> correct that they are the direct cause of the issue, and should move
> to someplace internal.
> 
> Cheers,
> Richard
> 
> On Sun, 27 Jan 2019 12:30:07 +0100,
> Dr Paul Dale wrote:
>> 
>> 
>> I’d generally prefer functions over macros — I think that the ctrl calls 
>> e.g. would be better
>> wrapped with function to provide type checking.
>> The overhead of a function call is pretty light these days so inline 
>> functions are difficult to
>> justify (as anything except a premature optimisation?).
>> 
>> Both safestack and lhash are problematic cases.  The inline functions come 
>> from macros which I
>> view as okay.  The problem is that some of these macros are expanded in the 
>> header for common
>> cases (e.g. stack of stings).  We could address this by distinguishing 
>> between the function
>> declarations and their instantiation and move the latter into its own C file.
>> 
>> Pauli
>> -- 
>> Dr Paul Dale | Cryptographer | Network Security & Encryption 
>> Phone +61 7 3031 7217
>> Oracle Australia
>> 
>>On 27 Jan 2019, at 8:33 pm, Tim Hudson  wrote:
>> 
>>From https://github.com/openssl/openssl/pull/7721
>> 
>>Tim - I think inline functions in public header files simply shouldn't be 
>> present.
>>Matt - I agree
>>Richard - 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.
>> 
>>It would be good to get a sense of the collective thoughts on the topic.
>> 
>>Tim.
>> 
>>___
>>openssl-project mailing list
>>openssl-project@openssl.org
>>https://mta.openssl.org/mailman/listinfo/openssl-project
>> 
>> 
>> ___
>> openssl-project mailing list
>> openssl-project@openssl.org
>> https://mta.openssl.org/mailman/listinfo/openssl-project
> -- 
> Richard Levitte levi...@openssl.org
> OpenSSL Project http://www.openssl.org/~levitte/
> ___
> openssl-project mailing list
> openssl-project@openssl.org
> https://mta.openssl.org/mailman/listinfo/openssl-project

___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project

Re: [openssl-project] inline functions

2019-01-27 Thread Richard Levitte
You're talking about these lines from safestack.h:

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

and these from lhash.h:

DEFINE_LHASH_OF(OPENSSL_STRING);
DEFINE_LHASH_OF(OPENSSL_CSTRING);

I didn't think of those when looking at the PR, but you're entirely
correct that they are the direct cause of the issue, and should move
to someplace internal.

Cheers,
Richard

On Sun, 27 Jan 2019 12:30:07 +0100,
Dr Paul Dale wrote:
> 
> 
> I’d generally prefer functions over macros ― I think that the ctrl calls e.g. 
> would be better
> wrapped with function to provide type checking.
> The overhead of a function call is pretty light these days so inline 
> functions are difficult to
> justify (as anything except a premature optimisation?).
> 
> Both safestack and lhash are problematic cases.  The inline functions come 
> from macros which I
> view as okay.  The problem is that some of these macros are expanded in the 
> header for common
> cases (e.g. stack of stings).  We could address this by distinguishing 
> between the function
> declarations and their instantiation and move the latter into its own C file.
> 
> Pauli
> -- 
> Dr Paul Dale | Cryptographer | Network Security & Encryption 
> Phone +61 7 3031 7217
> Oracle Australia
> 
> On 27 Jan 2019, at 8:33 pm, Tim Hudson  wrote:
>
> From https://github.com/openssl/openssl/pull/7721
>
> Tim - I think inline functions in public header files simply shouldn't be 
> present.
> Matt - I agree
> Richard - 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.
>
> It would be good to get a sense of the collective thoughts on the topic.
>
> Tim.
>
> ___
> openssl-project mailing list
> openssl-project@openssl.org
> https://mta.openssl.org/mailman/listinfo/openssl-project
> 
> 
> ___
> openssl-project mailing list
> openssl-project@openssl.org
> https://mta.openssl.org/mailman/listinfo/openssl-project
-- 
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/
___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project


Re: [openssl-project] inline functions

2019-01-27 Thread Dr Paul Dale
I’d generally prefer functions over macros — I think that the ctrl calls e.g. 
would be better wrapped with function to provide type checking.
The overhead of a function call is pretty light these days so inline functions 
are difficult to justify (as anything except a premature optimisation?).

Both safestack and lhash are problematic cases.  The inline functions come from 
macros which I view as okay.  The problem is that some of these macros are 
expanded in the header for common cases (e.g. stack of stings).  We could 
address this by distinguishing between the function declarations and their 
instantiation and move the latter into its own C file.


Pauli
-- 
Dr Paul Dale | Cryptographer | Network Security & Encryption 
Phone +61 7 3031 7217
Oracle Australia



> On 27 Jan 2019, at 8:33 pm, Tim Hudson  wrote:
> 
> From https://github.com/openssl/openssl/pull/7721 
> 
> 
> Tim - I think inline functions in public header files simply shouldn't be 
> present.
> Matt - I agree
> Richard - 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.
> 
> It would be good to get a sense of the collective thoughts on the topic.
> 
> Tim.
> 
> ___
> openssl-project mailing list
> openssl-project@openssl.org
> https://mta.openssl.org/mailman/listinfo/openssl-project

___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project

Re: [openssl-project] inline functions

2019-01-27 Thread Kurt Roeckx
On Sun, Jan 27, 2019 at 08:33:06PM +1000, Tim Hudson wrote:
> From https://github.com/openssl/openssl/pull/7721
> 
> Tim - I think inline functions in public header files simply shouldn't be
> present.
> Matt - I agree
> Richard - 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.

In general I have to agree that it's not a good thing to do,
specially in cases where it calls other functions. We went away
from making them just defines into functions, but they should
instead be functions in the library, at least for external users
of the library.


Kurt

___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project


[openssl-project] inline functions

2019-01-27 Thread Tim Hudson
>From https://github.com/openssl/openssl/pull/7721

Tim - I think inline functions in public header files simply shouldn't be
present.
Matt - I agree
Richard - 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.

It would be good to get a sense of the collective thoughts on the topic.

Tim.
___
openssl-project mailing list
openssl-project@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-project