On 07/22/15 23:07, David Woodhouse wrote:
> On Tue, 2015-06-23 at 09:54 -0400, Peter Jones wrote:
>> On Tue, Jun 23, 2015 at 03:34:25PM +0200, Laszlo Ersek wrote:
>>> Git commit f93f78ea70 (SVN r17633), with subject "CryptoPkg: Update
>>> openssl patch file from 0.9.8zf to 1.0.2c", replaced the file
>>>
>>>   CryptoPkg/Library/OpensslLib/EDKII_openssl-0.9.8zf.patch
>>>
>>> with
>>>
>>>   CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch
>>>
>>> In the process, two hunks were lost that used to add EFIAPI to the
>>> declaration of the variadic function ERR_add_error_data().
>>>
>>> The VA_START() macro, from "MdePkg/Include/Base.h", expands to an
>>> EFIAPI-dependent implementation when
>>>
>>>   !defined(__CC_ARM) && (!defined(__GNUC__) ||
>>>                          defined(NO_BUILTIN_VA_FUNCS))
>>>
>>> Under such circumstances, the va_start() macro invocation in
>>> ERR_add_error_data() -- which is translated to VA_START() by
>>> "CryptoPkg/Include/OpenSslSupport.h" -- results in EFIAPI-dependent code,
>>> but callers of the function pass the arguments incorrectly, because the
>>> declaration doesn't state EFIAPI.
>>>
>>> This leads to crashes when ERR_add_error_vdata(), called by
>>> ERR_add_error_data(), tries to access the arguments forwarded to it.
>>>
>>> Restore the missing hunk from before SVN r17633.
>>>
>>> Cc: Qin Long <qin.l...@intel.com>
>>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
>>> Cc: Gary Ching-Pang Lin <g...@suse.com>
>>> Cc: Peter Jones <pjo...@redhat.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>>
>> Yeah, EFIAPI to get the right calling conventions on x86_64 is
>> definitely the right thing here.  Is this what was breaking shim for
>> you?
>>
>> It's mildly annoying that EFIAPI is pretty much 100% about ABI calling
>> conventions and has nothing to do with actual API, but that ship has
>> long past sailed.
>>
>> Acked-by: Peter Jones <pjo...@redhat.com>
> 
> Alternatively (as I baulk at trying to send that particular one-liner
> upstream to OpenSSL), would it be possible to simply *avoid* defining
> NO_BUILTIN_VA_FUNCS for the OpenSSL part of the build?

I "guess" it should work at the moment (and -UNO_BUILTIN_VA_FUNCS would
be easy enough to add under [BuildOptions] in
"CryptoPkg/Library/OpensslLib/OpensslLib.inf").

But, as soon as a "genuinely" EFIAPI function with a variable argument
list appeared in "OpensslLib.inf", things would break again.

Any particular reason for disliking these hunks of
"EDKII_openssl-1.0.2d.patch" (over the other hunks)?

... The best thing would be to avoid NO_BUILTIN_VA_FUNCS *completely*.

If we did that, then the macros in question would be defined as:

> typedef __builtin_va_list VA_LIST;
>
> #define VA_START(Marker, Parameter) __builtin_va_start (Marker, Parameter)
>
> #define VA_ARG(Marker, TYPE) ((sizeof (TYPE) < sizeof (UINTN)) ? 
> (TYPE)(__builtin_va_arg (Marker, UINTN)) : (TYPE)(__builtin_va_arg (Marker, 
> TYPE)))
>
> #define VA_END(Marker) __builtin_va_end (Marker)
>
> #define VA_COPY(Dest, Start) __builtin_va_copy (Dest, Start)

Unfortunately, this is good for *non*-EFIAPI functions only. For EFIAPI
functions, you'd have to define them as __builtin_ms_va* (note the _ms).
Which, in turn, would be good only for EFIAPI functions. (See
"/usr/lib/gcc/*/*/include/cross-stdarg.h". See also
<https://bugzilla.redhat.com/show_bug.cgi?id=871889#c3>.)

So, this issue is not fixable globally under gcc. With gcc (compiling
for x86_64), EFIAPI and non-EFIAPI differ from each other, and the VA_*
macros would have to expand to different gcc builtins dependent *on the
function*, not some global (or even source file level) #define. And,
since the UEFI spec declares a number of functions that take variable
arguments, like InstallMultipleProtocolInterfaces(), we are forced to
stick with EFIAPI as default, and that compels all other varargs
functions (that use the VA_* macros internally) to be EFIAPI too. This
could only be avoided if __builtin_va* considered the containing
function's calling convention automatically, but that's not going to
happen (see bug 871889 again).

... Rather than undefining NO_BUILTIN_VA_FUNCS for openssl, we should
instead remove

> //
> // Map all va_xxxx elements to VA_xxx defined in MdePkg/Include/Base.h
> //
> #if !defined(__CC_ARM) // if va_list is not already defined
> #define va_list VA_LIST
> #define va_arg VA_ARG
> #define va_start VA_START
> #define va_end VA_END

from "CryptoPkg/Include/OpenSslSupport.h" (in the gcc case). Because,
without the VA_* macros (of which OpenSSL itself contains no instances),
there would be no requirement for EFIAPI either. In other words, the
EFIAPI hunks in "EDKII_openssl-1.0.2d.patch" are a solution to a problem
that "CryptoPkg/Include/OpenSslSupport.h" creates.

Thanks
Laszlo

------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Reply via email to