Fix misuse use of pg_b64_encode function (contrib/postgres_fdw/connection.c)
Hi.
Per Coverity.
CID 1590024: (CHECKED_RETURN)
Calling "pg_b64_encode" without checking return value (as is done elsewhere
8 out of 10 times).
The function *pg_b64_encode* has in the comments:
[0]: I think the most correct would be *or* not *and* word?
So, the function can fail.
All other calls check the return, In this case it could not be different.
Fix by checking the return and reporting a message to the user,
in case of failure.
best regards,
Ranier Vilela
[0]: I think the most correct would be *or* not *and* word?
Attachments:
fix-misuse-function-pg_b64_encode.patchapplication/octet-stream; name=fix-misuse-function-pg_b64_encode.patchDownload
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 0274d6c253..c9e4c3b2ae 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -559,23 +559,32 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
if (MyProcPort->has_scram_keys && UseScramPassthrough(server, user))
{
int len;
+ int encoded_len;
keywords[n] = "scram_client_key";
len = pg_b64_enc_len(sizeof(MyProcPort->scram_ClientKey));
/* don't forget the zero-terminator */
values[n] = palloc0(len + 1);
- pg_b64_encode((const char *) MyProcPort->scram_ClientKey,
+ encoded_len = pg_b64_encode((const char *) MyProcPort->scram_ClientKey,
sizeof(MyProcPort->scram_ClientKey),
(char *) values[n], len);
+ if (encoded_len < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
+ errmsg("could not encode scram client key")));
n++;
keywords[n] = "scram_server_key";
len = pg_b64_enc_len(sizeof(MyProcPort->scram_ServerKey));
/* don't forget the zero-terminator */
values[n] = palloc0(len + 1);
- pg_b64_encode((const char *) MyProcPort->scram_ServerKey,
+ encoded_len = pg_b64_encode((const char *) MyProcPort->scram_ServerKey,
sizeof(MyProcPort->scram_ServerKey),
(char *) values[n], len);
+ if (encoded_len < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
+ errmsg("could not encode scram server key")));
n++;
}
On Wed, Jan 15, 2025 at 10:12:51PM -0300, Ranier Vilela wrote:
Fix by checking the return and reporting a message to the user,
in case of failure.
You are right that it is inconsistent to not check the result returned
by these two calls of pg_b64_encode(), as introduced in 761c79508e7f.
Peter?
--
Michael
On 16.01.25 02:12, Ranier Vilela wrote:
Per Coverity.
CID 1590024: (CHECKED_RETURN)
Calling "pg_b64_encode" without checking return value (as is done
elsewhere 8 out of 10 times).The function *pg_b64_encode* has in the comments:
[0] "and -1 in the event of an error"So, the function can fail.
All other calls check the return, In this case it could not be different.Fix by checking the return and reporting a message to the user,
in case of failure.
Thanks, fixed. (I changed the ereports to elogs, which is how other
call sites do it.)
I also fixed a related problem in the pg_b64_decode() calls in libpq.
Maybe we could put a pg_nodiscard attribute on pg_b64_encode() and
pg_b64_decode()?
[0] I think the most correct would be *or* not *and* word?
I think both are ok here.
On Thu, Jan 16, 2025 at 09:07:45AM +0100, Peter Eisentraut wrote:
Maybe we could put a pg_nodiscard attribute on pg_b64_encode() and
pg_b64_decode()?
Sounds like a good idea to me. +1.
--
Michael
Em qui., 16 de jan. de 2025 às 05:07, Peter Eisentraut <peter@eisentraut.org>
escreveu:
On 16.01.25 02:12, Ranier Vilela wrote:
Per Coverity.
CID 1590024: (CHECKED_RETURN)
Calling "pg_b64_encode" without checking return value (as is done
elsewhere 8 out of 10 times).The function *pg_b64_encode* has in the comments:
[0] "and -1 in the event of an error"So, the function can fail.
All other calls check the return, In this case it could not be different.Fix by checking the return and reporting a message to the user,
in case of failure.Thanks, fixed. (I changed the ereports to elogs, which is how other
call sites do it.)
Thank you.
I also fixed a related problem in the pg_b64_decode() calls in libpq.
Maybe we could put a pg_nodiscard attribute on pg_b64_encode() and
pg_b64_decode()?
+1
[0] I think the most correct would be *or* not *and* word?
I think both are ok here.
Ok.
best regards,
Ranier Vilela
On 16.01.25 11:23, Ranier Vilela wrote:
Em qui., 16 de jan. de 2025 às 05:07, Peter Eisentraut
<peter@eisentraut.org <mailto:peter@eisentraut.org>> escreveu:On 16.01.25 02:12, Ranier Vilela wrote:
Per Coverity.
CID 1590024: (CHECKED_RETURN)
Calling "pg_b64_encode" without checking return value (as is done
elsewhere 8 out of 10 times).The function *pg_b64_encode* has in the comments:
[0] "and -1 in the event of an error"So, the function can fail.
All other calls check the return, In this case it could not bedifferent.
Fix by checking the return and reporting a message to the user,
in case of failure.Thanks, fixed. (I changed the ereports to elogs, which is how other
call sites do it.)Thank you.
I also fixed a related problem in the pg_b64_decode() calls in libpq.
Maybe we could put a pg_nodiscard attribute on pg_b64_encode() and
pg_b64_decode()?+1
done
Em sex., 17 de jan. de 2025 às 05:11, Peter Eisentraut <peter@eisentraut.org>
escreveu:
On 16.01.25 11:23, Ranier Vilela wrote:
Em qui., 16 de jan. de 2025 às 05:07, Peter Eisentraut
<peter@eisentraut.org <mailto:peter@eisentraut.org>> escreveu:On 16.01.25 02:12, Ranier Vilela wrote:
Per Coverity.
CID 1590024: (CHECKED_RETURN)
Calling "pg_b64_encode" without checking return value (as is done
elsewhere 8 out of 10 times).The function *pg_b64_encode* has in the comments:
[0] "and -1 in the event of an error"So, the function can fail.
All other calls check the return, In this case it could not bedifferent.
Fix by checking the return and reporting a message to the user,
in case of failure.Thanks, fixed. (I changed the ereports to elogs, which is how other
call sites do it.)Thank you.
I also fixed a related problem in the pg_b64_decode() calls in libpq.
Maybe we could put a pg_nodiscard attribute on pg_b64_encode() and
pg_b64_decode()?+1
done
Thanks Peter.
best regards,
Ranier Vilela