v17_review_suggestions.txt
text/plain
Filename: v17_review_suggestions.txt
Type: text/plain
Part: 0
commit 62b6bda852d09458ffcd5854b7810b3b316e2b1f
Author: Daniel Gustafsson <daniel@yesql.se>
Date: Wed Feb 28 18:26:03 2024 +0100
Suggested changes
diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c
index 765a18b9b2..f5cf271566 100644
--- a/src/backend/libpq/auth-oauth.c
+++ b/src/backend/libpq/auth-oauth.c
@@ -204,7 +204,7 @@ oauth_exchange(void *opaq, const char *input, int inputlen,
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("malformed OAUTHBEARER message"),
- errdetail("Comma expected, but found character %s.",
+ errdetail("Comma expected, but found character \"%s\".",
sanitize_char(*p))));
p++;
break;
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index 479310f598..2d1f30353a 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -42,6 +42,7 @@
#define appendStrValChar appendPQExpBufferChar
#define createStrVal createPQExpBuffer
#define resetStrVal resetPQExpBuffer
+#define destroyStrVal destroyPQExpBuffer
#else /* !FRONTEND */
@@ -53,6 +54,7 @@
#define appendStrValChar appendStringInfoChar
#define createStrVal makeStringInfo
#define resetStrVal resetStringInfo
+#define destroyStrVal destroyStringInfo
#endif
@@ -223,23 +225,11 @@ freeJsonLexContext(JsonLexContext *lex)
static const JsonLexContext empty = {0};
if (lex->flags & JSONLEX_FREE_STRVAL)
- {
-#ifdef FRONTEND
- destroyPQExpBuffer(lex->strval);
-#else
- pfree(lex->strval->data);
- pfree(lex->strval);
-#endif
- }
+ destroyStrVal(lex->strval);
+
if (lex->errormsg)
- {
-#ifdef FRONTEND
- destroyPQExpBuffer(lex->errormsg);
-#else
- pfree(lex->errormsg->data);
- pfree(lex->errormsg);
-#endif
- }
+ destroyStrVal(lex->errormsg);
+
if (lex->flags & JSONLEX_FREE_STRUCT)
pfree(lex);
else
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index c61d5c58f3..09419f6042 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -350,3 +350,10 @@ enlargeStringInfo(StringInfo str, int needed)
str->maxlen = newlen;
}
+
+void
+destroyStringInfo(StringInfo str)
+{
+ pfree(str->data);
+ pfree(str);
+}
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 2cd636b01c..64ec6419af 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -233,4 +233,6 @@ extern void appendBinaryStringInfoNT(StringInfo str,
*/
extern void enlargeStringInfo(StringInfo str, int needed);
+
+extern void destroyStringInfo(StringInfo str);
#endif /* STRINGINFO_H */
diff --git a/src/interfaces/libpq/fe-auth-oauth-curl.c b/src/interfaces/libpq/fe-auth-oauth-curl.c
index 3e20ba5818..7a27198d66 100644
--- a/src/interfaces/libpq/fe-auth-oauth-curl.c
+++ b/src/interfaces/libpq/fe-auth-oauth-curl.c
@@ -293,40 +293,39 @@ free_curl_async_ctx(PGconn *conn, void *ctx)
/*
* Macros for getting and setting state for the connection's two cURL handles,
- * so you don't have to write out the error handling every time. They assume
- * that they're embedded in a function returning bool, however.
+ * so you don't have to write out the error handling every time.
*/
-#define CHECK_MSETOPT(ACTX, OPT, VAL) \
+#define CHECK_MSETOPT(ACTX, OPT, VAL, FAILACTION) \
do { \
struct async_ctx *_actx = (ACTX); \
CURLMcode _setopterr = curl_multi_setopt(_actx->curlm, OPT, VAL); \
if (_setopterr) { \
actx_error(_actx, "failed to set %s on OAuth connection: %s",\
#OPT, curl_multi_strerror(_setopterr)); \
- return false; \
+ FAILACTION; \
} \
} while (0)
-#define CHECK_SETOPT(ACTX, OPT, VAL) \
+#define CHECK_SETOPT(ACTX, OPT, VAL, FAILACTION) \
do { \
struct async_ctx *_actx = (ACTX); \
CURLcode _setopterr = curl_easy_setopt(_actx->curl, OPT, VAL); \
if (_setopterr) { \
actx_error(_actx, "failed to set %s on OAuth connection: %s",\
#OPT, curl_easy_strerror(_setopterr)); \
- return false; \
+ FAILACTION; \
} \
} while (0)
-#define CHECK_GETINFO(ACTX, INFO, OUT) \
+#define CHECK_GETINFO(ACTX, INFO, OUT, FAILACTION) \
do { \
struct async_ctx *_actx = (ACTX); \
CURLcode _getinfoerr = curl_easy_getinfo(_actx->curl, INFO, OUT); \
if (_getinfoerr) { \
actx_error(_actx, "failed to get %s from OAuth response: %s",\
#INFO, curl_easy_strerror(_getinfoerr)); \
- return false; \
+ FAILACTION; \
} \
} while (0)
@@ -450,7 +449,7 @@ oauth_json_object_field_start(void *state, char *name, bool isnull)
while (field->name)
{
- if (!strcmp(name, field->name))
+ if (strcmp(name, field->name) == 0)
{
ctx->active = field;
break;
@@ -616,7 +615,7 @@ parse_oauth_json(struct async_ctx *actx, const struct json_field *fields)
bool success = false;
/* Make sure the server thinks it's given us JSON. */
- CHECK_GETINFO(actx, CURLINFO_CONTENT_TYPE, &content_type);
+ CHECK_GETINFO(actx, CURLINFO_CONTENT_TYPE, &content_type, return false);
if (!content_type)
{
@@ -1109,6 +1108,8 @@ register_timer(CURLM *curlm, long timeout, void *ctx)
static bool
setup_curl_handles(struct async_ctx *actx)
{
+ curl_version_info_data *curl_info;
+
/*
* Create our multi handle. This encapsulates the entire conversation with
* cURL for this connection.
@@ -1121,14 +1122,19 @@ setup_curl_handles(struct async_ctx *actx)
return false;
}
+ /*
+ * Extract information about the libcurl we are linked against.
+ */
+ curl_info = curl_version_info(CURLVERSION_NOW);
+
/*
* The multi handle tells us what to wait on using two callbacks. These
* will manipulate actx->mux as needed.
*/
- CHECK_MSETOPT(actx, CURLMOPT_SOCKETFUNCTION, register_socket);
- CHECK_MSETOPT(actx, CURLMOPT_SOCKETDATA, actx);
- CHECK_MSETOPT(actx, CURLMOPT_TIMERFUNCTION, register_timer);
- CHECK_MSETOPT(actx, CURLMOPT_TIMERDATA, actx);
+ CHECK_MSETOPT(actx, CURLMOPT_SOCKETFUNCTION, register_socket, return false);
+ CHECK_MSETOPT(actx, CURLMOPT_SOCKETDATA, actx, return false);
+ CHECK_MSETOPT(actx, CURLMOPT_TIMERFUNCTION, register_timer, return false);
+ CHECK_MSETOPT(actx, CURLMOPT_TIMERDATA, actx, return false);
/*
* Set up an easy handle. All of our requests are made serially, so we
@@ -1145,25 +1151,26 @@ setup_curl_handles(struct async_ctx *actx)
* Multi-threaded applications must set CURLOPT_NOSIGNAL. This requires us
* to handle the possibility of SIGPIPE ourselves.
*
- * TODO: This disables DNS resolution timeouts unless libcurl has been
- * compiled against alternative resolution support. We should check that.
- *
* TODO: handle SIGPIPE via pq_block_sigpipe(), or via a
* CURLOPT_SOCKOPTFUNCTION maybe...
*/
- CHECK_SETOPT(actx, CURLOPT_NOSIGNAL, 1L);
+ CHECK_SETOPT(actx, CURLOPT_NOSIGNAL, 1L, return false);
+ if (!curl_info->ares_num)
+ {
+ /* No alternative resolver, TODO: warn about timeouts */
+ }
/* TODO investigate using conn->Pfdebug and CURLOPT_DEBUGFUNCTION here */
- CHECK_SETOPT(actx, CURLOPT_VERBOSE, 1L);
- CHECK_SETOPT(actx, CURLOPT_ERRORBUFFER, actx->curl_err);
+ CHECK_SETOPT(actx, CURLOPT_VERBOSE, 1L, return false);
+ CHECK_SETOPT(actx, CURLOPT_ERRORBUFFER, actx->curl_err, return false);
/* TODO */
- CHECK_SETOPT(actx, CURLOPT_WRITEDATA, stderr);
+ CHECK_SETOPT(actx, CURLOPT_WRITEDATA, stderr, return false);
/*
* Only HTTP[S] is allowed. TODO: disallow HTTP without user opt-in
*/
- CHECK_SETOPT(actx, CURLOPT_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS);
+ CHECK_SETOPT(actx, CURLOPT_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS, return false);
/*
* Suppress the Accept header to make our request as minimal as possible.
@@ -1172,7 +1179,7 @@ setup_curl_handles(struct async_ctx *actx)
* what comes back anyway.)
*/
actx->headers = curl_slist_append(actx->headers, "Accept:"); /* TODO: check result */
- CHECK_SETOPT(actx, CURLOPT_HTTPHEADER, actx->headers);
+ CHECK_SETOPT(actx, CURLOPT_HTTPHEADER, actx->headers, return false);
return true;
}
@@ -1213,8 +1220,8 @@ start_request(struct async_ctx *actx)
int running;
resetPQExpBuffer(&actx->work_data);
- CHECK_SETOPT(actx, CURLOPT_WRITEFUNCTION, append_data);
- CHECK_SETOPT(actx, CURLOPT_WRITEDATA, &actx->work_data);
+ CHECK_SETOPT(actx, CURLOPT_WRITEFUNCTION, append_data, return false);
+ CHECK_SETOPT(actx, CURLOPT_WRITEDATA, &actx->work_data, return false);
err = curl_multi_add_handle(actx->curlm, actx->curl);
if (err)
@@ -1339,8 +1346,8 @@ drive_request(struct async_ctx *actx)
static bool
start_discovery(struct async_ctx *actx, const char *discovery_uri)
{
- CHECK_SETOPT(actx, CURLOPT_HTTPGET, 1L);
- CHECK_SETOPT(actx, CURLOPT_URL, discovery_uri);
+ CHECK_SETOPT(actx, CURLOPT_HTTPGET, 1L, return false);
+ CHECK_SETOPT(actx, CURLOPT_URL, discovery_uri, return false);
return start_request(actx);
}
@@ -1363,7 +1370,7 @@ finish_discovery(struct async_ctx *actx)
* validation into question), or non-authoritative responses, or any other
* complications.
*/
- CHECK_GETINFO(actx, CURLINFO_RESPONSE_CODE, &response_code);
+ CHECK_GETINFO(actx, CURLINFO_RESPONSE_CODE, &response_code, return false);
if (response_code != 200)
{
@@ -1387,20 +1394,17 @@ finish_discovery(struct async_ctx *actx)
* Per Section 3, the default is ["authorization_code", "implicit"].
*/
struct curl_slist *temp = actx->provider.grant_types_supported;
- bool oom = false;
temp = curl_slist_append(temp, "authorization_code");
- if (!temp)
- oom = true;
-
- temp = curl_slist_append(temp, "implicit");
- if (!temp)
- oom = true;
-
- if (oom)
+ if (temp)
{
- actx_error(actx, "out of memory");
- return false;
+ temp = curl_slist_append(temp, "implicit");
+ if (!temp)
+ {
+ curl_slist_free_all(temp);
+ actx_error(actx, "out of memory");
+ return false;
+ }
}
actx->provider.grant_types_supported = temp;
@@ -1491,8 +1495,8 @@ start_device_authz(struct async_ctx *actx, PGconn *conn)
/* TODO check for broken buffer */
/* Make our request. */
- CHECK_SETOPT(actx, CURLOPT_URL, device_authz_uri);
- CHECK_SETOPT(actx, CURLOPT_COPYPOSTFIELDS, work_buffer->data);
+ CHECK_SETOPT(actx, CURLOPT_URL, device_authz_uri, return false);
+ CHECK_SETOPT(actx, CURLOPT_COPYPOSTFIELDS, work_buffer->data, return false);
if (conn->oauth_client_secret)
{
@@ -1506,12 +1510,12 @@ start_device_authz(struct async_ctx *actx, PGconn *conn)
*
* TODO: should we omit client_id from the body in this case?
*/
- CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_BASIC);
- CHECK_SETOPT(actx, CURLOPT_USERNAME, conn->oauth_client_id);
- CHECK_SETOPT(actx, CURLOPT_PASSWORD, conn->oauth_client_secret);
+ CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_BASIC, return false);
+ CHECK_SETOPT(actx, CURLOPT_USERNAME, conn->oauth_client_id, return false);
+ CHECK_SETOPT(actx, CURLOPT_PASSWORD, conn->oauth_client_secret, return false);
}
else
- CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_NONE);
+ CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_NONE, return false);
return start_request(actx);
}
@@ -1521,7 +1525,7 @@ finish_device_authz(struct async_ctx *actx)
{
long response_code;
- CHECK_GETINFO(actx, CURLINFO_RESPONSE_CODE, &response_code);
+ CHECK_GETINFO(actx, CURLINFO_RESPONSE_CODE, &response_code, return false);
/*
* The device authorization endpoint uses the same error response as the
@@ -1593,8 +1597,8 @@ start_token_request(struct async_ctx *actx, PGconn *conn)
/* TODO check for broken buffer */
/* Make our request. */
- CHECK_SETOPT(actx, CURLOPT_URL, token_uri);
- CHECK_SETOPT(actx, CURLOPT_COPYPOSTFIELDS, work_buffer->data);
+ CHECK_SETOPT(actx, CURLOPT_URL, token_uri, return false);
+ CHECK_SETOPT(actx, CURLOPT_COPYPOSTFIELDS, work_buffer->data, return false);
if (conn->oauth_client_secret)
{
@@ -1608,16 +1612,16 @@ start_token_request(struct async_ctx *actx, PGconn *conn)
*
* TODO: should we omit client_id from the body in this case?
*/
- CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_BASIC);
- CHECK_SETOPT(actx, CURLOPT_USERNAME, conn->oauth_client_id);
- CHECK_SETOPT(actx, CURLOPT_PASSWORD, conn->oauth_client_secret);
+ CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_BASIC, return false);
+ CHECK_SETOPT(actx, CURLOPT_USERNAME, conn->oauth_client_id, return false);
+ CHECK_SETOPT(actx, CURLOPT_PASSWORD, conn->oauth_client_secret, return false);
}
else
- CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_NONE);
+ CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_NONE, return false);
resetPQExpBuffer(work_buffer);
- CHECK_SETOPT(actx, CURLOPT_WRITEFUNCTION, append_data);
- CHECK_SETOPT(actx, CURLOPT_WRITEDATA, work_buffer);
+ CHECK_SETOPT(actx, CURLOPT_WRITEFUNCTION, append_data, return false);
+ CHECK_SETOPT(actx, CURLOPT_WRITEDATA, work_buffer, return false);
return start_request(actx);
}
@@ -1627,7 +1631,7 @@ finish_token_request(struct async_ctx *actx, struct token *tok)
{
long response_code;
- CHECK_GETINFO(actx, CURLINFO_RESPONSE_CODE, &response_code);
+ CHECK_GETINFO(actx, CURLINFO_RESPONSE_CODE, &response_code, return false);
/*
* Per RFC 6749, Section 5, a successful response uses 200 OK. An error
@@ -1889,7 +1893,7 @@ pg_fe_run_oauth_flow(PGconn *conn, pgsocket *altsock)
* A slow_down error requires us to permanently increase our
* retry interval by five seconds. RFC 8628, Sec. 3.5.
*/
- if (!strcmp(err->error, "slow_down"))
+ if (strcmp(err->error, "slow_down") == 0)
{
actx->authz.interval += 5; /* TODO check for overflow? */
}
diff --git a/src/interfaces/libpq/fe-auth-oauth.c b/src/interfaces/libpq/fe-auth-oauth.c
index 2a35c7438c..66ee8ff076 100644
--- a/src/interfaces/libpq/fe-auth-oauth.c
+++ b/src/interfaces/libpq/fe-auth-oauth.c
@@ -49,7 +49,7 @@ oauth_init(PGconn *conn, const char *password,
* error.
*/
Assert(sasl_mechanism != NULL);
- Assert(!strcmp(sasl_mechanism, OAUTHBEARER_NAME));
+ Assert(strcmp(sasl_mechanism, OAUTHBEARER_NAME) == 0);
state = calloc(1, sizeof(*state));
if (!state)
@@ -156,17 +156,17 @@ oauth_json_object_field_start(void *state, char *name, bool isnull)
if (ctx->nested == 1)
{
- if (!strcmp(name, ERROR_STATUS_FIELD))
+ if (strcmp(name, ERROR_STATUS_FIELD) == 0)
{
ctx->target_field_name = ERROR_STATUS_FIELD;
ctx->target_field = &ctx->status;
}
- else if (!strcmp(name, ERROR_SCOPE_FIELD))
+ else if (strcmp(name, ERROR_SCOPE_FIELD) == 0)
{
ctx->target_field_name = ERROR_SCOPE_FIELD;
ctx->target_field = &ctx->scope;
}
- else if (!strcmp(name, ERROR_OPENID_CONFIGURATION_FIELD))
+ else if (strcmp(name, ERROR_OPENID_CONFIGURATION_FIELD) == 0)
{
ctx->target_field_name = ERROR_OPENID_CONFIGURATION_FIELD;
ctx->target_field = &ctx->discovery_uri;
@@ -243,7 +243,7 @@ handle_oauth_sasl_error(PGconn *conn, char *msg, int msglen)
if (strlen(msg) != msglen)
{
appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("server's error message contained an embedded NULL"));
+ libpq_gettext("server's error message contained an embedded NULL, and was discarded"));
return false;
}
@@ -316,7 +316,7 @@ handle_oauth_sasl_error(PGconn *conn, char *msg, int msglen)
return false;
}
- if (!strcmp(ctx.status, "invalid_token"))
+ if (strcmp(ctx.status, "invalid_token") == 0)
{
/*
* invalid_token is the only error code we'll automatically retry for,
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 9e084dd1c7..6e3538c9fd 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -525,15 +525,18 @@ pg_SASL_init(PGconn *conn, int payloadlen, bool *async)
conn->sasl = &pg_scram_mech;
conn->password_needed = true;
}
-#ifdef USE_OAUTH
else if (strcmp(mechanism_buf.data, OAUTHBEARER_NAME) == 0 &&
!selected_mechanism)
{
+#ifdef USE_OAUTH
selected_mechanism = OAUTHBEARER_NAME;
conn->sasl = &pg_oauth_mech;
conn->password_needed = false;
- }
+#else
+ libpq_append_conn_error(conn, "OAuth is required, but client does not support it");
+ goto error;
#endif
+ }
}
if (!selected_mechanism)