Missing NULL check after calling ecpg_strdup

Hi Evgeniy,

In case of out_of_memory, the ecpg_strdup function may return NULL.
Checks should be added in src/interfaces/ecpg/ecpglib/execute.c.
Patch attached.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

The patch looks correct, but I believe it's incomplete. It misses
several other places where ecpg_strdup() is called without proper
checks. A correct patch would look like the one attached.

While working on it I noticed a potentially problematic strcmp call,
marked with XXX in the patch. I didn't address this issue in v2.

Thoughts?

On Fri, Jul 11, 2025 at 07:22:36PM +0300, Aleksander Alekseev wrote:

The patch looks correct, but I believe it's incomplete. It misses
several other places where ecpg_strdup() is called without proper
checks. A correct patch would look like the one attached.

While working on it I noticed a potentially problematic strcmp call,
marked with XXX in the patch. I didn't address this issue in v2.

Thoughts?

The semantics that I'm finding really annoying is the fact that
ecpg_strdup() is OK to assume that a NULL input is valid to handle, so
there is no way to make the difference between what should be an
actual error and what should be valid, leading to more confusion
because "realname" can be NULL.

Should we actually check sqlca_t more seriously if failing one of the
strdup calls used for the port, host, etc. when attempting the
connection? The ecpg_log() assumes that a NULL value equals a
<DEFAULT>, which would be wrong if we failed one of these allocations
on OOM.
--
Michael

Hi Michael,

Should we actually check sqlca_t more seriously if failing one of the
strdup calls used for the port, host, etc. when attempting the
connection? The ecpg_log() assumes that a NULL value equals a
<DEFAULT>, which would be wrong if we failed one of these allocations
on OOM.

If I read this correctly, you propose to check if strdup returns an
error and if it does then somehow run extra checks on sqlca_t? Sorry,
I don't follow. Could you please elaborate what you are proposing?

Hi,

While working on it I noticed a potentially problematic strcmp call,
marked with XXX in the patch. I didn't address this issue in v2.

Here is the corrected patch v3. Changes since v2:

```
                for (con = all_connections; con != NULL; con = con->next)
                {
-                       /* XXX strcmp() will segfault if con->name is NULL */
-                       if (strcmp(connection_name, con->name) == 0)
+                       /* Check for NULL to prevent segfault */
+                       if (con->name != NULL &&
strcmp(connection_name, con->name) == 0)
                                break;
                }
                ret = con;
```

I was tired or something and didn't think of this trivial fix.

As a side note it looks like ecpg could use some refactoring, but this
is subject for another patch IMO.

Hi,

Here is the corrected patch v3. Changes since v2:

```
for (con = all_connections; con != NULL; con = con->next)
{
-                       /* XXX strcmp() will segfault if con->name is NULL */
-                       if (strcmp(connection_name, con->name) == 0)
+                       /* Check for NULL to prevent segfault */
+                       if (con->name != NULL &&
strcmp(connection_name, con->name) == 0)
break;
}
ret = con;
```

I was tired or something and didn't think of this trivial fix.

As a side note it looks like ecpg could use some refactoring, but this
is subject for another patch IMO.

Forgot the attachment. Sorry for the noise.

On 2025-Jul-14, Aleksander Alekseev wrote:

