[PATCH] libpq improvements and fixes
Hi,
Coverity detected a dead code in the src / interfaces / libpq / fe-auth.c
file, to correct it, a simplification was made and the oom_error goto was
removed, since it is clearly redundant and its presence can be confusing.
The second part of the patch refers to the file src / interfaces / libpq /
fe-exec.c.
First, a correction was made to the return types of some functions that
clearly return bool, but are defined as int.
According to some functions, they do a basic check and if they fail, they
return immediately, so it does not make sense to start communication and
then return.
It makes more sense to do the basic checks, only to start communicating
with the server afterwards.
These changes are passing the regression tests and are in use in libpq.dll,
used in production by my customers.
regards,
Ranier Vilela
Attachments:
libpq.patchapplication/octet-stream; name=libpq.patchDownload
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 7e9afbbeee..bde5966f87 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -453,7 +453,11 @@ pg_SASL_init(PGconn *conn, int payloadlen)
goto error;
}
if (PQExpBufferDataBroken(mechanism_buf))
- goto oom_error;
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("out of memory\n"));
+ goto error;
+ }
/* An empty string indicates end of list */
if (mechanism_buf.data[0] == '\0')
@@ -561,7 +565,11 @@ pg_SASL_init(PGconn *conn, int payloadlen)
password,
selected_mechanism);
if (!conn->sasl_state)
- goto oom_error;
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("out of memory\n"));
+ goto error;
+ }
/* Get the mechanism-specific Initial Client Response, if any */
pg_fe_scram_exchange(conn->sasl_state,
@@ -601,14 +609,7 @@ error:
termPQExpBuffer(&mechanism_buf);
if (initialresponse)
free(initialresponse);
- return STATUS_ERROR;
-oom_error:
- termPQExpBuffer(&mechanism_buf);
- if (initialresponse)
- free(initialresponse);
- printfPQExpBuffer(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
return STATUS_ERROR;
}
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index eea0237c3a..ed8ff5b830 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -69,7 +69,7 @@ static bool PQexecStart(PGconn *conn);
static PGresult *PQexecFinish(PGconn *conn);
static int PQsendDescribe(PGconn *conn, char desc_type,
const char *desc_target);
-static int check_field_number(const PGresult *res, int field_num);
+static bool check_field_number(const PGresult *res, int field_num);
/* ----------------
@@ -225,7 +225,7 @@ PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status)
* function fails, it returns zero. If the function succeeds, it
* returns a non-zero value.
*/
-int
+bool
PQsetResultAttrs(PGresult *res, int numAttributes, PGresAttDesc *attDescs)
{
int i;
@@ -427,7 +427,7 @@ dupEvents(PGEvent *events, int count, size_t *memSize)
* Returns a non-zero value for success and zero for failure.
* (On failure, we report the specific problem via pqInternalNotice.)
*/
-int
+bool
PQsetvalue(PGresult *res, int tup_num, int field_num, char *value, int len)
{
PGresAttValue *attval;
@@ -694,27 +694,29 @@ void
PQclear(PGresult *res)
{
PGresult_data *block;
- int i;
if (!res)
return;
- for (i = 0; i < res->nEvents; i++)
+ if (res->events)
{
- /* only send DESTROY to successfully-initialized event procs */
- if (res->events[i].resultInitialized)
- {
- PGEventResultDestroy evt;
+ int i;
- evt.result = res;
- (void) res->events[i].proc(PGEVT_RESULTDESTROY, &evt,
- res->events[i].passThrough);
- }
- free(res->events[i].name);
- }
+ for (i = 0; i < res->nEvents; i++)
+ {
+ /* only send DESTROY to successfully-initialized event procs */
+ if (res->events[i].resultInitialized)
+ {
+ PGEventResultDestroy evt;
- if (res->events)
+ evt.result = res;
+ (void) res->events[i].proc(PGEVT_RESULTDESTROY, &evt,
+ res->events[i].passThrough);
+ }
+ free(res->events[i].name);
+ }
free(res->events);
+ }
/* Free all the subsidiary blocks */
while ((block = res->curBlock) != NULL)
@@ -1233,9 +1235,6 @@ fail:
int
PQsendQuery(PGconn *conn, const char *query)
{
- if (!PQsendQueryStart(conn))
- return 0;
-
/* check the argument */
if (!query)
{
@@ -1243,6 +1242,8 @@ PQsendQuery(PGconn *conn, const char *query)
libpq_gettext("command string is a null pointer\n"));
return 0;
}
+ if (!PQsendQueryStart(conn))
+ return 0;
/* construct the outgoing Query message */
if (pqPutMsgStart('Q', false, conn) < 0 ||
@@ -1291,9 +1292,6 @@ PQsendQueryParams(PGconn *conn,
const int *paramFormats,
int resultFormat)
{
- if (!PQsendQueryStart(conn))
- return 0;
-
/* check the arguments */
if (!command)
{
@@ -1307,6 +1305,8 @@ PQsendQueryParams(PGconn *conn,
libpq_gettext("number of parameters must be between 0 and 65535\n"));
return 0;
}
+ if (!PQsendQueryStart(conn))
+ return 0;
return PQsendQueryGuts(conn,
command,
@@ -1331,9 +1331,6 @@ PQsendPrepare(PGconn *conn,
const char *stmtName, const char *query,
int nParams, const Oid *paramTypes)
{
- if (!PQsendQueryStart(conn))
- return 0;
-
/* check the arguments */
if (!stmtName)
{
@@ -1362,6 +1359,9 @@ PQsendPrepare(PGconn *conn,
return 0;
}
+ if (!PQsendQueryStart(conn))
+ return 0;
+
/* construct the Parse message */
if (pqPutMsgStart('P', false, conn) < 0 ||
pqPuts(stmtName, conn) < 0 ||
@@ -1432,9 +1432,6 @@ PQsendQueryPrepared(PGconn *conn,
const int *paramFormats,
int resultFormat)
{
- if (!PQsendQueryStart(conn))
- return 0;
-
/* check the arguments */
if (!stmtName)
{
@@ -1449,6 +1446,9 @@ PQsendQueryPrepared(PGconn *conn,
return 0;
}
+ if (!PQsendQueryStart(conn))
+ return 0;
+
return PQsendQueryGuts(conn,
NULL, /* no command to parse */
stmtName,
@@ -2219,13 +2219,6 @@ PQsendDescribePortal(PGconn *conn, const char *portal)
static int
PQsendDescribe(PGconn *conn, char desc_type, const char *desc_target)
{
- /* Treat null desc_target as empty string */
- if (!desc_target)
- desc_target = "";
-
- if (!PQsendQueryStart(conn))
- return 0;
-
/* This isn't gonna work on a 2.0 server */
if (PG_PROTOCOL_MAJOR(conn->pversion) < 3)
{
@@ -2234,6 +2227,13 @@ PQsendDescribe(PGconn *conn, char desc_type, const char *desc_target)
return 0;
}
+ if (!PQsendQueryStart(conn))
+ return 0;
+
+ /* Treat null desc_target as empty string */
+ if (!desc_target)
+ desc_target = "";
+
/* construct the Describe message */
if (pqPutMsgStart('D', false, conn) < 0 ||
pqPutc(desc_type, conn) < 0 ||
@@ -2794,7 +2794,7 @@ PQbinaryTuples(const PGresult *res)
* Return true if OK, false if not
*/
-static int
+static bool
check_field_number(const PGresult *res, int field_num)
{
if (!res)
@@ -2809,7 +2809,7 @@ check_field_number(const PGresult *res, int field_num)
return true;
}
-static int
+static bool
check_tuple_field_number(const PGresult *res,
int tup_num, int field_num)
{
@@ -2832,7 +2832,7 @@ check_tuple_field_number(const PGresult *res,
return true;
}
-static int
+static bool
check_param_number(const PGresult *res, int param_num)
{
if (!res)
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index c9e6ac2b76..c7794a9a1f 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -524,10 +524,10 @@ extern void PQfreemem(void *ptr);
/* Create and manipulate PGresults */
extern PGresult *PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status);
extern PGresult *PQcopyResult(const PGresult *src, int flags);
-extern int PQsetResultAttrs(PGresult *res, int numAttributes, PGresAttDesc *attDescs);
+extern bool PQsetResultAttrs(PGresult *res, int numAttributes, PGresAttDesc *attDescs);
extern void *PQresultAlloc(PGresult *res, size_t nBytes);
extern size_t PQresultMemorySize(const PGresult *res);
-extern int PQsetvalue(PGresult *res, int tup_num, int field_num, char *value, int len);
+extern bool PQsetvalue(PGresult *res, int tup_num, int field_num, char *value, int len);
/* Quoting strings before inclusion in queries. */
extern size_t PQescapeStringConn(PGconn *conn,
Ranier Vilela <ranier.vf@gmail.com> writes:
Coverity detected a dead code in the src / interfaces / libpq / fe-auth.c
file, to correct it, a simplification was made and the oom_error goto was
removed, since it is clearly redundant and its presence can be confusing.
I'm kind of disinclined to let Coverity dictate our coding style here.
We've dismissed many hundreds of its reports as false positives, and
this seems like one that could get (probably already has gotten) the
same treatment. I also don't feel like duplicating error messages
as you propose is an improvement.
If we did want to adjust the code in pg_SASL_init, my thought would
be to reduce not increase the code duplication, by making the error
exits look like
...
return STATUS_OK;
oom_error:
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("out of memory\n"));
/* FALL THRU */
error:
termPQExpBuffer(&mechanism_buf);
if (initialresponse)
free(initialresponse);
return STATUS_ERROR;
}
It's only marginally worth the trouble though.
First, a correction was made to the return types of some functions that
clearly return bool, but are defined as int.
This is ancient history that doesn't seem worth revisiting. There is
certainly exactly zero chance of us changing libpq's external API
as you propose, because of the ensuing ABI breakage. Maybe we could
change the static functions, but I'm not very excited about it.
I can't get excited about the other code rearrangements you're proposing
here either. They seem to make the code more intellectually complex for
little benefit.
regards, tom lane
Em qua., 12 de fev. de 2020 às 22:25, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Ranier Vilela <ranier.vf@gmail.com> writes:
Coverity detected a dead code in the src / interfaces / libpq / fe-auth.c
file, to correct it, a simplification was made and the oom_error goto was
removed, since it is clearly redundant and its presence can be confusing.I'm kind of disinclined to let Coverity dictate our coding style here.
We've dismissed many hundreds of its reports as false positives, and
this seems like one that could get (probably already has gotten) the
same treatment. I also don't feel like duplicating error messages
as you propose is an improvement.
If you look closely at the code in the source, you will see that the style
there is:
if (test)
{
msg;
goto;
}
I just kept it, even if I duplicated the error message, the style was kept
and in my opinion it is much more coherent and readable.
But your solution is also good, and yes, it is worth it, because even with
small benefits, the change improves the code and prevents Coverity or
another tool from continuing to report false positives or not.
If we did want to adjust the code in pg_SASL_init, my thought would
be to reduce not increase the code duplication, by making the error
exits look like...
return STATUS_OK;oom_error:
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("out of memory\n"));
/* FALL THRU */error:
termPQExpBuffer(&mechanism_buf);
if (initialresponse)
free(initialresponse);
return STATUS_ERROR;
}It's only marginally worth the trouble though.
Sounds good to me.
First, a correction was made to the return types of some functions that
clearly return bool, but are defined as int.
This is ancient history that doesn't seem worth revisiting. There is
certainly exactly zero chance of us changing libpq's external API
as you propose, because of the ensuing ABI breakage. Maybe we could
change the static functions, but I'm not very excited about it.
Virtually no code will break for the change, since bool and int are
internally the same types.
I believe that no code will have either adjusted to work with corrected
functions, even if they use compiled libraries.
And again, it is worth correcting at least the static ones, because the
goal here, too, is to improve readability.
I can't get excited about the other code rearrangements you're proposing
here either. They seem to make the code more intellectually complex for
little benefit.
I cannot agree with you that these changes add complexity.
It was using the principle enshrined in programming that I proposed these
changes.
"Get out quick."
For 99% of calls to these functions, there won't be any changes, since all
parameters are ok, tests will be done and the PQsendQueryStart function
will be called anyway.
If it were possible, it would be better to eliminate the basic tests, but
this is not possible, so better to do them first and get out of there soon,
without doing anything else.
regards,
Ranier Villela
On Thu, Feb 13, 2020 at 02:22:36PM -0300, Ranier Vilela wrote:
I just kept it, even if I duplicated the error message, the style was kept
and in my opinion it is much more coherent and readable.
But your solution is also good, and yes, it is worth it, because even with
small benefits, the change improves the code and prevents Coverity or
another tool from continuing to report false positives or not.
Complaints from static analyzers need to be taken with a pinch of
salt, and I agree with Tom here.
Virtually no code will break for the change, since bool and int are
internally the same types.
I believe that no code will have either adjusted to work with corrected
functions, even if they use compiled libraries.
And again, it is worth correcting at least the static ones, because the
goal here, too, is to improve readability.
FWIW, looking at the patch from upthread, I think that it is not that
wise to blindly break the error compatibility handling of all PQsend*
routines by switching the error handling of the connection to be after
the compatibility checks, and all the other changes don't justify a
breakage making back-patching more complicated nor do they improve
readability at great lengths.
--
Michael
Em sex., 14 de fev. de 2020 às 03:13, Michael Paquier <michael@paquier.xyz>
escreveu:
On Thu, Feb 13, 2020 at 02:22:36PM -0300, Ranier Vilela wrote:
I just kept it, even if I duplicated the error message, the style was
kept
and in my opinion it is much more coherent and readable.
But your solution is also good, and yes, it is worth it, because evenwith
small benefits, the change improves the code and prevents Coverity or
another tool from continuing to report false positives or not.Complaints from static analyzers need to be taken with a pinch of
salt, and I agree with Tom here.
That's right, I will try avoid sending patches that only satisfy static
analysis tools.
Virtually no code will break for the change, since bool and int are
internally the same types.
I believe that no code will have either adjusted to work with corrected
functions, even if they use compiled libraries.
And again, it is worth correcting at least the static ones, because the
goal here, too, is to improve readability.FWIW, looking at the patch from upthread, I think that it is not that
wise to blindly break the error compatibility handling of all PQsend*
routines by switching the error handling of the connection to be after
the compatibility checks, and all the other changes don't justify a
breakage making back-patching more complicated nor do they improve
readability at great lengths.
It is difficult to understand what you consider to be improvement.
Another programming principle I follow is to remove anything static from
loops that can be executed outside the loop.
In this specific case, from the loop modified in fe-exec, two branches were
removed, is this an improvement for you or not?
See patch attached.
regards,
Ranier Vilela
Attachments:
fe-exec.patchapplication/octet-stream; name=fe-exec.patchDownload
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index eea0237c3a..075e2d0205 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -1524,6 +1524,13 @@ PQsendQueryGuts(PGconn *conn,
libpq_gettext("function requires at least protocol version 3.0\n"));
return 0;
}
+ /* Don't bother server if parameters are wrong */
+ if (paramFormats && !paramLengths)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("length must be given for binary parameter\n"));
+ return 0;
+ }
/*
* We will send Parse (if needed), Bind, Describe Portal, Execute, Sync,
@@ -1583,38 +1590,43 @@ PQsendQueryGuts(PGconn *conn,
goto sendFailed;
/* Send parameters */
- for (i = 0; i < nParams; i++)
- {
- if (paramValues && paramValues[i])
- {
- int nbytes;
-
- if (paramFormats && paramFormats[i] != 0)
- {
- /* binary parameter */
- if (paramLengths)
- nbytes = paramLengths[i];
- else
- {
- printfPQExpBuffer(&conn->errorMessage,
- libpq_gettext("length must be given for binary parameter\n"));
- goto sendFailed;
- }
- }
- else
- {
- /* text parameter, do not use paramLengths */
- nbytes = strlen(paramValues[i]);
- }
- if (pqPutInt(nbytes, 4, conn) < 0 ||
- pqPutnchar(paramValues[i], nbytes, conn) < 0)
- goto sendFailed;
+ if (paramValues)
+ {
+ int nbytes;
+
+ for (i = 0; i < nParams; i++)
+ {
+ if (paramValues[i])
+ {
+ if (paramFormats && paramFormats[i] != 0)
+ {
+ /* binary parameter */
+ nbytes = paramLengths[i];
+ }
+ else
+ {
+ /* text parameter, do not use paramLengths */
+ nbytes = strlen(paramValues[i]);
+ }
+ if (pqPutInt(nbytes, 4, conn) < 0 ||
+ pqPutnchar(paramValues[i], nbytes, conn) < 0)
+ goto sendFailed;
+ }
+ else
+ {
+ /* take the param as NULL */
+ if (pqPutInt(-1, 4, conn) < 0)
+ goto sendFailed;
+ }
}
- else
- {
- /* take the param as NULL */
- if (pqPutInt(-1, 4, conn) < 0)
- goto sendFailed;
+ }
+ else
+ {
+ /* take the params as NULL */
+ for (i = 0; i < nParams; i++)
+ {
+ if (pqPutInt(-1, 4, conn) < 0)
+ goto sendFailed;
}
}
if (pqPutInt(1, 2, conn) < 0 ||