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], 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] != '\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]: https://www.postgresql.org/message-id/CAJ7c6TPn+eO4cVH7urFr+G8d5TmMm0svsVQXjhY_C2LuBm8a7g@mail.gmail.com
--
Michael