@@ -460,7 +461,21 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*/
conn_keywords = (const char **) ecpg_alloc((connect_params + 1) * sizeof(char *), lineno);
conn_values = (const char **) ecpg_alloc(connect_params * sizeof(char *), lineno);
-	if (conn_keywords == NULL || conn_values == NULL)
+
+	/* Allocate memory for connection name */
+	if (connection_name != NULL)
+		this->name = ecpg_strdup(connection_name, lineno);
+	else
+		this->name = ecpg_strdup(realname, lineno);
+
+	/*
+	 * Note that NULL is a correct value for realname and ecpg_strdup(NULL,
+	 * ...) just returns NULL. For named reasons the ckecks for this->name are
+	 * a bit complicated here.
+	 */
+	if (conn_keywords == NULL || conn_values == NULL ||
+		(connection_name != NULL && this->name == NULL) ||	/* first call failed */
+		(connection_name == NULL && realname != NULL && this->name == NULL))	/* second call failed */
{
if (host)
ecpg_free(host);

This looks super baroque. Why not simplify a bit instead? Maybe
something like

@@ -267,6 +268,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*options = NULL;
const char **conn_keywords;
const char **conn_values;
+ bool strdup_failed;

 	if (sqlca == NULL)
 	{
@@ -460,7 +462,21 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
 	 */
 	conn_keywords = (const char **) ecpg_alloc((connect_params + 1) * sizeof(char *), lineno);
 	conn_values = (const char **) ecpg_alloc(connect_params * sizeof(char *), lineno);
-	if (conn_keywords == NULL || conn_values == NULL)
+
+	/* Decide on a connection name */
+	strdup_failed = false;
+	if (connection_name != NULL || realname != NULL)
+	{
+		this->name = ecpg_strdup(connection_name ? connection_name : realname,
+								 lineno);
+		if (this->name == NULL)
+			strdup_failed = true;
+	}
+	else
+		this->name = NULL;
+
+	/* Deal with any failed allocations above */
+	if (conn_keywords == NULL || conn_values == NULL || strdup_failed)
 	{
 		if (host)
 			ecpg_free(host);

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

Hi Alvaro,

This looks super baroque. Why not simplify a bit instead? Maybe
something like

[...]

Fair point. Here is the corrected patch.

On Mon, Jul 14, 2025 at 04:03:42PM +0300, Aleksander Alekseev wrote:

Hi Michael,

Should we actually check sqlca_t more seriously if failing one of the
strdup calls used for the port, host, etc. when attempting the
connection? The ecpg_log() assumes that a NULL value equals a
<DEFAULT>, which would be wrong if we failed one of these allocations
on OOM.

If I read this correctly, you propose to check if strdup returns an
error and if it does then somehow run extra checks on sqlca_t? Sorry,
I don't follow. Could you please elaborate what you are proposing?

What I mean is that we need to be smarter with the error handling done
by the result returned by ecpg_strdup(), and that while your patch is
an improvement, we have much more problems that we should address.

For example, even if I look at your v4 posted at [1]/messages/by-id/CAJ7c6TPn+eO4cVH7urFr+G8d5TmMm0svsVQXjhY_C2LuBm8a7g@mail.gmail.com -- Michael, I am still
seeing such code block for the various fields when filling in the data
of a connection:
char *dbname = name ? ecpg_strdup(name, lineno) : NULL,
[...]
if (tmp[1]/messages/by-id/CAJ7c6TPn+eO4cVH7urFr+G8d5TmMm0svsVQXjhY_C2LuBm8a7g@mail.gmail.com -- Michael != '\0') /* non-empty database name */
{
realname = ecpg_strdup(tmp + 1, lineno);
connect_params++;
}
*tmp = '\0';
[...]
if (tmp != NULL) /* port number given */
{
*tmp = '\0';
port = ecpg_strdup(tmp + 1, lineno);
connect_params++;
}
[...]
etc.

So it happens that if there is a value to allocate and that we fail to
allocate it it, we log an OOM error with ecpg_raise(), but we don't
return immediately from ECPGconnect().

At the end the current coding means, if I am reading that right, that
we could create a connection data structure where we
think there are default values because these are NULL, but the caller
may have included a completely different value (note: I've also
confirmed that by forcing an error, and we happily try to open a
connection). This means this stuff could connect to a completely
unrelated server if some of the ecpg_strdup() calls fail on OOM, while
the next ones work. connect_params is used to count the number of
connection parameters, but it's not really used as in sanity checks
depending on what's set in a URI. I think that we need to redesign a
bit ecpg_strdup(), perhaps by providing an extra input argument so as
we can detect hard failures on OOM and let ECPGconnect() return early
if we find a problem. We should also force callers to take decisions
if they always have a non-NULL input, which is what we expect from
most of the fields extracted from the URI with strrchr().

[1]: /messages/by-id/CAJ7c6TPn+eO4cVH7urFr+G8d5TmMm0svsVQXjhY_C2LuBm8a7g@mail.gmail.com -- Michael
--
Michael

Hi Michael,

depending on what's set in a URI. I think that we need to redesign a
bit ecpg_strdup(), perhaps by providing an extra input argument so as
we can detect hard failures on OOM and let ECPGconnect() return early
if we find a problem.

Makes sense. In this case however I believe we should refactor the
rest of the functions in src/interfaces/ecpg/ecpglib/memory.c for
consistency. E.g. we will remove `lineno` argument and pass `bool*
alloc_failed` and let the callers make decisions.

Since this is going to be a fairly large refactoring I would like to
propose it as a separate patch after we agree on this one, if it works
for you. I believe change like this deserves a separate thread for
better visibility and also separate discussion.

We should also force callers to take decisions
if they always have a non-NULL input, which is what we expect from
most of the fields extracted from the URI with strrchr().

Not 100% sure if I fully understand this part. Are you proposing to move:

```
if (string == NULL)
return NULL;
```

... check from ecpg_strdup() on the calling side, or something else?

On Wed, Jul 16, 2025 at 02:04:12PM +0300, Aleksander Alekseev wrote:

Hi Michael,

depending on what's set in a URI. I think that we need to redesign a
bit ecpg_strdup(), perhaps by providing an extra input argument so as
we can detect hard failures on OOM and let ECPGconnect() return early
if we find a problem.

Makes sense. In this case however I believe we should refactor the
rest of the functions in src/interfaces/ecpg/ecpglib/memory.c for
consistency. E.g. we will remove `lineno` argument and pass `bool*
alloc_failed` and let the callers make decisions.

Or you could keep the "lineno" and the error generated in memory.c as
this gives enough details about the location where the problem
happens. We are going to need the extra "alloc_failed".

Since this is going to be a fairly large refactoring I would like to
propose it as a separate patch after we agree on this one, if it works
for you. I believe change like this deserves a separate thread for
better visibility and also separate discussion.

Hmm. But both are related and the same problem, no? This is touching
the same code paths..

Not 100% sure if I fully understand this part. Are you proposing to move:

if (string == NULL)
return NULL;

... check from ecpg_strdup() on the calling side, or something else?

Nope, we could keep this shortcut in ecpg_strdup() as long as there is
an extra field to track if an error has happened.
--
Michael

Hi Michael,

Thanks for all your great feedback.

Or you could keep the "lineno" and the error generated in memory.c as
this gives enough details about the location where the problem
happens. We are going to need the extra "alloc_failed".

[...]

Hmm. But both are related and the same problem, no? This is touching
the same code paths..

[...]

Nope, we could keep this shortcut in ecpg_strdup() as long as there is
an extra field to track if an error has happened.

OK, patch 0002 implements this idea with minimal changes to the existing logic.

Hi,

OK, patch 0002 implements this idea with minimal changes to the existing logic.

Here is a slightly modified version:

``
--- a/src/interfaces/ecpg/ecpglib/prepare.c
+++ b/src/interfaces/ecpg/ecpglib/prepare.c
@@ -570,7 +570,7 @@ AddStmtToCache(int lineno,          /* line # of
statement */
        entry = &stmtCacheEntries[entNo];
        entry->lineno = lineno;
        entry->ecpgQuery = ecpg_strdup(ecpgQuery, lineno, &alloc_failed);
-       if (!entry->ecpgQuery)
+       if (alloc_failed)
                return -1;
        entry->connection = connection;
        entry->execs = 0;
```

We know that ecpgQuery can't be NULL because we hash its value above.
Thus ecpg_strdup can fail only if strdup() fails.

On Mon, Jul 21, 2025 at 11:48:27AM +0300, Aleksander Alekseev wrote:

+            /* Check for NULL to prevent segfault */
+            if (con->name != NULL && strcmp(connection_name, con->name) == 0)
break;

I have spent some time rechecking the whole code, and I have
backpatched this part. One pattern where I've easily been able to
trigger the problem is by specifying "\0" as the database name. The
preprocessor rejects empty strings, but it fails to reject the case of
a CONNECT TO "\0". Perhaps it should actually, found that funny..
Anyway, it does not change the fact that if we do a named connection
lookup in a stack where at least one NULL connection is set, we would
crash during the lookup.

I have also hacked on the rest, spotted a couple of bugs and
inconsistencies.

-        stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
+        stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno, &alloc_failed);
+        if (alloc_failed)
+            return false;

Hmm.. Aren't you missing a va_end(args) in the early exit you are
adding here?

In ecpg_store_input() and ecpg_do_prologue(), we can do without any
alloc_failed. All the inputs can never be NULL.

I think that the order of the operations in ecpg_auto_prepare() is
incorrect: we would add statements to the cache even if the strdup()
fails. We have only one caller of ecpg_auto_prepare(), where the
*name we set does not matter.

At the end, I finish with the attached, where alloc_failed matters for
the failure checks with repeated calls of strdup() in ECPGconnect()
and also the setlocale() case. This is for HEAD due to how unlikely
these issues would occur in practice.
--
Michael

Hi,

I have spent some time rechecking the whole code, and I have
backpatched this part. [...]

Many thanks!

Hmm.. Aren't you missing a va_end(args) in the early exit you are
adding here?

I do, and it's rather stupid of me. Thanks.

[...]
At the end, I finish with the attached, where alloc_failed matters for
the failure checks with repeated calls of strdup() in ECPGconnect()
and also the setlocale() case. This is for HEAD due to how unlikely
these issues would occur in practice.

v7 may have a compilation warning on Linux:

```
warning: unused variable ‘alloc_failed’ [-Wunused-variable]
```

... because the only use of the variable is hidden under #ifdef's.

Fixed in v8:

```
--- a/src/interfaces/ecpg/ecpglib/descriptor.c
+++ b/src/interfaces/ecpg/ecpglib/descriptor.c
@@ -240,9 +240,9 @@ ECPGget_desc(int lineno, const char *desc_name,
int index,...)
                                act_tuple;
        struct variable data_var;
        struct sqlca_t *sqlca = ECPGget_sqlca();
-       bool            alloc_failed = false;
+       bool            alloc_failed = (sqlca == NULL);

- if (sqlca == NULL)
+ if (alloc_failed)
{
ecpg_raise(lineno, ECPG_OUT_OF_MEMORY,
ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
```

This code is also more consistent with what we ended up having in connect.c.

Other than that the patch looks OK to me.

On Tue, Jul 22, 2025 at 04:20:53PM +0300, Aleksander Alekseev wrote:

v7 may have a compilation warning on Linux:

```
warning: unused variable ‘alloc_failed’ [-Wunused-variable]
```

... because the only use of the variable is hidden under #ifdef's.

Yep, thanks, didn't see this one coming. Doing a CI run was still on
my list of things still to do. I have just done one after a second
lookup and I think that we should be OK.

What you are suggesting for ECPGget_desc() is fine by me at the end.
Both are immediate failure checks and the first one is never expected
to be NULL, but the second with setlocale() needs that, so...

Applied v8 on HEAD.
--
Michael