Обсуждение: Conflicting declarations for b64_encode etc. on Solaris 11.4 Beta

I just tried to compile postgresql 10.1 on Solaris 11.4 Beta with the
bundled GCC 5.5 and failed in two places:

/vol/src/postgresql/postgresql/postgresql-10.1/src/backend/utils/adt/encode.c:218:1: error: conflicting types for
‘b64_encode’
 b64_encode(const char *src, unsigned len, char *dst)
 ^
In file included from /vol/src/postgresql/postgresql/postgresql-10.1/src/include/c.h:83:0,
                 from /vol/src/postgresql/postgresql/postgresql-10.1/src/include/postgres.h:47,
                 from /vol/src/postgresql/postgresql/postgresql-10.1/src/backend/utils/adt/encode.c:14:
/usr/include/string.h:218:16: note: previous declaration of ‘b64_encode’ was here
 extern ssize_t b64_encode(char *_RESTRICT_KYWD outbuf, size_t outbufsz,
                ^
/vol/src/postgresql/postgresql/postgresql-10.1/src/backend/utils/adt/encode.c:265:1: error: conflicting types for
‘b64_decode’
 b64_decode(const char *src, unsigned len, char *dst)
 ^
In file included from /vol/src/postgresql/postgresql/postgresql-10.1/src/include/c.h:83:0,
                 from /vol/src/postgresql/postgresql/postgresql-10.1/src/include/postgres.h:47,
                 from /vol/src/postgresql/postgresql/postgresql-10.1/src/backend/utils/adt/encode.c:14:
/usr/include/string.h:221:16: note: previous declaration of ‘b64_decode’ was here
 extern ssize_t b64_decode(void *outbuf, size_t outbufsz, const char *inbuf,
                ^
make[4]: *** [<builtin>: encode.o] Error 1

Beside the static definition of b64_encode and b64_decode in
src/backend/utils/adt/encode.c, both functions are also declared in <string.h>:

#if defined(__EXTENSIONS__) ||                    \
    (!defined(_STRICT_STDC) && !defined(__XOPEN_OR_POSIX))
extern ssize_t b64_encode(char *_RESTRICT_KYWD outbuf, size_t outbufsz,
    const void *_RESTRICT_KYWD inbuf, size_t inbufsz, const char *alpha,
    uint64_t flags);
extern ssize_t b64_decode(void *outbuf, size_t outbufsz, const char *inbuf,
    const char *alpha, uint64_t flags);

During the compilation, neither was __EXTENSIONS__ defined nor any macro
that would lead to _STRICT_STDC or _XOPEN_OR_POSIX being defined (any of
_STDC_VERSION/_XOPEN_SOURCE/_POSIX_SOURCE).

During make world, I ran into the same problem in contrib/pgcrypto/pgp-armor.c.

There are already a couple of instances of those functions with a pg_
prefix, obviously to avoid conflict with differing b64_{encode,decode}
declarations on other systems, but they don't match exactly:
e.g. src/include/common/base64.h has

extern int    pg_b64_encode(const char *src, int len, char *dst);
extern int    pg_b64_decode(const char *src, int len, char *dst);

while the encode.c and pgp-armor.c versions use an unsigned return value
and len argument.

However, since those two latter versions are static, adding a pg_ prefix
there, too, worked without conflict, allowed the compilation to finish
and make check to succeed.

    Rainer

--
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University


===================================================================
RCS file: contrib/pgcrypto/RCS/pgp-armor.c,v
retrieving revision 1.1
diff -up -r1.1 contrib/pgcrypto/pgp-armor.c
--- contrib/pgcrypto/pgp-armor.c    2018/01/23 14:42:44    1.1
+++ contrib/pgcrypto/pgp-armor.c    2018/01/23 14:43:07
@@ -42,7 +42,7 @@ static const unsigned char _base64[] =
 "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
 
 static int
-b64_encode(const uint8 *src, unsigned len, uint8 *dst)
+pg_b64_encode(const uint8 *src, unsigned len, uint8 *dst)
 {
     uint8       *p,
                *lend = dst + 76;
@@ -92,7 +92,7 @@ b64_encode(const uint8 *src, unsigned le
 
 /* probably should use lookup table */
 static int
-b64_decode(const uint8 *src, unsigned len, uint8 *dst)
+pg_b64_decode(const uint8 *src, unsigned len, uint8 *dst)
 {
     const uint8 *srcend = src + len,
                *s = src;
@@ -160,7 +160,7 @@ b64_decode(const uint8 *src, unsigned le
 }
 
 static unsigned
-b64_enc_len(unsigned srclen)
+pg_b64_enc_len(unsigned srclen)
 {
     /*
      * 3 bytes will be converted to 4, linefeed after 76 chars
@@ -169,7 +169,7 @@ b64_enc_len(unsigned srclen)
 }
 
 static unsigned
-b64_dec_len(unsigned srclen)
+pg_b64_dec_len(unsigned srclen)
 {
     return (srclen * 3) >> 2;
 }
@@ -218,11 +218,11 @@ pgp_armor_encode(const uint8 *src, unsig
         appendStringInfo(dst, "%s: %s\n", keys[n], values[n]);
     appendStringInfoChar(dst, '\n');
 
-    /* make sure we have enough room to b64_encode() */
-    b64len = b64_enc_len(len);
+    /* make sure we have enough room to pg_b64_encode() */
+    b64len = pg_b64_enc_len(len);
     enlargeStringInfo(dst, (int) b64len);
 
-    res = b64_encode(src, len, (uint8 *) dst->data + dst->len);
+    res = pg_b64_encode(src, len, (uint8 *) dst->data + dst->len);
     if (res > b64len)
         elog(FATAL, "overflow - encode estimate too small");
     dst->len += res;
@@ -358,14 +358,14 @@ pgp_armor_decode(const uint8 *src, int l
         goto out;
 
     /* decode crc */
-    if (b64_decode(p + 1, 4, buf) != 3)
+    if (pg_b64_decode(p + 1, 4, buf) != 3)
         goto out;
     crc = (((long) buf[0]) << 16) + (((long) buf[1]) << 8) + (long) buf[2];
 
     /* decode data */
-    blen = (int) b64_dec_len(len);
+    blen = (int) pg_b64_dec_len(len);
     enlargeStringInfo(dst, blen);
-    res = b64_decode(base64_start, base64_end - base64_start, (uint8 *) dst->data);
+    res = pg_b64_decode(base64_start, base64_end - base64_start, (uint8 *) dst->data);
     if (res > blen)
         elog(FATAL, "overflow - decode estimate too small");
     if (res >= 0)
===================================================================
RCS file: src/backend/utils/adt/RCS/encode.c,v
retrieving revision 1.1
diff -up -r1.1 src/backend/utils/adt/encode.c
--- src/backend/utils/adt/encode.c    2018/01/23 14:26:39    1.1
+++ src/backend/utils/adt/encode.c    2018/01/23 14:27:04
@@ -215,7 +215,7 @@ static const int8 b64lookup[128] = {
 };
 
 static unsigned
-b64_encode(const char *src, unsigned len, char *dst)
+pg_b64_encode(const char *src, unsigned len, char *dst)
 {
     char       *p,
                *lend = dst + 76;
@@ -262,7 +262,7 @@ b64_encode(const char *src, unsigned len
 }
 
 static unsigned
-b64_decode(const char *src, unsigned len, char *dst)
+pg_b64_decode(const char *src, unsigned len, char *dst)
 {
     const char *srcend = src + len,
                *s = src;
@@ -332,14 +332,14 @@ b64_decode(const char *src, unsigned len
 
 
 static unsigned
-b64_enc_len(const char *src, unsigned srclen)
+pg_b64_enc_len(const char *src, unsigned srclen)
 {
     /* 3 bytes will be converted to 4, linefeed after 76 chars */
     return (srclen + 2) * 4 / 3 + srclen / (76 * 3 / 4);
 }
 
 static unsigned
-b64_dec_len(const char *src, unsigned srclen)
+pg_b64_dec_len(const char *src, unsigned srclen)
 {
     return (srclen * 3) >> 2;
 }
@@ -532,7 +532,7 @@ static const struct
     {
         "base64",
         {
-            b64_enc_len, b64_dec_len, b64_encode, b64_decode
+            pg_b64_enc_len, pg_b64_dec_len, pg_b64_encode, pg_b64_decode
         }
     },
     {

On Tue, Jan 23, 2018 at 04:45:50PM +0100, Rainer Orth wrote:
> There are already a couple of instances of those functions with a pg_
> prefix, obviously to avoid conflict with differing b64_{encode,decode}
> declarations on other systems, but they don't match exactly:
> e.g. src/include/common/base64.h has
>
> extern int    pg_b64_encode(const char *src, int len, char *dst);
> extern int    pg_b64_decode(const char *src, int len, char *dst);

Those are new as of v10, being used by the SCRAM implementation with a
more generic API designed for this purpose. The main difference with
what is in encode.c is the whitespace handling to cope with what RFC
5802 requires.

> while the encode.c and pgp-armor.c versions use an unsigned return value
> and len argument.
>
> However, since those two latter versions are static, adding a pg_ prefix
> there, too, worked without conflict, allowed the compilation to finish
> and make check to succeed.

I am not much a fan of using the same function name for both the static
functions in pgcrypto and encode.c, as well as what is in
src/common/. In order to avoid any conflicts, why not just changing at
least the prefix from "b64" to "base64"? That's not completely
problem-proof for the problem either, as php has a base64_encode for
example. So my suggestion would be to change the prefix on all branches
and to append pg_ to all the routines in encode.c for
consistency. Better naming suggestions welcome.
--
Michael

On Wed, Jan 24, 2018 at 11:16:11AM +0900, Michael Paquier wrote:
> I am not much a fan of using the same function name for both the
> static functions in pgcrypto and encode.c, as well as what is in
> src/common/. In order to avoid any conflicts, why not just changing at
> least the prefix from "b64" to "base64"? That's not completely
> problem-proof for the problem either, as php has a base64_encode for
> example. So my suggestion would be to change the prefix on all
> branches and to append pg_ to all the routines in encode.c for
> consistency. Better naming suggestions welcome.

Attached is an updated patch which does a bit more consistency work.  I
have renamed the base64 functions with pg_base64_ as prefix in encode.c,
to avoid any conflicts in encode.c.  pgp-armor.c also gets the same
treatment.  There is no real point in renaming the other functions is
not necessary, and hex_encode/hex_decode are publicly available, so
renaming them would cause breakage for any callers of them in plugins.

It is a bit sad that both pgcrypto and encode.c hold copies of base64
functions.  If one looks closely, they use the same logic for
pg_base64_encode, pg_base64_enc_len and pg_base64_dec_len.  Each
pg_b64_decode uses different code paths for error handling, but they
actually have the same conversion logic (see b64_test.c attached which
emulates both, I was too lazy to read and compare b64lookup).  One way
to remove the duplication is to extend the _decode API with a path to
not complain with elog and return a status code, then have encode.c use
a wrapper on top of it, but that may not be worth the complication, and
both code copies are not going to change anyway.  At the end, just the
attached would do the work.

Rainer, could you double-check if this solves your problem with
Solaris 11?  If you expect this OS to be supported, it would be nice if
you could add a buildfarm machine:
https://wiki.postgresql.org/wiki/PostgreSQL_Buildfarm_Howto
https://buildfarm.postgresql.org/
There are currently two machines running Solaris 10, maintained by Dave
Page: protosciurus and castoroides.  But there is nothing for Solaris
11.

I am adding that to the next commit fest for consideration as well
under my name.  I'll take care of any reviews and/or feedback.  I'll add
as well your name as author if I can find your community account, I hope
you don't mind.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> Attached is an updated patch which does a bit more consistency work.  I
> have renamed the base64 functions with pg_base64_ as prefix in encode.c,
> to avoid any conflicts in encode.c.  pgp-armor.c also gets the same
> treatment.  There is no real point in renaming the other functions is
> not necessary, and hex_encode/hex_decode are publicly available, so
> renaming them would cause breakage for any callers of them in plugins.

Pushed.

> It is a bit sad that both pgcrypto and encode.c hold copies of base64
> functions.

And on top of that, there's src/common/base64.c with again almost the
same functionality.  But refactoring to fix that would be a bit invasive
and not something to back-patch.  I think what you did here is appropriate
as a minimal portability fix.  Maybe later somebody will look into
removing the duplication, as a HEAD-only improvement.

            regards, tom lane

On Wed, Feb 28, 2018 at 06:37:27PM -0500, Tom Lane wrote:
> And on top of that, there's src/common/base64.c with again almost the
> same functionality.  But refactoring to fix that would be a bit invasive
> and not something to back-patch.  I think what you did here is appropriate
> as a minimal portability fix.  Maybe later somebody will look into
> removing the duplication, as a HEAD-only improvement.

Thanks for pushing the patch.

base64.c ignores some whitespace handling, which is in line with what
the RFCs of SCRAM expect when doing conversion of the data exchanged in
the protocol so that's a bit more different than the other
implementations.  Maybe that's worth refactoring, still I am not
completely convinced that we have much to gain from such a move either.
--
Michael