Fix 2054 -- missing get/set for inheritance flags #2079
Conversation
| @@ -16,7 +16,7 @@ | |||
| struct X509_VERIFY_PARAM_st { | |||
| char *name; | |||
| time_t check_time; /* Time to use */ | |||
| unsigned long inh_flags; /* Inheritance flags */ | |||
| uint32_t inh_flags; /* Inheritance flags */ | |||
There was a problem hiding this comment.
Why is that type change important?
There was a problem hiding this comment.
Because "long" is a useful type in an ABI (no formal width defined). So making it uint32 is better and since the structure is opaque, I can make the change all the way back to the structure itself.
There was a problem hiding this comment.
Did you mean "Because "long" isN'T a useful type in an ABI"?
There was a problem hiding this comment.
Either way, I'm not sure how int32_t is better unless we're talking interop between systems, i.e. that a dump of this structure could be passed between systems where the system ABI isn't the same.
It's just that it strikes me as oddly arbitrary, considering the types of the other fields in that structure.
There was a problem hiding this comment.
Suppose LONG is 64 bits. Then the structure type will not match the API/ABI parameter type. And binaries will not port across.
| { | ||
| param->inh_flags = flags; | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Taking the discussion here. Why is int32_t important as a flags type at all, i.e. as an input type and return type. This is really the same question I'm asking about the inh_flags field.
There was a problem hiding this comment.
On platforms today, a long can be 32 or 64 bits depending on the compiler flags. In order to have ABI portability across hosts with different compiler flag settings, we need to use a type that has a strictly-defined width (number of bytes). u32int is that. long isn't. Since the API is correct, might as well change the field type as well, because we can, and it avoids compile issues on places where sizeof(uint32_t) != sizeof(long). @ekasper has written about this on the team mailing list, I believe.
There was a problem hiding this comment.
Yes, I get you there. However, the rest of the file is littered with long / int, and so goes for the rest of libcrypto and libssl. It strikes me as odd to do this for one arbitrary function.
I guess you and I have a different view on how to do this. I'm thinking in terms of overhauls, while this change has me guess you want to start a grassroot kinda trend.
Either way, I think I have my question answered...
There was a problem hiding this comment.
We can't change the API now, but we can stop adding to the mistake :)
|
Just to be safe, I'll do a VMS build on this... just to make sure we don't forget to include some needed header file. |
| If B<X509_VP_FLAG_LOCKED> is set then no values are copied. | ||
|
|
||
| If B<X509_VP_FLAG_ONCE> is set then the current setting is zeroed | ||
| after the next call. |
There was a problem hiding this comment.
Are these flags all mutually exclusive? If not, which ones are and how do they interact?
There was a problem hiding this comment.
updated commit pushed to answer this.
Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #2079)
Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #2079)
Fixes #2054
For 1.1.0 and then forward-pick to master.