Re: Missing NULL check after calling ecpg_strdup

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