* [Patch 0/4] *** Resolving CryptoPkg build issues ***
@ 2017-03-31 17:05 Qin Long
2017-03-31 17:05 ` [Patch 1/4] CryptoPkg/OpensslLib: Suppress extra build warnings in openssl source Qin Long
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Qin Long @ 2017-03-31 17:05 UTC (permalink / raw)
To: edk2-devel; +Cc: ting.ye, lersek, hao.a.wu, feng.tian, eric.dong
This patch series introduced some hotfixes and workaround to resolve
the build issues under different toolchain, and from potential external
consumers, including:
- build warning under GCC48 and VS2010 toolchain;
- Potential unresolved external symbol link issue;
- One bug fix of timer() wrapper in ConstantTimeClock.c;
- One workaround to resolve macro re-definitions issue from some
external BaseCryptLib consumer.
(https://github.com/qloong/edk2/commits/dev-openssl-hotfix)
Qin Long (4):
CryptoPkg/OpensslLib: Suppress extra build warnings in openssl source
CryptoPkg: Fix possible unresolved external symbol issue.
CryptoPkg/BaseCryptLib: Adding NULL checking in timer() wrapper.
CryptoPkg: One workaround to resolve potential build issue.
CryptoPkg/Include/CrtLibSupport.h | 1 +
CryptoPkg/Include/openssl/e_os2.h | 315 +++++++++++++++++++++
.../BaseCryptLib/SysCall/ConstantTimeClock.c | 6 +-
CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c | 6 +
CryptoPkg/Library/OpensslLib/OpensslLib.inf | 15 +-
CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf | 15 +-
6 files changed, 346 insertions(+), 12 deletions(-)
create mode 100644 CryptoPkg/Include/openssl/e_os2.h
--
2.12.2.windows.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Patch 1/4] CryptoPkg/OpensslLib: Suppress extra build warnings in openssl source
2017-03-31 17:05 [Patch 0/4] *** Resolving CryptoPkg build issues *** Qin Long
@ 2017-03-31 17:05 ` Qin Long
2017-03-31 18:23 ` Laszlo Ersek
2017-03-31 17:05 ` [Patch 2/4] CryptoPkg: Fix possible unresolved external symbol issue Qin Long
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Qin Long @ 2017-03-31 17:05 UTC (permalink / raw)
To: edk2-devel; +Cc: ting.ye, lersek, hao.a.wu, feng.tian, eric.dong
This patch added some extra build options to suppress possible warnings
when building openssl source under GCC48 and VS2010. Including:
Adding "-Wno-error=maybe-uninitialized" to suppress the following GCC48
build warning:
OpensslLib/openssl/ssl/statem/statem_clnt.c:2543:9: error: "len" may
be used uninitialized in this function [-Werror=maybe-uninitialized]
len += pskhdrlen;
^
And adding "/wd4306" to suppress the following VS2010 build warning:
openssl\crypto\asn1\tasn_dec.c(795) : warning C4306: 'type cast' :
conversion from 'int' to 'ASN1_VALUE *' of greater size
Cc: Ting Ye <ting.ye@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qin Long <qin.long@intel.com>
---
CryptoPkg/Library/OpensslLib/OpensslLib.inf | 15 ++++++++++-----
CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf | 15 ++++++++++-----
2 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/CryptoPkg/Library/OpensslLib/OpensslLib.inf b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
index 42f72f4f1f..cbabb34bdd 100644
--- a/CryptoPkg/Library/OpensslLib/OpensslLib.inf
+++ b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
@@ -535,21 +535,26 @@
# C4244: conversion from type1 to type2, possible loss of data
# C4245: conversion from type1 to type2, signed/unsigned mismatch
# C4267: conversion from size_t to type, possible loss of data
+ # C4306: 'identifier' : conversion from 'type1' to 'type2' of greater size
# C4389: 'operator' : signed/unsigned mismatch (xxxx)
# C4702: unreachable code
# C4706: assignment within conditional expression
#
MSFT:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
- MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
- MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
+ MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706
+ MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706
INTEL:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
INTEL:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
INTEL:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
- GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
- GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -DNO_MSABI_VA_FUNCS
- GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
+ #
+ # Suppress the following build warnings in openssl so we don't break the build with -Werror
+ # -Werror=maybe-uninitialized: there exist some other paths for which the variable is not initialized.
+ #
+ GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized
+ GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized -DNO_MSABI_VA_FUNCS
+ GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized
GCC:*_*_ARM_CC_FLAGS = $(OPENSSL_FLAGS)
GCC:*_*_AARCH64_CC_FLAGS = $(OPENSSL_FLAGS)
diff --git a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
index cbbb456d70..026b551bca 100644
--- a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
+++ b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
@@ -496,21 +496,26 @@
# C4244: conversion from type1 to type2, possible loss of data
# C4245: conversion from type1 to type2, signed/unsigned mismatch
# C4267: conversion from size_t to type, possible loss of data
+ # C4306: 'identifier' : conversion from 'type1' to 'type2' of greater size
# C4389: 'operator' : signed/unsigned mismatch (xxxx)
# C4702: unreachable code
# C4706: assignment within conditional expression
#
MSFT:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
- MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
- MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
+ MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706
+ MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706
INTEL:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
INTEL:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
INTEL:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
- GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
- GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -DNO_MSABI_VA_FUNCS
- GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
+ #
+ # Suppress the following build warnings in openssl so we don't break the build with -Werror
+ # -Werror=maybe-uninitialized: there exist some other paths for which the variable is not initialized.
+ #
+ GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized
+ GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized -DNO_MSABI_VA_FUNCS
+ GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized
GCC:*_*_ARM_CC_FLAGS = $(OPENSSL_FLAGS)
GCC:*_*_AARCH64_CC_FLAGS = $(OPENSSL_FLAGS)
--
2.12.2.windows.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Patch 2/4] CryptoPkg: Fix possible unresolved external symbol issue.
2017-03-31 17:05 [Patch 0/4] *** Resolving CryptoPkg build issues *** Qin Long
2017-03-31 17:05 ` [Patch 1/4] CryptoPkg/OpensslLib: Suppress extra build warnings in openssl source Qin Long
@ 2017-03-31 17:05 ` Qin Long
2017-03-31 18:44 ` Laszlo Ersek
2017-03-31 17:05 ` [Patch 3/4] CryptoPkg/BaseCryptLib: Adding NULL checking in timer() wrapper Qin Long
2017-03-31 17:05 ` [Patch 4/4] CryptoPkg: One workaround to resolve potential build issue Qin Long
3 siblings, 1 reply; 13+ messages in thread
From: Qin Long @ 2017-03-31 17:05 UTC (permalink / raw)
To: edk2-devel; +Cc: ting.ye, lersek, hao.a.wu, feng.tian, eric.dong
The compiler (visual studio) may optimize some explicit strcmp call
to use the intrinsic memcmp call. In CrtLibSupport.h, we just use
#define to mapping memcmp to CompareMem API. So in Link phase, this
kind of intrinsic optimization will cause the "unresolved external
symbol" error. For example:
OpensslLib.lib(v3_utl.obj) : error LNK2001:
unresolved external symbol _memcmp
This patch will keep the memcmp mapping, and provide extra Intrinsic
memcmp wrapper to satisfy the symbol link.
Cc: Ting Ye <ting.ye@intel.com>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qin Long <qin.long@intel.com>
---
CryptoPkg/Include/CrtLibSupport.h | 1 +
CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c | 6 ++++++
2 files changed, 7 insertions(+)
diff --git a/CryptoPkg/Include/CrtLibSupport.h b/CryptoPkg/Include/CrtLibSupport.h
index ddf7784a37..7f1ec12302 100644
--- a/CryptoPkg/Include/CrtLibSupport.h
+++ b/CryptoPkg/Include/CrtLibSupport.h
@@ -133,6 +133,7 @@ void *malloc (size_t);
void *realloc (void *, size_t);
void free (void *);
void *memset (void *, int, size_t);
+int memcmp (const void *, const void *, size_t);
int isdigit (int);
int isspace (int);
int isxdigit (int);
diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
index e8a76d07ff..33489aa6f4 100644
--- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
+++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
@@ -46,6 +46,12 @@ void * memset (void *dest, char ch, unsigned int count)
return dest;
}
+/* Compare characters in two buffers. */
+int memcmp (const void *buf1, const void *buf2, unsigned int count)
+{
+ return (int)(CompareMem(buf1,buf2,(UINTN)(count)));
+}
+
int strcmp (const char *s1, const char *s2)
{
return (int)AsciiStrCmp(s1, s2);
--
2.12.2.windows.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Patch 3/4] CryptoPkg/BaseCryptLib: Adding NULL checking in timer() wrapper.
2017-03-31 17:05 [Patch 0/4] *** Resolving CryptoPkg build issues *** Qin Long
2017-03-31 17:05 ` [Patch 1/4] CryptoPkg/OpensslLib: Suppress extra build warnings in openssl source Qin Long
2017-03-31 17:05 ` [Patch 2/4] CryptoPkg: Fix possible unresolved external symbol issue Qin Long
@ 2017-03-31 17:05 ` Qin Long
2017-03-31 18:45 ` Laszlo Ersek
2017-03-31 17:05 ` [Patch 4/4] CryptoPkg: One workaround to resolve potential build issue Qin Long
3 siblings, 1 reply; 13+ messages in thread
From: Qin Long @ 2017-03-31 17:05 UTC (permalink / raw)
To: edk2-devel; +Cc: ting.ye, lersek, hao.a.wu, feng.tian, eric.dong
There are some explicit timer(NULL) calls in openssl-1.1.0xx source,
but the dummy timer() wrapper in ConstantTimeClock.c (used by PEI
and SMM module) has no any checks on NULL parameter. This will
cause the memory access issue.
This patch adds the NULL parameter checking in timer() wrapper.
Cc: Ting Ye <ting.ye@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qin Long <qin.long@intel.com>
---
CryptoPkg/Library/BaseCryptLib/SysCall/ConstantTimeClock.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/ConstantTimeClock.c b/CryptoPkg/Library/BaseCryptLib/SysCall/ConstantTimeClock.c
index 7f20164999..0cd90434ca 100644
--- a/CryptoPkg/Library/BaseCryptLib/SysCall/ConstantTimeClock.c
+++ b/CryptoPkg/Library/BaseCryptLib/SysCall/ConstantTimeClock.c
@@ -31,8 +31,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
time_t time (time_t *timer)
{
- *timer = 0;
- return *timer;
+ if (timer != NULL) {
+ *timer = 0;
+ }
+ return 0;
}
struct tm * gmtime (const time_t *timer)
--
2.12.2.windows.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Patch 4/4] CryptoPkg: One workaround to resolve potential build issue.
2017-03-31 17:05 [Patch 0/4] *** Resolving CryptoPkg build issues *** Qin Long
` (2 preceding siblings ...)
2017-03-31 17:05 ` [Patch 3/4] CryptoPkg/BaseCryptLib: Adding NULL checking in timer() wrapper Qin Long
@ 2017-03-31 17:05 ` Qin Long
2017-03-31 18:50 ` Laszlo Ersek
3 siblings, 1 reply; 13+ messages in thread
From: Qin Long @ 2017-03-31 17:05 UTC (permalink / raw)
To: edk2-devel; +Cc: ting.ye, lersek, hao.a.wu, feng.tian, eric.dong
This patch duplicates one e_os2.h with minor changes into
"CryptoPkg/Include/openssl/" as one workaround to resolve some
external components/modules build issue.
The BaseCryptLib consumer should ONLY include "BaseCryptLib.h" to
use the exposed APIs. But some external modules call OpenSSL API
directly via include "openssl/xxx.h", which causes some macro
re-definitions errors in e_os2.h (under default _WIN32/_WIN64).
We duplicate one e_os2.h with the following changes to fix this
kind of macro re-definition error.
-# if defined(OPENSSL_SYS_UEFI) && !defined(ssize_t)
+# if defined(OPENSSL_SYS_UEFI) && !defined(ossl_ssize_t)
+# if !defined(ssize_t)
# define ossl_ssize_t int
+# else
+# define ossl_ssize_t ssize_t
+# endif
# define OSSL_SSIZE_MAX INT_MAX
# endif
This is just one workaround before other external modules were
cleaned. And this update of e_os2.h could also be upstreaming
into openssl to eliminate the potential risk.
Cc: Ting Ye <ting.ye@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qin Long <qin.long@intel.com>
---
CryptoPkg/Include/openssl/e_os2.h | 315 ++++++++++++++++++++++++++++++++++++++
1 file changed, 315 insertions(+)
create mode 100644 CryptoPkg/Include/openssl/e_os2.h
diff --git a/CryptoPkg/Include/openssl/e_os2.h b/CryptoPkg/Include/openssl/e_os2.h
new file mode 100644
index 0000000000..a0adbc195b
--- /dev/null
+++ b/CryptoPkg/Include/openssl/e_os2.h
@@ -0,0 +1,315 @@
+/*
+ * Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.
+ *
+ * Licensed under the OpenSSL license (the "License"). You may not use
+ * this file except in compliance with the License. You can obtain a copy
+ * in the file LICENSE in the source distribution or at
+ * https://www.openssl.org/source/license.html
+ */
+
+#ifndef HEADER_E_OS2_H
+# define HEADER_E_OS2_H
+
+# include <openssl/opensslconf.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/******************************************************************************
+ * Detect operating systems. This probably needs completing.
+ * The result is that at least one OPENSSL_SYS_os macro should be defined.
+ * However, if none is defined, Unix is assumed.
+ **/
+
+# define OPENSSL_SYS_UNIX
+
+/* --------------------- Microsoft operating systems ---------------------- */
+
+/*
+ * Note that MSDOS actually denotes 32-bit environments running on top of
+ * MS-DOS, such as DJGPP one.
+ */
+# if defined(OPENSSL_SYS_MSDOS)
+# undef OPENSSL_SYS_UNIX
+# endif
+
+/*
+ * For 32 bit environment, there seems to be the CygWin environment and then
+ * all the others that try to do the same thing Microsoft does...
+ */
+/*
+ * UEFI lives here because it might be built with a Microsoft toolchain and
+ * we need to avoid the false positive match on Windows.
+ */
+# if defined(OPENSSL_SYS_UEFI)
+# undef OPENSSL_SYS_UNIX
+# elif defined(OPENSSL_SYS_UWIN)
+# undef OPENSSL_SYS_UNIX
+# define OPENSSL_SYS_WIN32_UWIN
+# else
+# if defined(__CYGWIN__) || defined(OPENSSL_SYS_CYGWIN)
+# undef OPENSSL_SYS_UNIX
+# define OPENSSL_SYS_WIN32_CYGWIN
+# else
+# if defined(_WIN32) || defined(OPENSSL_SYS_WIN32)
+# undef OPENSSL_SYS_UNIX
+# if !defined(OPENSSL_SYS_WIN32)
+# define OPENSSL_SYS_WIN32
+# endif
+# endif
+# if defined(_WIN64) || defined(OPENSSL_SYS_WIN64)
+# undef OPENSSL_SYS_UNIX
+# if !defined(OPENSSL_SYS_WIN64)
+# define OPENSSL_SYS_WIN64
+# endif
+# endif
+# if defined(OPENSSL_SYS_WINNT)
+# undef OPENSSL_SYS_UNIX
+# endif
+# if defined(OPENSSL_SYS_WINCE)
+# undef OPENSSL_SYS_UNIX
+# endif
+# endif
+# endif
+
+/* Anything that tries to look like Microsoft is "Windows" */
+# if defined(OPENSSL_SYS_WIN32) || defined(OPENSSL_SYS_WIN64) || defined(OPENSSL_SYS_WINNT) || defined(OPENSSL_SYS_WINCE)
+# undef OPENSSL_SYS_UNIX
+# define OPENSSL_SYS_WINDOWS
+# ifndef OPENSSL_SYS_MSDOS
+# define OPENSSL_SYS_MSDOS
+# endif
+# endif
+
+/*
+ * DLL settings. This part is a bit tough, because it's up to the
+ * application implementor how he or she will link the application, so it
+ * requires some macro to be used.
+ */
+# ifdef OPENSSL_SYS_WINDOWS
+# ifndef OPENSSL_OPT_WINDLL
+# if defined(_WINDLL) /* This is used when building OpenSSL to
+ * indicate that DLL linkage should be used */
+# define OPENSSL_OPT_WINDLL
+# endif
+# endif
+# endif
+
+/* ------------------------------- OpenVMS -------------------------------- */
+# if defined(__VMS) || defined(VMS) || defined(OPENSSL_SYS_VMS)
+# if !defined(OPENSSL_SYS_VMS)
+# undef OPENSSL_SYS_UNIX
+# endif
+# define OPENSSL_SYS_VMS
+# if defined(__DECC)
+# define OPENSSL_SYS_VMS_DECC
+# elif defined(__DECCXX)
+# define OPENSSL_SYS_VMS_DECC
+# define OPENSSL_SYS_VMS_DECCXX
+# else
+# define OPENSSL_SYS_VMS_NODECC
+# endif
+# endif
+
+/* -------------------------------- Unix ---------------------------------- */
+# ifdef OPENSSL_SYS_UNIX
+# if defined(linux) || defined(__linux__) && !defined(OPENSSL_SYS_LINUX)
+# define OPENSSL_SYS_LINUX
+# endif
+# if defined(_AIX) && !defined(OPENSSL_SYS_AIX)
+# define OPENSSL_SYS_AIX
+# endif
+# endif
+
+/* -------------------------------- VOS ----------------------------------- */
+# if defined(__VOS__) && !defined(OPENSSL_SYS_VOS)
+# define OPENSSL_SYS_VOS
+# ifdef __HPPA__
+# define OPENSSL_SYS_VOS_HPPA
+# endif
+# ifdef __IA32__
+# define OPENSSL_SYS_VOS_IA32
+# endif
+# endif
+
+/**
+ * That's it for OS-specific stuff
+ *****************************************************************************/
+
+/* Specials for I/O an exit */
+# ifdef OPENSSL_SYS_MSDOS
+# define OPENSSL_UNISTD_IO <io.h>
+# define OPENSSL_DECLARE_EXIT extern void exit(int);
+# else
+# define OPENSSL_UNISTD_IO OPENSSL_UNISTD
+# define OPENSSL_DECLARE_EXIT /* declared in unistd.h */
+# endif
+
+/*-
+ * Definitions of OPENSSL_GLOBAL and OPENSSL_EXTERN, to define and declare
+ * certain global symbols that, with some compilers under VMS, have to be
+ * defined and declared explicitly with globaldef and globalref.
+ * Definitions of OPENSSL_EXPORT and OPENSSL_IMPORT, to define and declare
+ * DLL exports and imports for compilers under Win32. These are a little
+ * more complicated to use. Basically, for any library that exports some
+ * global variables, the following code must be present in the header file
+ * that declares them, before OPENSSL_EXTERN is used:
+ *
+ * #ifdef SOME_BUILD_FLAG_MACRO
+ * # undef OPENSSL_EXTERN
+ * # define OPENSSL_EXTERN OPENSSL_EXPORT
+ * #endif
+ *
+ * The default is to have OPENSSL_EXPORT, OPENSSL_EXTERN and OPENSSL_GLOBAL
+ * have some generally sensible values.
+ */
+
+# if defined(OPENSSL_SYS_VMS_NODECC)
+# define OPENSSL_EXPORT globalref
+# define OPENSSL_EXTERN globalref
+# define OPENSSL_GLOBAL globaldef
+# elif defined(OPENSSL_SYS_WINDOWS) && defined(OPENSSL_OPT_WINDLL)
+# define OPENSSL_EXPORT extern __declspec(dllexport)
+# define OPENSSL_EXTERN extern __declspec(dllimport)
+# define OPENSSL_GLOBAL
+# else
+# define OPENSSL_EXPORT extern
+# define OPENSSL_EXTERN extern
+# define OPENSSL_GLOBAL
+# endif
+
+/*-
+ * Macros to allow global variables to be reached through function calls when
+ * required (if a shared library version requires it, for example.
+ * The way it's done allows definitions like this:
+ *
+ * // in foobar.c
+ * OPENSSL_IMPLEMENT_GLOBAL(int,foobar,0)
+ * // in foobar.h
+ * OPENSSL_DECLARE_GLOBAL(int,foobar);
+ * #define foobar OPENSSL_GLOBAL_REF(foobar)
+ */
+# ifdef OPENSSL_EXPORT_VAR_AS_FUNCTION
+# define OPENSSL_IMPLEMENT_GLOBAL(type,name,value) \
+ type *_shadow_##name(void) \
+ { static type _hide_##name=value; return &_hide_##name; }
+# define OPENSSL_DECLARE_GLOBAL(type,name) type *_shadow_##name(void)
+# define OPENSSL_GLOBAL_REF(name) (*(_shadow_##name()))
+# else
+# define OPENSSL_IMPLEMENT_GLOBAL(type,name,value) OPENSSL_GLOBAL type _shadow_##name=value;
+# define OPENSSL_DECLARE_GLOBAL(type,name) OPENSSL_EXPORT type _shadow_##name
+# define OPENSSL_GLOBAL_REF(name) _shadow_##name
+# endif
+
+# ifdef _WIN32
+# ifdef _WIN64
+# define ossl_ssize_t __int64
+# define OSSL_SSIZE_MAX _I64_MAX
+# else
+# define ossl_ssize_t int
+# define OSSL_SSIZE_MAX INT_MAX
+# endif
+# endif
+
+# if defined(OPENSSL_SYS_UEFI) && !defined(ossl_ssize_t)
+# if !defined(ssize_t)
+# define ossl_ssize_t int
+# else
+# define ossl_ssize_t ssize_t
+# endif
+# define OSSL_SSIZE_MAX INT_MAX
+# endif
+
+# ifndef ossl_ssize_t
+# define ossl_ssize_t ssize_t
+# if defined(SSIZE_MAX)
+# define OSSL_SSIZE_MAX SSIZE_MAX
+# elif defined(_POSIX_SSIZE_MAX)
+# define OSSL_SSIZE_MAX _POSIX_SSIZE_MAX
+# endif
+# endif
+
+# ifdef DEBUG_UNUSED
+# define __owur __attribute__((__warn_unused_result__))
+# else
+# define __owur
+# endif
+
+/* Standard integer types */
+# if defined(OPENSSL_SYS_UEFI)
+typedef INT8 int8_t;
+typedef UINT8 uint8_t;
+typedef INT16 int16_t;
+typedef UINT16 uint16_t;
+typedef INT32 int32_t;
+typedef UINT32 uint32_t;
+typedef INT64 int64_t;
+typedef UINT64 uint64_t;
+# define PRIu64 "%Lu"
+# elif (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) || \
+ defined(__osf__) || defined(__sgi) || defined(__hpux) || \
+ defined(OPENSSL_SYS_VMS) || defined (__OpenBSD__)
+# include <inttypes.h>
+# elif defined(_MSC_VER) && _MSC_VER<=1500
+/*
+ * minimally required typdefs for systems not supporting inttypes.h or
+ * stdint.h: currently just older VC++
+ */
+typedef signed char int8_t;
+typedef unsigned char uint8_t;
+typedef short int16_t;
+typedef unsigned short uint16_t;
+typedef int int32_t;
+typedef unsigned int uint32_t;
+typedef __int64 int64_t;
+typedef unsigned __int64 uint64_t;
+# else
+# include <stdint.h>
+# endif
+
+/*
+ * We need a format operator for some client tools for uint64_t. If inttypes.h
+ * isn't available or did not define it, just go with hard-coded.
+ */
+# ifndef PRIu64
+# ifdef SIXTY_FOUR_BIT_LONG
+# define PRIu64 "lu"
+# else
+# define PRIu64 "llu"
+# endif
+# endif
+
+/* ossl_inline: portable inline definition usable in public headers */
+# if !defined(inline) && !defined(__cplusplus)
+# if defined(__STDC_VERSION__) && __STDC_VERSION__>=199901L
+ /* just use inline */
+# define ossl_inline inline
+# elif defined(__GNUC__) && __GNUC__>=2
+# define ossl_inline __inline__
+# elif defined(_MSC_VER)
+ /*
+ * Visual Studio: inline is available in C++ only, however
+ * __inline is available for C, see
+ * http://msdn.microsoft.com/en-us/library/z8y1yy88.aspx
+ */
+# define ossl_inline __inline
+# else
+# define ossl_inline
+# endif
+# else
+# define ossl_inline inline
+# endif
+
+# if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L
+# define ossl_noreturn _Noreturn
+# elif defined(__GNUC__) && __GNUC__ >= 2
+# define ossl_noreturn __attribute__((noreturn))
+# else
+# define ossl_noreturn
+# endif
+
+#ifdef __cplusplus
+}
+#endif
+#endif
--
2.12.2.windows.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Patch 1/4] CryptoPkg/OpensslLib: Suppress extra build warnings in openssl source
2017-03-31 17:05 ` [Patch 1/4] CryptoPkg/OpensslLib: Suppress extra build warnings in openssl source Qin Long
@ 2017-03-31 18:23 ` Laszlo Ersek
2017-04-01 1:10 ` Long, Qin
0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2017-03-31 18:23 UTC (permalink / raw)
To: Qin Long, edk2-devel; +Cc: ting.ye, hao.a.wu, feng.tian, eric.dong
On 03/31/17 19:05, Qin Long wrote:
> This patch added some extra build options to suppress possible warnings
> when building openssl source under GCC48 and VS2010. Including:
>
> Adding "-Wno-error=maybe-uninitialized" to suppress the following GCC48
> build warning:
> OpensslLib/openssl/ssl/statem/statem_clnt.c:2543:9: error: "len" may
> be used uninitialized in this function [-Werror=maybe-uninitialized]
> len += pskhdrlen;
> ^
This happens in the tls_construct_client_key_exchange() function. After
reviewing the function, the warning is indeed incorrect. I agree with
the fix (adding -Wno-error=maybe-uninitialized), but I propose the
following too:
- file an upstream OpenSSL bug report about this (the existent "len = 0"
assignment can be moved to the unconditional part of the code),
- file a TianoCore BZ as well, so that we don't forget removing
-Wno-error=maybe-uninitialized once the OpenSSL bug is fixed and we
adopt the next stable release with the OpenSSL change.
The point is, I think -Werror=maybe-uninitialized is a valuable warning,
and it might help us catch an actual bug in OpenSSL later on. So I don't
think it should be turned off permanently.
> And adding "/wd4306" to suppress the following VS2010 build warning:
> openssl\crypto\asn1\tasn_dec.c(795) : warning C4306: 'type cast' :
> conversion from 'int' to 'ASN1_VALUE *' of greater size
That again seems like a valid warning, the C standard doesn't guarantee
that (int) can be converted to a pointer type and vice versa. The code
should be using intptr_t or uintptr_t (in edk2 we use UINTN for that).
This is the code in question:
case V_ASN1_NULL:
if (len) {
ASN1err(ASN1_F_ASN1_EX_C2I, ASN1_R_NULL_IS_WRONG_LENGTH);
goto err;
}
*pval = (ASN1_VALUE *)1;
break;
This is really awful. But, if it is necessary, it should be
*pval = (ASN1_VALUE *)(uintptr_t)1;
Again, I think it's OK to suppress the warnings for now, but I think we
should have a longer term reminder to reenable them.
>
> Cc: Ting Ye <ting.ye@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qin Long <qin.long@intel.com>
> ---
> CryptoPkg/Library/OpensslLib/OpensslLib.inf | 15 ++++++++++-----
> CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf | 15 ++++++++++-----
> 2 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/CryptoPkg/Library/OpensslLib/OpensslLib.inf b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
> index 42f72f4f1f..cbabb34bdd 100644
> --- a/CryptoPkg/Library/OpensslLib/OpensslLib.inf
> +++ b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
> @@ -535,21 +535,26 @@
> # C4244: conversion from type1 to type2, possible loss of data
> # C4245: conversion from type1 to type2, signed/unsigned mismatch
> # C4267: conversion from size_t to type, possible loss of data
> + # C4306: 'identifier' : conversion from 'type1' to 'type2' of greater size
> # C4389: 'operator' : signed/unsigned mismatch (xxxx)
> # C4702: unreachable code
> # C4706: assignment within conditional expression
> #
> MSFT:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
> - MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
> - MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
> + MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706
> + MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706
>
> INTEL:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
> INTEL:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
> INTEL:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
>
> - GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
> - GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -DNO_MSABI_VA_FUNCS
> - GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
> + #
> + # Suppress the following build warnings in openssl so we don't break the build with -Werror
> + # -Werror=maybe-uninitialized: there exist some other paths for which the variable is not initialized.
If that was true, that would be an actual bug in the OpenSSL code base,
and suppressing the warning in such a case would also be a bug. We may
only suppress the warning because it is *invalid* -- there is no such
problematic code path, only the compiler (incorrectly) thinks there is.
So please update the comment. I would even go as far as including the
TianoCore BZ number in this comment, for the BZ which tracks the
longer-term re-enablement of this warning.
Looks good to me otherwise.
(Note: personally I can't reproduce the build issues that I reported, at
least with GCC48 that I use.)
Thanks!
Laszlo
> + #
> + GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized
> + GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized -DNO_MSABI_VA_FUNCS
> + GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized
> GCC:*_*_ARM_CC_FLAGS = $(OPENSSL_FLAGS)
> GCC:*_*_AARCH64_CC_FLAGS = $(OPENSSL_FLAGS)
>
> diff --git a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> index cbbb456d70..026b551bca 100644
> --- a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> +++ b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> @@ -496,21 +496,26 @@
> # C4244: conversion from type1 to type2, possible loss of data
> # C4245: conversion from type1 to type2, signed/unsigned mismatch
> # C4267: conversion from size_t to type, possible loss of data
> + # C4306: 'identifier' : conversion from 'type1' to 'type2' of greater size
> # C4389: 'operator' : signed/unsigned mismatch (xxxx)
> # C4702: unreachable code
> # C4706: assignment within conditional expression
> #
> MSFT:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
> - MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
> - MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702 /wd4706
> + MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706
> + MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389 /wd4702 /wd4706
>
> INTEL:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
> INTEL:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
> INTEL:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC $(OPENSSL_FLAGS) /w
>
> - GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
> - GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -DNO_MSABI_VA_FUNCS
> - GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
> + #
> + # Suppress the following build warnings in openssl so we don't break the build with -Werror
> + # -Werror=maybe-uninitialized: there exist some other paths for which the variable is not initialized.
> + #
> + GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized
> + GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized -DNO_MSABI_VA_FUNCS
> + GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -Wno-error=maybe-uninitialized
> GCC:*_*_ARM_CC_FLAGS = $(OPENSSL_FLAGS)
> GCC:*_*_AARCH64_CC_FLAGS = $(OPENSSL_FLAGS)
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 2/4] CryptoPkg: Fix possible unresolved external symbol issue.
2017-03-31 17:05 ` [Patch 2/4] CryptoPkg: Fix possible unresolved external symbol issue Qin Long
@ 2017-03-31 18:44 ` Laszlo Ersek
2017-04-01 2:31 ` Long, Qin
0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2017-03-31 18:44 UTC (permalink / raw)
To: Qin Long, edk2-devel; +Cc: ting.ye, hao.a.wu, feng.tian, eric.dong
On 03/31/17 19:05, Qin Long wrote:
> The compiler (visual studio) may optimize some explicit strcmp call
> to use the intrinsic memcmp call. In CrtLibSupport.h, we just use
> #define to mapping memcmp to CompareMem API. So in Link phase, this
> kind of intrinsic optimization will cause the "unresolved external
> symbol" error. For example:
> OpensslLib.lib(v3_utl.obj) : error LNK2001:
> unresolved external symbol _memcmp
>
> This patch will keep the memcmp mapping, and provide extra Intrinsic
> memcmp wrapper to satisfy the symbol link.
>
> Cc: Ting Ye <ting.ye@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qin Long <qin.long@intel.com>
> ---
> CryptoPkg/Include/CrtLibSupport.h | 1 +
> CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c | 6 ++++++
> 2 files changed, 7 insertions(+)
>
> diff --git a/CryptoPkg/Include/CrtLibSupport.h b/CryptoPkg/Include/CrtLibSupport.h
> index ddf7784a37..7f1ec12302 100644
> --- a/CryptoPkg/Include/CrtLibSupport.h
> +++ b/CryptoPkg/Include/CrtLibSupport.h
> @@ -133,6 +133,7 @@ void *malloc (size_t);
> void *realloc (void *, size_t);
> void free (void *);
> void *memset (void *, int, size_t);
> +int memcmp (const void *, const void *, size_t);
> int isdigit (int);
> int isspace (int);
> int isxdigit (int);
> diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> index e8a76d07ff..33489aa6f4 100644
> --- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> +++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> @@ -46,6 +46,12 @@ void * memset (void *dest, char ch, unsigned int count)
> return dest;
> }
>
> +/* Compare characters in two buffers. */
> +int memcmp (const void *buf1, const void *buf2, unsigned int count)
> +{
> + return (int)(CompareMem(buf1,buf2,(UINTN)(count)));
> +}
> +
> int strcmp (const char *s1, const char *s2)
> {
> return (int)AsciiStrCmp(s1, s2);
>
Can we just suppress the optimization to the intrinsic with VS?
If we can't, then:
- The prototype and the function definition don't match. The last
argument should be "size_t" in the function definition too, not
"unsigned int".
- No need for the explicit cast to UINTN for "count", because size_t
will match UINTN exactly.
- No need for the parens around "count".
- Please add spaces between the arguments, after the commas.
- the comment should say, "compare bytes", not "compare characters".
- In general, INTN cannot be safely cast to "int" (for the return
value). In practice, it is likely safe, because the CompareMem
implementations in edk2 return byte differences, which can be
represented as "int". So I guess this is safe. (I had the same thought
when I reviewed the strcmp() wrapper.)
- Given that we are adding an actual memcmp() function, can we remove
the #define for it? In commit 420e508397c7 ("CryptoPkg: Fix handling of
&strcmp function pointers", 2017-03-23), we did the same for strcmp():
added the function, removed the macro.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 3/4] CryptoPkg/BaseCryptLib: Adding NULL checking in timer() wrapper.
2017-03-31 17:05 ` [Patch 3/4] CryptoPkg/BaseCryptLib: Adding NULL checking in timer() wrapper Qin Long
@ 2017-03-31 18:45 ` Laszlo Ersek
2017-04-01 2:14 ` Long, Qin
0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2017-03-31 18:45 UTC (permalink / raw)
To: Qin Long, edk2-devel; +Cc: ting.ye, hao.a.wu, feng.tian, eric.dong
On 03/31/17 19:05, Qin Long wrote:
> There are some explicit timer(NULL) calls in openssl-1.1.0xx source,
> but the dummy timer() wrapper in ConstantTimeClock.c (used by PEI
> and SMM module) has no any checks on NULL parameter. This will
> cause the memory access issue.
> This patch adds the NULL parameter checking in timer() wrapper.
>
> Cc: Ting Ye <ting.ye@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qin Long <qin.long@intel.com>
> ---
> CryptoPkg/Library/BaseCryptLib/SysCall/ConstantTimeClock.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/ConstantTimeClock.c b/CryptoPkg/Library/BaseCryptLib/SysCall/ConstantTimeClock.c
> index 7f20164999..0cd90434ca 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SysCall/ConstantTimeClock.c
> +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/ConstantTimeClock.c
> @@ -31,8 +31,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>
> time_t time (time_t *timer)
> {
> - *timer = 0;
> - return *timer;
> + if (timer != NULL) {
> + *timer = 0;
> + }
> + return 0;
> }
>
> struct tm * gmtime (const time_t *timer)
>
This looks okay, except the function is called time(), not timer().
Please update the commit message (both subject line and body -- several
instances).
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 4/4] CryptoPkg: One workaround to resolve potential build issue.
2017-03-31 17:05 ` [Patch 4/4] CryptoPkg: One workaround to resolve potential build issue Qin Long
@ 2017-03-31 18:50 ` Laszlo Ersek
2017-04-01 2:27 ` Long, Qin
0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2017-03-31 18:50 UTC (permalink / raw)
To: Qin Long, edk2-devel; +Cc: ting.ye, hao.a.wu, feng.tian, eric.dong
On 03/31/17 19:05, Qin Long wrote:
> This patch duplicates one e_os2.h with minor changes into
> "CryptoPkg/Include/openssl/" as one workaround to resolve some
> external components/modules build issue.
> The BaseCryptLib consumer should ONLY include "BaseCryptLib.h" to
> use the exposed APIs. But some external modules call OpenSSL API
> directly via include "openssl/xxx.h", which causes some macro
> re-definitions errors in e_os2.h (under default _WIN32/_WIN64).
>
> We duplicate one e_os2.h with the following changes to fix this
> kind of macro re-definition error.
> -# if defined(OPENSSL_SYS_UEFI) && !defined(ssize_t)
> +# if defined(OPENSSL_SYS_UEFI) && !defined(ossl_ssize_t)
> +# if !defined(ssize_t)
> # define ossl_ssize_t int
> +# else
> +# define ossl_ssize_t ssize_t
> +# endif
> # define OSSL_SSIZE_MAX INT_MAX
> # endif
>
> This is just one workaround before other external modules were
> cleaned. And this update of e_os2.h could also be upstreaming
> into openssl to eliminate the potential risk.
The update *must* be upstreamed to OpenSSL, now that we are using it
verbatim. I have the same comments here as for patch #1:
- please file an OpenSSL bug for this issue,
- please file a TianoCore BZ for backing out this patch as soon as the
OpenSSL bug is fixed and a new stable release is published,
- please reference the TianoCore BZ in both the commit message *and* the
duplicated header file too, in this patch.
Thanks,
Laszlo
>
> Cc: Ting Ye <ting.ye@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qin Long <qin.long@intel.com>
> ---
> CryptoPkg/Include/openssl/e_os2.h | 315 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 315 insertions(+)
> create mode 100644 CryptoPkg/Include/openssl/e_os2.h
>
> diff --git a/CryptoPkg/Include/openssl/e_os2.h b/CryptoPkg/Include/openssl/e_os2.h
> new file mode 100644
> index 0000000000..a0adbc195b
> --- /dev/null
> +++ b/CryptoPkg/Include/openssl/e_os2.h
> @@ -0,0 +1,315 @@
> +/*
> + * Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.
> + *
> + * Licensed under the OpenSSL license (the "License"). You may not use
> + * this file except in compliance with the License. You can obtain a copy
> + * in the file LICENSE in the source distribution or at
> + * https://www.openssl.org/source/license.html
> + */
> +
> +#ifndef HEADER_E_OS2_H
> +# define HEADER_E_OS2_H
> +
> +# include <openssl/opensslconf.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/******************************************************************************
> + * Detect operating systems. This probably needs completing.
> + * The result is that at least one OPENSSL_SYS_os macro should be defined.
> + * However, if none is defined, Unix is assumed.
> + **/
> +
> +# define OPENSSL_SYS_UNIX
> +
> +/* --------------------- Microsoft operating systems ---------------------- */
> +
> +/*
> + * Note that MSDOS actually denotes 32-bit environments running on top of
> + * MS-DOS, such as DJGPP one.
> + */
> +# if defined(OPENSSL_SYS_MSDOS)
> +# undef OPENSSL_SYS_UNIX
> +# endif
> +
> +/*
> + * For 32 bit environment, there seems to be the CygWin environment and then
> + * all the others that try to do the same thing Microsoft does...
> + */
> +/*
> + * UEFI lives here because it might be built with a Microsoft toolchain and
> + * we need to avoid the false positive match on Windows.
> + */
> +# if defined(OPENSSL_SYS_UEFI)
> +# undef OPENSSL_SYS_UNIX
> +# elif defined(OPENSSL_SYS_UWIN)
> +# undef OPENSSL_SYS_UNIX
> +# define OPENSSL_SYS_WIN32_UWIN
> +# else
> +# if defined(__CYGWIN__) || defined(OPENSSL_SYS_CYGWIN)
> +# undef OPENSSL_SYS_UNIX
> +# define OPENSSL_SYS_WIN32_CYGWIN
> +# else
> +# if defined(_WIN32) || defined(OPENSSL_SYS_WIN32)
> +# undef OPENSSL_SYS_UNIX
> +# if !defined(OPENSSL_SYS_WIN32)
> +# define OPENSSL_SYS_WIN32
> +# endif
> +# endif
> +# if defined(_WIN64) || defined(OPENSSL_SYS_WIN64)
> +# undef OPENSSL_SYS_UNIX
> +# if !defined(OPENSSL_SYS_WIN64)
> +# define OPENSSL_SYS_WIN64
> +# endif
> +# endif
> +# if defined(OPENSSL_SYS_WINNT)
> +# undef OPENSSL_SYS_UNIX
> +# endif
> +# if defined(OPENSSL_SYS_WINCE)
> +# undef OPENSSL_SYS_UNIX
> +# endif
> +# endif
> +# endif
> +
> +/* Anything that tries to look like Microsoft is "Windows" */
> +# if defined(OPENSSL_SYS_WIN32) || defined(OPENSSL_SYS_WIN64) || defined(OPENSSL_SYS_WINNT) || defined(OPENSSL_SYS_WINCE)
> +# undef OPENSSL_SYS_UNIX
> +# define OPENSSL_SYS_WINDOWS
> +# ifndef OPENSSL_SYS_MSDOS
> +# define OPENSSL_SYS_MSDOS
> +# endif
> +# endif
> +
> +/*
> + * DLL settings. This part is a bit tough, because it's up to the
> + * application implementor how he or she will link the application, so it
> + * requires some macro to be used.
> + */
> +# ifdef OPENSSL_SYS_WINDOWS
> +# ifndef OPENSSL_OPT_WINDLL
> +# if defined(_WINDLL) /* This is used when building OpenSSL to
> + * indicate that DLL linkage should be used */
> +# define OPENSSL_OPT_WINDLL
> +# endif
> +# endif
> +# endif
> +
> +/* ------------------------------- OpenVMS -------------------------------- */
> +# if defined(__VMS) || defined(VMS) || defined(OPENSSL_SYS_VMS)
> +# if !defined(OPENSSL_SYS_VMS)
> +# undef OPENSSL_SYS_UNIX
> +# endif
> +# define OPENSSL_SYS_VMS
> +# if defined(__DECC)
> +# define OPENSSL_SYS_VMS_DECC
> +# elif defined(__DECCXX)
> +# define OPENSSL_SYS_VMS_DECC
> +# define OPENSSL_SYS_VMS_DECCXX
> +# else
> +# define OPENSSL_SYS_VMS_NODECC
> +# endif
> +# endif
> +
> +/* -------------------------------- Unix ---------------------------------- */
> +# ifdef OPENSSL_SYS_UNIX
> +# if defined(linux) || defined(__linux__) && !defined(OPENSSL_SYS_LINUX)
> +# define OPENSSL_SYS_LINUX
> +# endif
> +# if defined(_AIX) && !defined(OPENSSL_SYS_AIX)
> +# define OPENSSL_SYS_AIX
> +# endif
> +# endif
> +
> +/* -------------------------------- VOS ----------------------------------- */
> +# if defined(__VOS__) && !defined(OPENSSL_SYS_VOS)
> +# define OPENSSL_SYS_VOS
> +# ifdef __HPPA__
> +# define OPENSSL_SYS_VOS_HPPA
> +# endif
> +# ifdef __IA32__
> +# define OPENSSL_SYS_VOS_IA32
> +# endif
> +# endif
> +
> +/**
> + * That's it for OS-specific stuff
> + *****************************************************************************/
> +
> +/* Specials for I/O an exit */
> +# ifdef OPENSSL_SYS_MSDOS
> +# define OPENSSL_UNISTD_IO <io.h>
> +# define OPENSSL_DECLARE_EXIT extern void exit(int);
> +# else
> +# define OPENSSL_UNISTD_IO OPENSSL_UNISTD
> +# define OPENSSL_DECLARE_EXIT /* declared in unistd.h */
> +# endif
> +
> +/*-
> + * Definitions of OPENSSL_GLOBAL and OPENSSL_EXTERN, to define and declare
> + * certain global symbols that, with some compilers under VMS, have to be
> + * defined and declared explicitly with globaldef and globalref.
> + * Definitions of OPENSSL_EXPORT and OPENSSL_IMPORT, to define and declare
> + * DLL exports and imports for compilers under Win32. These are a little
> + * more complicated to use. Basically, for any library that exports some
> + * global variables, the following code must be present in the header file
> + * that declares them, before OPENSSL_EXTERN is used:
> + *
> + * #ifdef SOME_BUILD_FLAG_MACRO
> + * # undef OPENSSL_EXTERN
> + * # define OPENSSL_EXTERN OPENSSL_EXPORT
> + * #endif
> + *
> + * The default is to have OPENSSL_EXPORT, OPENSSL_EXTERN and OPENSSL_GLOBAL
> + * have some generally sensible values.
> + */
> +
> +# if defined(OPENSSL_SYS_VMS_NODECC)
> +# define OPENSSL_EXPORT globalref
> +# define OPENSSL_EXTERN globalref
> +# define OPENSSL_GLOBAL globaldef
> +# elif defined(OPENSSL_SYS_WINDOWS) && defined(OPENSSL_OPT_WINDLL)
> +# define OPENSSL_EXPORT extern __declspec(dllexport)
> +# define OPENSSL_EXTERN extern __declspec(dllimport)
> +# define OPENSSL_GLOBAL
> +# else
> +# define OPENSSL_EXPORT extern
> +# define OPENSSL_EXTERN extern
> +# define OPENSSL_GLOBAL
> +# endif
> +
> +/*-
> + * Macros to allow global variables to be reached through function calls when
> + * required (if a shared library version requires it, for example.
> + * The way it's done allows definitions like this:
> + *
> + * // in foobar.c
> + * OPENSSL_IMPLEMENT_GLOBAL(int,foobar,0)
> + * // in foobar.h
> + * OPENSSL_DECLARE_GLOBAL(int,foobar);
> + * #define foobar OPENSSL_GLOBAL_REF(foobar)
> + */
> +# ifdef OPENSSL_EXPORT_VAR_AS_FUNCTION
> +# define OPENSSL_IMPLEMENT_GLOBAL(type,name,value) \
> + type *_shadow_##name(void) \
> + { static type _hide_##name=value; return &_hide_##name; }
> +# define OPENSSL_DECLARE_GLOBAL(type,name) type *_shadow_##name(void)
> +# define OPENSSL_GLOBAL_REF(name) (*(_shadow_##name()))
> +# else
> +# define OPENSSL_IMPLEMENT_GLOBAL(type,name,value) OPENSSL_GLOBAL type _shadow_##name=value;
> +# define OPENSSL_DECLARE_GLOBAL(type,name) OPENSSL_EXPORT type _shadow_##name
> +# define OPENSSL_GLOBAL_REF(name) _shadow_##name
> +# endif
> +
> +# ifdef _WIN32
> +# ifdef _WIN64
> +# define ossl_ssize_t __int64
> +# define OSSL_SSIZE_MAX _I64_MAX
> +# else
> +# define ossl_ssize_t int
> +# define OSSL_SSIZE_MAX INT_MAX
> +# endif
> +# endif
> +
> +# if defined(OPENSSL_SYS_UEFI) && !defined(ossl_ssize_t)
> +# if !defined(ssize_t)
> +# define ossl_ssize_t int
> +# else
> +# define ossl_ssize_t ssize_t
> +# endif
> +# define OSSL_SSIZE_MAX INT_MAX
> +# endif
> +
> +# ifndef ossl_ssize_t
> +# define ossl_ssize_t ssize_t
> +# if defined(SSIZE_MAX)
> +# define OSSL_SSIZE_MAX SSIZE_MAX
> +# elif defined(_POSIX_SSIZE_MAX)
> +# define OSSL_SSIZE_MAX _POSIX_SSIZE_MAX
> +# endif
> +# endif
> +
> +# ifdef DEBUG_UNUSED
> +# define __owur __attribute__((__warn_unused_result__))
> +# else
> +# define __owur
> +# endif
> +
> +/* Standard integer types */
> +# if defined(OPENSSL_SYS_UEFI)
> +typedef INT8 int8_t;
> +typedef UINT8 uint8_t;
> +typedef INT16 int16_t;
> +typedef UINT16 uint16_t;
> +typedef INT32 int32_t;
> +typedef UINT32 uint32_t;
> +typedef INT64 int64_t;
> +typedef UINT64 uint64_t;
> +# define PRIu64 "%Lu"
> +# elif (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) || \
> + defined(__osf__) || defined(__sgi) || defined(__hpux) || \
> + defined(OPENSSL_SYS_VMS) || defined (__OpenBSD__)
> +# include <inttypes.h>
> +# elif defined(_MSC_VER) && _MSC_VER<=1500
> +/*
> + * minimally required typdefs for systems not supporting inttypes.h or
> + * stdint.h: currently just older VC++
> + */
> +typedef signed char int8_t;
> +typedef unsigned char uint8_t;
> +typedef short int16_t;
> +typedef unsigned short uint16_t;
> +typedef int int32_t;
> +typedef unsigned int uint32_t;
> +typedef __int64 int64_t;
> +typedef unsigned __int64 uint64_t;
> +# else
> +# include <stdint.h>
> +# endif
> +
> +/*
> + * We need a format operator for some client tools for uint64_t. If inttypes.h
> + * isn't available or did not define it, just go with hard-coded.
> + */
> +# ifndef PRIu64
> +# ifdef SIXTY_FOUR_BIT_LONG
> +# define PRIu64 "lu"
> +# else
> +# define PRIu64 "llu"
> +# endif
> +# endif
> +
> +/* ossl_inline: portable inline definition usable in public headers */
> +# if !defined(inline) && !defined(__cplusplus)
> +# if defined(__STDC_VERSION__) && __STDC_VERSION__>=199901L
> + /* just use inline */
> +# define ossl_inline inline
> +# elif defined(__GNUC__) && __GNUC__>=2
> +# define ossl_inline __inline__
> +# elif defined(_MSC_VER)
> + /*
> + * Visual Studio: inline is available in C++ only, however
> + * __inline is available for C, see
> + * http://msdn.microsoft.com/en-us/library/z8y1yy88.aspx
> + */
> +# define ossl_inline __inline
> +# else
> +# define ossl_inline
> +# endif
> +# else
> +# define ossl_inline inline
> +# endif
> +
> +# if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L
> +# define ossl_noreturn _Noreturn
> +# elif defined(__GNUC__) && __GNUC__ >= 2
> +# define ossl_noreturn __attribute__((noreturn))
> +# else
> +# define ossl_noreturn
> +# endif
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +#endif
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 1/4] CryptoPkg/OpensslLib: Suppress extra build warnings in openssl source
2017-03-31 18:23 ` Laszlo Ersek
@ 2017-04-01 1:10 ` Long, Qin
0 siblings, 0 replies; 13+ messages in thread
From: Long, Qin @ 2017-04-01 1:10 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Ye, Ting, Wu, Hao A, Tian, Feng, Dong, Eric
Exactly. Thanks, Laszlo.
Since we will keep the openssl source original after this upgrade, I have to leverage extra
build option to suppress these two warnings this time.
Yes, I prefer to keep /Werror as possible, I will follow-up the next openssl PR and a TianoCore BZ.
Best Regards & Thanks,
LONG, Qin
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Saturday, April 01, 2017 2:24 AM
> To: Long, Qin; edk2-devel@lists.01.org
> Cc: Ye, Ting; Wu, Hao A; Tian, Feng; Dong, Eric
> Subject: Re: [Patch 1/4] CryptoPkg/OpensslLib: Suppress extra build warnings
> in openssl source
>
> On 03/31/17 19:05, Qin Long wrote:
> > This patch added some extra build options to suppress possible
> > warnings when building openssl source under GCC48 and VS2010. Including:
> >
> > Adding "-Wno-error=maybe-uninitialized" to suppress the following
> > GCC48 build warning:
> > OpensslLib/openssl/ssl/statem/statem_clnt.c:2543:9: error: "len" may
> > be used uninitialized in this function [-Werror=maybe-uninitialized]
> > len += pskhdrlen;
> > ^
>
> This happens in the tls_construct_client_key_exchange() function. After
> reviewing the function, the warning is indeed incorrect. I agree with the fix
> (adding -Wno-error=maybe-uninitialized), but I propose the following too:
>
> - file an upstream OpenSSL bug report about this (the existent "len = 0"
> assignment can be moved to the unconditional part of the code),
> - file a TianoCore BZ as well, so that we don't forget removing -Wno-
> error=maybe-uninitialized once the OpenSSL bug is fixed and we adopt the
> next stable release with the OpenSSL change.
>
> The point is, I think -Werror=maybe-uninitialized is a valuable warning, and it
> might help us catch an actual bug in OpenSSL later on. So I don't think it
> should be turned off permanently.
>
> > And adding "/wd4306" to suppress the following VS2010 build warning:
> > openssl\crypto\asn1\tasn_dec.c(795) : warning C4306: 'type cast' :
> > conversion from 'int' to 'ASN1_VALUE *' of greater size
>
> That again seems like a valid warning, the C standard doesn't guarantee that
> (int) can be converted to a pointer type and vice versa. The code should be
> using intptr_t or uintptr_t (in edk2 we use UINTN for that).
>
> This is the code in question:
>
> case V_ASN1_NULL:
> if (len) {
> ASN1err(ASN1_F_ASN1_EX_C2I, ASN1_R_NULL_IS_WRONG_LENGTH);
> goto err;
> }
> *pval = (ASN1_VALUE *)1;
> break;
>
> This is really awful. But, if it is necessary, it should be
>
> *pval = (ASN1_VALUE *)(uintptr_t)1;
>
> Again, I think it's OK to suppress the warnings for now, but I think we should
> have a longer term reminder to reenable them.
>
> >
> > Cc: Ting Ye <ting.ye@intel.com>
> > Cc: Hao Wu <hao.a.wu@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Qin Long <qin.long@intel.com>
> > ---
> > CryptoPkg/Library/OpensslLib/OpensslLib.inf | 15 ++++++++++-----
> > CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf | 15
> > ++++++++++-----
> > 2 files changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/CryptoPkg/Library/OpensslLib/OpensslLib.inf
> > b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
> > index 42f72f4f1f..cbabb34bdd 100644
> > --- a/CryptoPkg/Library/OpensslLib/OpensslLib.inf
> > +++ b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
> > @@ -535,21 +535,26 @@
> > # C4244: conversion from type1 to type2, possible loss of data
> > # C4245: conversion from type1 to type2, signed/unsigned mismatch
> > # C4267: conversion from size_t to type, possible loss of data
> > + # C4306: 'identifier' : conversion from 'type1' to 'type2' of greater size
> > # C4389: 'operator' : signed/unsigned mismatch (xxxx)
> > # C4702: unreachable code
> > # C4706: assignment within conditional expression
> > #
> > MSFT:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER
> $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702
> /wd4706
> > - MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER
> $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702
> /wd4706
> > - MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER
> $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702
> /wd4706
> > + MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER
> $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389
> /wd4702 /wd4706
> > + MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER
> $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389
> /wd4702 /wd4706
> >
> > INTEL:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -
> U__ICC $(OPENSSL_FLAGS) /w
> > INTEL:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -
> U__ICC $(OPENSSL_FLAGS) /w
> > INTEL:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -
> U__ICC $(OPENSSL_FLAGS) /w
> >
> > - GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
> > - GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -
> DNO_MSABI_VA_FUNCS
> > - GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
> > + #
> > + # Suppress the following build warnings in openssl so we don't break the
> build with -Werror
> > + # -Werror=maybe-uninitialized: there exist some other paths for which
> the variable is not initialized.
>
> If that was true, that would be an actual bug in the OpenSSL code base, and
> suppressing the warning in such a case would also be a bug. We may only
> suppress the warning because it is *invalid* -- there is no such problematic
> code path, only the compiler (incorrectly) thinks there is.
>
> So please update the comment. I would even go as far as including the
> TianoCore BZ number in this comment, for the BZ which tracks the longer-
> term re-enablement of this warning.
>
> Looks good to me otherwise.
>
> (Note: personally I can't reproduce the build issues that I reported, at least
> with GCC48 that I use.)
>
> Thanks!
> Laszlo
>
> > + #
> > + GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -
> Wno-error=maybe-uninitialized
> > + GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -
> Wno-error=maybe-uninitialized -DNO_MSABI_VA_FUNCS
> > + GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -
> Wno-error=maybe-uninitialized
> > GCC:*_*_ARM_CC_FLAGS = $(OPENSSL_FLAGS)
> > GCC:*_*_AARCH64_CC_FLAGS = $(OPENSSL_FLAGS)
> >
> > diff --git a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> > b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> > index cbbb456d70..026b551bca 100644
> > --- a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> > +++ b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> > @@ -496,21 +496,26 @@
> > # C4244: conversion from type1 to type2, possible loss of data
> > # C4245: conversion from type1 to type2, signed/unsigned mismatch
> > # C4267: conversion from size_t to type, possible loss of data
> > + # C4306: 'identifier' : conversion from 'type1' to 'type2' of greater size
> > # C4389: 'operator' : signed/unsigned mismatch (xxxx)
> > # C4702: unreachable code
> > # C4706: assignment within conditional expression
> > #
> > MSFT:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER
> $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702
> /wd4706
> > - MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER
> $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702
> /wd4706
> > - MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER
> $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4389 /wd4702
> /wd4706
> > + MSFT:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER
> $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389
> /wd4702 /wd4706
> > + MSFT:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER
> $(OPENSSL_FLAGS) /wd4090 /wd4244 /wd4245 /wd4267 /wd4306 /wd4389
> /wd4702 /wd4706
> >
> > INTEL:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -
> U__ICC $(OPENSSL_FLAGS) /w
> > INTEL:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -
> U__ICC $(OPENSSL_FLAGS) /w
> > INTEL:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 -U_MSC_VER -
> U__ICC $(OPENSSL_FLAGS) /w
> >
> > - GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
> > - GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -
> DNO_MSABI_VA_FUNCS
> > - GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
> > + #
> > + # Suppress the following build warnings in openssl so we don't break the
> build with -Werror
> > + # -Werror=maybe-uninitialized: there exist some other paths for which
> the variable is not initialized.
> > + #
> > + GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -
> Wno-error=maybe-uninitialized
> > + GCC:*_*_X64_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -
> Wno-error=maybe-uninitialized -DNO_MSABI_VA_FUNCS
> > + GCC:*_*_IPF_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -
> Wno-error=maybe-uninitialized
> > GCC:*_*_ARM_CC_FLAGS = $(OPENSSL_FLAGS)
> > GCC:*_*_AARCH64_CC_FLAGS = $(OPENSSL_FLAGS)
> >
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 3/4] CryptoPkg/BaseCryptLib: Adding NULL checking in timer() wrapper.
2017-03-31 18:45 ` Laszlo Ersek
@ 2017-04-01 2:14 ` Long, Qin
0 siblings, 0 replies; 13+ messages in thread
From: Long, Qin @ 2017-04-01 2:14 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Ye, Ting, Wu, Hao A, Tian, Feng, Dong, Eric
Got it. Will correct the commit message.
Thanks.
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Saturday, April 01, 2017 2:46 AM
> To: Long, Qin; edk2-devel@lists.01.org
> Cc: Ye, Ting; Wu, Hao A; Tian, Feng; Dong, Eric
> Subject: Re: [Patch 3/4] CryptoPkg/BaseCryptLib: Adding NULL checking in
> timer() wrapper.
>
> On 03/31/17 19:05, Qin Long wrote:
> > There are some explicit timer(NULL) calls in openssl-1.1.0xx source,
> > but the dummy timer() wrapper in ConstantTimeClock.c (used by PEI and
> > SMM module) has no any checks on NULL parameter. This will cause the
> > memory access issue.
> > This patch adds the NULL parameter checking in timer() wrapper.
> >
> > Cc: Ting Ye <ting.ye@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Qin Long <qin.long@intel.com>
> > ---
> > CryptoPkg/Library/BaseCryptLib/SysCall/ConstantTimeClock.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git
> > a/CryptoPkg/Library/BaseCryptLib/SysCall/ConstantTimeClock.c
> > b/CryptoPkg/Library/BaseCryptLib/SysCall/ConstantTimeClock.c
> > index 7f20164999..0cd90434ca 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/SysCall/ConstantTimeClock.c
> > +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/ConstantTimeClock.c
> > @@ -31,8 +31,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
> >
> > time_t time (time_t *timer)
> > {
> > - *timer = 0;
> > - return *timer;
> > + if (timer != NULL) {
> > + *timer = 0;
> > + }
> > + return 0;
> > }
> >
> > struct tm * gmtime (const time_t *timer)
> >
>
> This looks okay, except the function is called time(), not timer().
> Please update the commit message (both subject line and body -- several
> instances).
>
> Thanks,
> Laszlo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 4/4] CryptoPkg: One workaround to resolve potential build issue.
2017-03-31 18:50 ` Laszlo Ersek
@ 2017-04-01 2:27 ` Long, Qin
0 siblings, 0 replies; 13+ messages in thread
From: Long, Qin @ 2017-04-01 2:27 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Ye, Ting, Wu, Hao A, Tian, Feng, Dong, Eric
Yes, will follow the openssl upstreaming and next clean-ups.
Thanks.
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Saturday, April 01, 2017 2:50 AM
> To: Long, Qin; edk2-devel@lists.01.org
> Cc: Ye, Ting; Wu, Hao A; Tian, Feng; Dong, Eric
> Subject: Re: [Patch 4/4] CryptoPkg: One workaround to resolve potential
> build issue.
>
> On 03/31/17 19:05, Qin Long wrote:
> > This patch duplicates one e_os2.h with minor changes into
> > "CryptoPkg/Include/openssl/" as one workaround to resolve some
> > external components/modules build issue.
> > The BaseCryptLib consumer should ONLY include "BaseCryptLib.h" to use
> > the exposed APIs. But some external modules call OpenSSL API directly
> > via include "openssl/xxx.h", which causes some macro re-definitions
> > errors in e_os2.h (under default _WIN32/_WIN64).
> >
> > We duplicate one e_os2.h with the following changes to fix this kind
> > of macro re-definition error.
> > -# if defined(OPENSSL_SYS_UEFI) && !defined(ssize_t)
> > +# if defined(OPENSSL_SYS_UEFI) && !defined(ossl_ssize_t)
> > +# if !defined(ssize_t)
> > # define ossl_ssize_t int
> > +# else
> > +# define ossl_ssize_t ssize_t
> > +# endif
> > # define OSSL_SSIZE_MAX INT_MAX
> > # endif
> >
> > This is just one workaround before other external modules were
> > cleaned. And this update of e_os2.h could also be upstreaming into
> > openssl to eliminate the potential risk.
>
> The update *must* be upstreamed to OpenSSL, now that we are using it
> verbatim. I have the same comments here as for patch #1:
>
> - please file an OpenSSL bug for this issue,
> - please file a TianoCore BZ for backing out this patch as soon as the OpenSSL
> bug is fixed and a new stable release is published,
> - please reference the TianoCore BZ in both the commit message *and* the
> duplicated header file too, in this patch.
>
> Thanks,
> Laszlo
>
> >
> > Cc: Ting Ye <ting.ye@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Hao Wu <hao.a.wu@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Qin Long <qin.long@intel.com>
> > ---
> > CryptoPkg/Include/openssl/e_os2.h | 315
> > ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 315 insertions(+)
> > create mode 100644 CryptoPkg/Include/openssl/e_os2.h
> >
> > diff --git a/CryptoPkg/Include/openssl/e_os2.h
> > b/CryptoPkg/Include/openssl/e_os2.h
> > new file mode 100644
> > index 0000000000..a0adbc195b
> > --- /dev/null
> > +++ b/CryptoPkg/Include/openssl/e_os2.h
> > @@ -0,0 +1,315 @@
> > +/*
> > + * Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.
> > + *
> > + * Licensed under the OpenSSL license (the "License"). You may not
> > +use
> > + * this file except in compliance with the License. You can obtain a
> > +copy
> > + * in the file LICENSE in the source distribution or at
> > + * https://www.openssl.org/source/license.html
> > + */
> > +
> > +#ifndef HEADER_E_OS2_H
> > +# define HEADER_E_OS2_H
> > +
> > +# include <openssl/opensslconf.h>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> >
> +/*********************************************************
> ***********
> > +**********
> > + * Detect operating systems. This probably needs completing.
> > + * The result is that at least one OPENSSL_SYS_os macro should be
> defined.
> > + * However, if none is defined, Unix is assumed.
> > + **/
> > +
> > +# define OPENSSL_SYS_UNIX
> > +
> > +/* --------------------- Microsoft operating systems
> > +---------------------- */
> > +
> > +/*
> > + * Note that MSDOS actually denotes 32-bit environments running on
> > +top of
> > + * MS-DOS, such as DJGPP one.
> > + */
> > +# if defined(OPENSSL_SYS_MSDOS)
> > +# undef OPENSSL_SYS_UNIX
> > +# endif
> > +
> > +/*
> > + * For 32 bit environment, there seems to be the CygWin environment
> > +and then
> > + * all the others that try to do the same thing Microsoft does...
> > + */
> > +/*
> > + * UEFI lives here because it might be built with a Microsoft
> > +toolchain and
> > + * we need to avoid the false positive match on Windows.
> > + */
> > +# if defined(OPENSSL_SYS_UEFI)
> > +# undef OPENSSL_SYS_UNIX
> > +# elif defined(OPENSSL_SYS_UWIN)
> > +# undef OPENSSL_SYS_UNIX
> > +# define OPENSSL_SYS_WIN32_UWIN
> > +# else
> > +# if defined(__CYGWIN__) || defined(OPENSSL_SYS_CYGWIN)
> > +# undef OPENSSL_SYS_UNIX
> > +# define OPENSSL_SYS_WIN32_CYGWIN
> > +# else
> > +# if defined(_WIN32) || defined(OPENSSL_SYS_WIN32)
> > +# undef OPENSSL_SYS_UNIX
> > +# if !defined(OPENSSL_SYS_WIN32)
> > +# define OPENSSL_SYS_WIN32
> > +# endif
> > +# endif
> > +# if defined(_WIN64) || defined(OPENSSL_SYS_WIN64)
> > +# undef OPENSSL_SYS_UNIX
> > +# if !defined(OPENSSL_SYS_WIN64)
> > +# define OPENSSL_SYS_WIN64
> > +# endif
> > +# endif
> > +# if defined(OPENSSL_SYS_WINNT)
> > +# undef OPENSSL_SYS_UNIX
> > +# endif
> > +# if defined(OPENSSL_SYS_WINCE)
> > +# undef OPENSSL_SYS_UNIX
> > +# endif
> > +# endif
> > +# endif
> > +
> > +/* Anything that tries to look like Microsoft is "Windows" */ # if
> > +defined(OPENSSL_SYS_WIN32) || defined(OPENSSL_SYS_WIN64) ||
> > +defined(OPENSSL_SYS_WINNT) || defined(OPENSSL_SYS_WINCE) #
> undef
> > +OPENSSL_SYS_UNIX # define OPENSSL_SYS_WINDOWS # ifndef
> > +OPENSSL_SYS_MSDOS
> > +# define OPENSSL_SYS_MSDOS
> > +# endif
> > +# endif
> > +
> > +/*
> > + * DLL settings. This part is a bit tough, because it's up to the
> > + * application implementor how he or she will link the application,
> > +so it
> > + * requires some macro to be used.
> > + */
> > +# ifdef OPENSSL_SYS_WINDOWS
> > +# ifndef OPENSSL_OPT_WINDLL
> > +# if defined(_WINDLL) /* This is used when building OpenSSL to
> > + * indicate that DLL linkage should be used */
> > +# define OPENSSL_OPT_WINDLL
> > +# endif
> > +# endif
> > +# endif
> > +
> > +/* ------------------------------- OpenVMS
> > +-------------------------------- */ # if defined(__VMS) ||
> > +defined(VMS) || defined(OPENSSL_SYS_VMS) #
> if !defined(OPENSSL_SYS_VMS)
> > +# undef OPENSSL_SYS_UNIX
> > +# endif
> > +# define OPENSSL_SYS_VMS
> > +# if defined(__DECC)
> > +# define OPENSSL_SYS_VMS_DECC
> > +# elif defined(__DECCXX)
> > +# define OPENSSL_SYS_VMS_DECC
> > +# define OPENSSL_SYS_VMS_DECCXX
> > +# else
> > +# define OPENSSL_SYS_VMS_NODECC
> > +# endif
> > +# endif
> > +
> > +/* -------------------------------- Unix
> > +---------------------------------- */ # ifdef OPENSSL_SYS_UNIX # if
> > +defined(linux) || defined(__linux__) && !defined(OPENSSL_SYS_LINUX)
> > +# define OPENSSL_SYS_LINUX
> > +# endif
> > +# if defined(_AIX) && !defined(OPENSSL_SYS_AIX)
> > +# define OPENSSL_SYS_AIX
> > +# endif
> > +# endif
> > +
> > +/* -------------------------------- VOS
> > +----------------------------------- */ # if defined(__VOS__) &&
> > +!defined(OPENSSL_SYS_VOS) # define OPENSSL_SYS_VOS # ifdef
> __HPPA__
> > +# define OPENSSL_SYS_VOS_HPPA
> > +# endif
> > +# ifdef __IA32__
> > +# define OPENSSL_SYS_VOS_IA32
> > +# endif
> > +# endif
> > +
> > +/**
> > + * That's it for OS-specific stuff
> > +
> >
> +*********************************************************
> ************
> > +********/
> > +
> > +/* Specials for I/O an exit */
> > +# ifdef OPENSSL_SYS_MSDOS
> > +# define OPENSSL_UNISTD_IO <io.h>
> > +# define OPENSSL_DECLARE_EXIT extern void exit(int); # else #
> > +define OPENSSL_UNISTD_IO OPENSSL_UNISTD # define
> > +OPENSSL_DECLARE_EXIT /* declared in unistd.h */ # endif
> > +
> > +/*-
> > + * Definitions of OPENSSL_GLOBAL and OPENSSL_EXTERN, to define and
> > +declare
> > + * certain global symbols that, with some compilers under VMS, have
> > +to be
> > + * defined and declared explicitly with globaldef and globalref.
> > + * Definitions of OPENSSL_EXPORT and OPENSSL_IMPORT, to define and
> > +declare
> > + * DLL exports and imports for compilers under Win32. These are a
> > +little
> > + * more complicated to use. Basically, for any library that exports
> > +some
> > + * global variables, the following code must be present in the header
> > +file
> > + * that declares them, before OPENSSL_EXTERN is used:
> > + *
> > + * #ifdef SOME_BUILD_FLAG_MACRO
> > + * # undef OPENSSL_EXTERN
> > + * # define OPENSSL_EXTERN OPENSSL_EXPORT
> > + * #endif
> > + *
> > + * The default is to have OPENSSL_EXPORT, OPENSSL_EXTERN and
> > +OPENSSL_GLOBAL
> > + * have some generally sensible values.
> > + */
> > +
> > +# if defined(OPENSSL_SYS_VMS_NODECC)
> > +# define OPENSSL_EXPORT globalref
> > +# define OPENSSL_EXTERN globalref
> > +# define OPENSSL_GLOBAL globaldef
> > +# elif defined(OPENSSL_SYS_WINDOWS) &&
> defined(OPENSSL_OPT_WINDLL) #
> > +define OPENSSL_EXPORT extern __declspec(dllexport) # define
> > +OPENSSL_EXTERN extern __declspec(dllimport) # define
> OPENSSL_GLOBAL
> > +# else # define OPENSSL_EXPORT extern # define OPENSSL_EXTERN
> > +extern # define OPENSSL_GLOBAL # endif
> > +
> > +/*-
> > + * Macros to allow global variables to be reached through function
> > +calls when
> > + * required (if a shared library version requires it, for example.
> > + * The way it's done allows definitions like this:
> > + *
> > + * // in foobar.c
> > + * OPENSSL_IMPLEMENT_GLOBAL(int,foobar,0)
> > + * // in foobar.h
> > + * OPENSSL_DECLARE_GLOBAL(int,foobar);
> > + * #define foobar OPENSSL_GLOBAL_REF(foobar)
> > + */
> > +# ifdef OPENSSL_EXPORT_VAR_AS_FUNCTION
> > +# define OPENSSL_IMPLEMENT_GLOBAL(type,name,value) \
> > + type *_shadow_##name(void) \
> > + { static type _hide_##name=value; return &_hide_##name; } #
> > +define OPENSSL_DECLARE_GLOBAL(type,name) type
> *_shadow_##name(void) #
> > +define OPENSSL_GLOBAL_REF(name) (*(_shadow_##name())) # else #
> > +define OPENSSL_IMPLEMENT_GLOBAL(type,name,value)
> OPENSSL_GLOBAL type
> > +_shadow_##name=value; # define
> OPENSSL_DECLARE_GLOBAL(type,name)
> > +OPENSSL_EXPORT type _shadow_##name # define
> OPENSSL_GLOBAL_REF(name)
> > +_shadow_##name # endif
> > +
> > +# ifdef _WIN32
> > +# ifdef _WIN64
> > +# define ossl_ssize_t __int64
> > +# define OSSL_SSIZE_MAX _I64_MAX
> > +# else
> > +# define ossl_ssize_t int
> > +# define OSSL_SSIZE_MAX INT_MAX
> > +# endif
> > +# endif
> > +
> > +# if defined(OPENSSL_SYS_UEFI) && !defined(ossl_ssize_t) # if
> > +!defined(ssize_t)
> > +# define ossl_ssize_t int
> > +# else
> > +# define ossl_ssize_t ssize_t
> > +# endif
> > +# define OSSL_SSIZE_MAX INT_MAX
> > +# endif
> > +
> > +# ifndef ossl_ssize_t
> > +# define ossl_ssize_t ssize_t
> > +# if defined(SSIZE_MAX)
> > +# define OSSL_SSIZE_MAX SSIZE_MAX
> > +# elif defined(_POSIX_SSIZE_MAX)
> > +# define OSSL_SSIZE_MAX _POSIX_SSIZE_MAX
> > +# endif
> > +# endif
> > +
> > +# ifdef DEBUG_UNUSED
> > +# define __owur __attribute__((__warn_unused_result__))
> > +# else
> > +# define __owur
> > +# endif
> > +
> > +/* Standard integer types */
> > +# if defined(OPENSSL_SYS_UEFI)
> > +typedef INT8 int8_t;
> > +typedef UINT8 uint8_t;
> > +typedef INT16 int16_t;
> > +typedef UINT16 uint16_t;
> > +typedef INT32 int32_t;
> > +typedef UINT32 uint32_t;
> > +typedef INT64 int64_t;
> > +typedef UINT64 uint64_t;
> > +# define PRIu64 "%Lu"
> > +# elif (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L)
> || \
> > + defined(__osf__) || defined(__sgi) || defined(__hpux) || \
> > + defined(OPENSSL_SYS_VMS) || defined (__OpenBSD__) # include
> > +<inttypes.h> # elif defined(_MSC_VER) && _MSC_VER<=1500
> > +/*
> > + * minimally required typdefs for systems not supporting inttypes.h
> > +or
> > + * stdint.h: currently just older VC++ */ typedef signed char
> > +int8_t; typedef unsigned char uint8_t; typedef short int16_t; typedef
> > +unsigned short uint16_t; typedef int int32_t; typedef unsigned int
> > +uint32_t; typedef __int64 int64_t; typedef unsigned __int64 uint64_t;
> > +# else # include <stdint.h> # endif
> > +
> > +/*
> > + * We need a format operator for some client tools for uint64_t. If
> > +inttypes.h
> > + * isn't available or did not define it, just go with hard-coded.
> > + */
> > +# ifndef PRIu64
> > +# ifdef SIXTY_FOUR_BIT_LONG
> > +# define PRIu64 "lu"
> > +# else
> > +# define PRIu64 "llu"
> > +# endif
> > +# endif
> > +
> > +/* ossl_inline: portable inline definition usable in public headers
> > +*/ # if !defined(inline) && !defined(__cplusplus) # if
> > +defined(__STDC_VERSION__) && __STDC_VERSION__>=199901L
> > + /* just use inline */
> > +# define ossl_inline inline
> > +# elif defined(__GNUC__) && __GNUC__>=2
> > +# define ossl_inline __inline__
> > +# elif defined(_MSC_VER)
> > + /*
> > + * Visual Studio: inline is available in C++ only, however
> > + * __inline is available for C, see
> > + * http://msdn.microsoft.com/en-us/library/z8y1yy88.aspx
> > + */
> > +# define ossl_inline __inline
> > +# else
> > +# define ossl_inline
> > +# endif
> > +# else
> > +# define ossl_inline inline
> > +# endif
> > +
> > +# if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L #
> > +define ossl_noreturn _Noreturn # elif defined(__GNUC__) &&
> __GNUC__
> > +>= 2 # define ossl_noreturn __attribute__((noreturn)) # else #
> > +define ossl_noreturn # endif
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +#endif
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch 2/4] CryptoPkg: Fix possible unresolved external symbol issue.
2017-03-31 18:44 ` Laszlo Ersek
@ 2017-04-01 2:31 ` Long, Qin
0 siblings, 0 replies; 13+ messages in thread
From: Long, Qin @ 2017-04-01 2:31 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Ye, Ting, Wu, Hao A, Tian, Feng, Dong, Eric
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Saturday, April 01, 2017 2:45 AM
> To: Long, Qin; edk2-devel@lists.01.org
> Cc: Ye, Ting; Wu, Hao A; Tian, Feng; Dong, Eric
> Subject: Re: [Patch 2/4] CryptoPkg: Fix possible unresolved external symbol
> issue.
>
> On 03/31/17 19:05, Qin Long wrote:
> > The compiler (visual studio) may optimize some explicit strcmp call to
> > use the intrinsic memcmp call. In CrtLibSupport.h, we just use
> > #define to mapping memcmp to CompareMem API. So in Link phase, this
> > kind of intrinsic optimization will cause the "unresolved external
> > symbol" error. For example:
> > OpensslLib.lib(v3_utl.obj) : error LNK2001:
> > unresolved external symbol _memcmp
> >
> > This patch will keep the memcmp mapping, and provide extra Intrinsic
> > memcmp wrapper to satisfy the symbol link.
> >
> > Cc: Ting Ye <ting.ye@intel.com>
> > Cc: Feng Tian <feng.tian@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Qin Long <qin.long@intel.com>
> > ---
> > CryptoPkg/Include/CrtLibSupport.h | 1 +
> > CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c | 6 ++++++
> > 2 files changed, 7 insertions(+)
> >
> > diff --git a/CryptoPkg/Include/CrtLibSupport.h
> > b/CryptoPkg/Include/CrtLibSupport.h
> > index ddf7784a37..7f1ec12302 100644
> > --- a/CryptoPkg/Include/CrtLibSupport.h
> > +++ b/CryptoPkg/Include/CrtLibSupport.h
> > @@ -133,6 +133,7 @@ void *malloc (size_t);
> > void *realloc (void *, size_t);
> > void free (void *);
> > void *memset (void *, int, size_t);
> > +int memcmp (const void *, const void *, size_t);
> > int isdigit (int);
> > int isspace (int);
> > int isxdigit (int);
> > diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > index e8a76d07ff..33489aa6f4 100644
> > --- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > +++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
> > @@ -46,6 +46,12 @@ void * memset (void *dest, char ch, unsigned int
> count)
> > return dest;
> > }
> >
> > +/* Compare characters in two buffers. */ int memcmp (const void
> > +*buf1, const void *buf2, unsigned int count) {
> > + return (int)(CompareMem(buf1,buf2,(UINTN)(count)));
> > +}
> > +
> > int strcmp (const char *s1, const char *s2) {
> > return (int)AsciiStrCmp(s1, s2);
> >
>
> Can we just suppress the optimization to the intrinsic with VS?
Noop.
We leverage the global optimization option (/GL) under VS. So the compiler
may still produce the intrinsic replacement even if we use some kind of
no-instrinsic options. :-(
>
> If we can't, then:
>
> - The prototype and the function definition don't match. The last argument
> should be "size_t" in the function definition too, not "unsigned int".
>
> - No need for the explicit cast to UINTN for "count", because size_t will
> match UINTN exactly.
>
> - No need for the parens around "count".
>
> - Please add spaces between the arguments, after the commas.
>
> - the comment should say, "compare bytes", not "compare characters".
Make sense for me. Will reproduce the patch to refine these.
>
> - In general, INTN cannot be safely cast to "int" (for the return value). In
> practice, it is likely safe, because the CompareMem implementations in edk2
> return byte differences, which can be represented as "int". So I guess this is
> safe. (I had the same thought when I reviewed the strcmp() wrapper.)
>
> - Given that we are adding an actual memcmp() function, can we remove the
> #define for it? In commit 420e508397c7 ("CryptoPkg: Fix handling of &strcmp
> function pointers", 2017-03-23), we did the same for strcmp():
> added the function, removed the macro.
I prefer to keep the memcmp mapping which will replace those explicit memcmp
calls (several places in openssl source) to UEFI functions directly.
The intrinsic memcmp wrapper is just to link those optimized _memcmp symbol.
>
> Thanks,
> Laszlo
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-04-01 2:31 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-31 17:05 [Patch 0/4] *** Resolving CryptoPkg build issues *** Qin Long
2017-03-31 17:05 ` [Patch 1/4] CryptoPkg/OpensslLib: Suppress extra build warnings in openssl source Qin Long
2017-03-31 18:23 ` Laszlo Ersek
2017-04-01 1:10 ` Long, Qin
2017-03-31 17:05 ` [Patch 2/4] CryptoPkg: Fix possible unresolved external symbol issue Qin Long
2017-03-31 18:44 ` Laszlo Ersek
2017-04-01 2:31 ` Long, Qin
2017-03-31 17:05 ` [Patch 3/4] CryptoPkg/BaseCryptLib: Adding NULL checking in timer() wrapper Qin Long
2017-03-31 18:45 ` Laszlo Ersek
2017-04-01 2:14 ` Long, Qin
2017-03-31 17:05 ` [Patch 4/4] CryptoPkg: One workaround to resolve potential build issue Qin Long
2017-03-31 18:50 ` Laszlo Ersek
2017-04-01 2:27 ` Long, Qin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox