fix warnings in 9.6, 10, 11's contrib when compiling without openssl
Hello,
after plain ./configure without options (including noticeable absence of
--with-openssl) and make,
make -C contrib COPT=-Werror gives error with gcc-11 on REL_9_6_STABLE,
REL_10_STABLE or REL_11_STABLE.
The warning/error is about misleading indent in
contrib/pgcrypto/imath.c's CLAMP macro:
imath.c: In function ‘mp_int_add’:
imath.c:133:1: error: this ‘while’ clause does not guard...
[-Werror=misleading-indentation]
133 | while(uz_ > 1 && (*dz_-- == 0)) --uz_;MP_USED(z_)=uz_;}while(0)
| ^~~~~
imath.c:670:17: note: in expansion of macro ‘CLAMP’
670 | CLAMP(c);
| ^~~~~
In file included from imath.c:34:
imath.h:68:26: note: ...this statement, but the latter is misleadingly
indented as if it were guarded by the ‘while’
68 | #define MP_USED(Z) ((Z)->used)
| ^
imath.c:133:39: note: in expansion of macro ‘MP_USED’
133 | while(uz_ > 1 && (*dz_-- == 0)) --uz_;MP_USED(z_)=uz_;}while(0)
| ^~~~~~~
imath.c:670:17: note: in expansion of macro ‘CLAMP’
670 | CLAMP(c);
| ^~~~~
pgcrypto-fix-imath-clamp-warning.patch, attached, is a suggested minimal
fix:
diff --git a/contrib/pgcrypto/imath.c b/contrib/pgcrypto/imath.c
index b94a51b81a4..801b843cbe3 100644
--- a/contrib/pgcrypto/imath.c
+++ b/contrib/pgcrypto/imath.c
@@ -130,7 +130,8 @@ do{T *u_=(A),*v_=u_+(N)-1;while(u_<v_){T
xch=*u_;*u_++=*v_;*v_--=xch;}}while(0)
#else
#define CLAMP(Z) \
do{mp_int z_=(Z);mp_size uz_=MP_USED(z_);mp_digit
*dz_=MP_DIGITS(z_)+uz_-1;\
-while(uz_ > 1 && (*dz_-- == 0)) --uz_;MP_USED(z_)=uz_;}while(0)
+while(uz_ > 1 && (*dz_-- == 0)) --uz_;\
+MP_USED(z_)=uz_;}while(0)
#endif#undef MIN
The same patch works for all 9.6, 10 and 11. It's not needed in 12 or
later, they compile without warnings just fine even without --with-openssl.
In addition, 9.6 (unlike 10 and 11) gives four "array-parameter="
warnings about contrib/pgcrypto/sha2.c:
sha2.c:552:20: error: argument 1 of type ‘uint8[]’ {aka ‘unsigned
char[]’} with mismatched bound [-Werror=array-parameter=]
552 | SHA256_Final(uint8 digest[], SHA256_CTX *context)
| ~~~~~~^~~~~~~~
In file included from sha2.c:44:
sha2.h:93:30: note: previously declared as ‘uint8[32]’ {aka ‘unsigned
char[32]’}
93 | void SHA256_Final(uint8[SHA256_DIGEST_LENGTH],
SHA256_CTX *);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
and the same for SHA{512,384,224}_Final.
pgcrypto-fix-sha2-warning.patch, attached, is a suggested minimal fix
for that:
void
-SHA256_Final(uint8 digest[], SHA256_CTX *context)
+SHA256_Final(uint8 digest[SHA256_DIGEST_LENGTH], SHA256_CTX *context)
{
etc.Please consider fixing those warnings.
--
Anton Voloshin
Postgres Professional, The Russian Postgres Company
https://postgrespro.ru
Attachments:
pgcrypto-fix-imath-clamp-warning.patchtext/x-patch; charset=UTF-8; name=pgcrypto-fix-imath-clamp-warning.patchDownload
diff --git a/contrib/pgcrypto/imath.c b/contrib/pgcrypto/imath.c
index b94a51b81a4..801b843cbe3 100644
--- a/contrib/pgcrypto/imath.c
+++ b/contrib/pgcrypto/imath.c
@@ -130,7 +130,8 @@ do{T *u_=(A),*v_=u_+(N)-1;while(u_<v_){T xch=*u_;*u_++=*v_;*v_--=xch;}}while(0)
#else
#define CLAMP(Z) \
do{mp_int z_=(Z);mp_size uz_=MP_USED(z_);mp_digit *dz_=MP_DIGITS(z_)+uz_-1;\
-while(uz_ > 1 && (*dz_-- == 0)) --uz_;MP_USED(z_)=uz_;}while(0)
+while(uz_ > 1 && (*dz_-- == 0)) --uz_;\
+MP_USED(z_)=uz_;}while(0)
#endif
#undef MIN
pgcrypto-fix-sha2-warning.patchtext/x-patch; charset=UTF-8; name=pgcrypto-fix-sha2-warning.patchDownload
diff --git a/contrib/pgcrypto/sha2.c b/contrib/pgcrypto/sha2.c
index 231f9dfbb0e..11311deda8d 100644
--- a/contrib/pgcrypto/sha2.c
+++ b/contrib/pgcrypto/sha2.c
@@ -549,7 +549,7 @@ SHA256_Last(SHA256_CTX *context)
}
void
-SHA256_Final(uint8 digest[], SHA256_CTX *context)
+SHA256_Final(uint8 digest[SHA256_DIGEST_LENGTH], SHA256_CTX *context)
{
/* If no digest buffer is passed, we don't bother doing this: */
if (digest != NULL)
@@ -877,7 +877,7 @@ SHA512_Last(SHA512_CTX *context)
}
void
-SHA512_Final(uint8 digest[], SHA512_CTX *context)
+SHA512_Final(uint8 digest[SHA512_DIGEST_LENGTH], SHA512_CTX *context)
{
/* If no digest buffer is passed, we don't bother doing this: */
if (digest != NULL)
@@ -922,7 +922,7 @@ SHA384_Update(SHA384_CTX *context, const uint8 *data, size_t len)
}
void
-SHA384_Final(uint8 digest[], SHA384_CTX *context)
+SHA384_Final(uint8 digest[SHA384_DIGEST_LENGTH], SHA384_CTX *context)
{
/* If no digest buffer is passed, we don't bother doing this: */
if (digest != NULL)
@@ -966,7 +966,7 @@ SHA224_Update(SHA224_CTX *context, const uint8 *data, size_t len)
}
void
-SHA224_Final(uint8 digest[], SHA224_CTX *context)
+SHA224_Final(uint8 digest[SHA224_DIGEST_LENGTH], SHA224_CTX *context)
{
/* If no digest buffer is passed, we don't bother doing this: */
if (digest != NULL)
On 10 Nov 2021, at 11:14, Anton Voloshin <a.voloshin@postgrespro.ru> wrote:
The warning/error is about misleading indent in contrib/pgcrypto/imath.c's CLAMP macro:
This is upstream code in a stable branch formatted by pgindent at the time of
the release, I'm not sure the churn is worth it for fixing warnings for
non-standard compiler options here. I don't see any of these warnings skimming
logs in the buildfarm.
In addition, 9.6 (unlike 10 and 11) gives four "array-parameter=" warnings about contrib/pgcrypto/sha2.c:
9.6 will be EOL from the release tomorrow, no more fixes are applied to that
branch.
--
Daniel Gustafsson https://vmware.com/
On Wed, Nov 10, 2021 at 02:56:00PM +0100, Daniel Gustafsson wrote:
This is upstream code in a stable branch formatted by pgindent at the time of
the release, I'm not sure the churn is worth it for fixing warnings for
non-standard compiler options here. I don't see any of these warnings skimming
logs in the buildfarm.
I would suggest to send any improvements for imath directly to
upstream, then Postgres would just feed on that when we update it:
https://github.com/creachadair/imath
Peter has removed this code as of db7d1a7, but there could still be a
point in updating imath on the stable branches where it exists if
there is an issue with it impacting pgcrypto without OpenSSL.
--
Michael
On 11/11/2021 08:14, Michael Paquier wrote:
I would suggest to send any improvements for imath directly to
upstream, then Postgres would just feed on that when we update it:
https://github.com/creachadair/imath
I didn't realise imath comes (occasionally) from external repo. Thank you.
The compilation warnings are only relevant for their old releases,
before imath v1.27 where they've replaced macros with inline functions, see
https://github.com/creachadair/imath/commit/d4760eef8
So imath in upstream doesn't require updates (especially since those
warnings come specifically from pgindent's work, not from the upstream
source). But Postgres' 10_STABLE and 11_STABLE still use old version
with CLAMP as a macro.
Noah Misch was last to update imath from upstream in February 2019 and
that change became part of REL_12_STABLE and later:
Import changes from IMath versions (1.3, 1.29].
Noah and others, do you think it would be a good idea to update imath in
REL_10_STABLE and REL_11_STABLE to a later upstream version to avoid
those warnings about misleading indent? (see beginning of this thread)
Admittedly, those warnings can only be seen in unusual circumstances (no
openssl), but still, as you have mentioned in your commit message,
We're better off naively tracking upstream than reactively
maintaining a twelve-year-old snapshot of upstream.
Perhaps even update imath in all relevant stable branches, 10..14
inclusive, to the same upstream version?
On 11/11/2021 08:14, Michael Paquier wrote:
Peter has removed this code as of db7d1a7, but there could still be a
point in updating imath on the stable branches where it exists if
there is an issue with it impacting pgcrypto without OpenSSL.
Perhaps that would be a good idea, see above.
--
Anton Voloshin
https://postgrespro.ru
Postgres Professional, The Russian Postgres Company