This routines includes three code paths that can fail, with the
allocated prepared statement name going out of scope.
Per report from Coverity. Oversight in commit a6eabec680, that has
played with the order of some ecpg_strdup() calls in this code path.
Various code paths of the ECPG code did not check for memory allocation
failures, including the specific case where ecpg_strdup() considers a
NULL value given in input as a valid behavior. strdup() returning
itself NULL on failure, there was no way to make the difference between
what could be valid and what should fail.
With the different cases in mind, ecpg_strdup() is redesigned and gains
a new optional argument, giving its callers the possibility to
differentiate allocation failures and valid cases where the caller is
giving a NULL value in input. Most of the ECPG code does not expect a
NULL value, at the exception of ECPGget_desc() (setlocale) and
ECPGconnect(), like dbname being unspecified, with repeated strdup
calls.
The code is adapted to work with this new routine. Note the case of
ecpg_auto_prepare(), where the code order is switched so as we handle
failures with ecpg_strdup() before manipulating any cached data,
avoiding inconsistencies.
This class of failure is unlikely a problem in practice, so no backpatch
is done. Random OOM failures in ECPGconnect() could cause the driver to
connect to a different server than the one wanted by the caller, because
it could fallback to default values instead of the parameters defined
depending on the combinations of allocation failures and successes.
Author: Evgeniy Gorbanev <gorbanyoves@basealt.ru>
Co-authored-by: Aleksander Alekseev <aleksander@tigerdata.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/a6b193c1-6994-4d9c-9059-aca4aaf41ddd@basealt.ru
ECPGconnect() caches established connections to the server, supporting
the case of a NULL connection name when a database name is not specified
by its caller.
A follow-up call to ECPGget_PGconn() to get an established connection
from the cached set with a non-NULL name could cause a NULL pointer
dereference if a NULL connection was listed in the cache and checked for
a match. At least two connections are necessary to reproduce the issue:
one with a NULL name and one with a non-NULL name.
Author: Aleksander Alekseev <aleksander@tigerdata.com>
Discussion: https://postgr.es/m/CAJ7c6TNvFTPUTZQuNAoqgzaSGz-iM4XR61D7vEj5PsQXwg2RyA@mail.gmail.com
Backpatch-through: 13
Remove setlocale() and _configthreadlocal() as fallback strategy on
systems that don't have uselocale(), where ECPG tries to control
LC_NUMERIC formatting on input and output of floating point numbers. It
was probably broken on some systems (NetBSD), and the code was also
quite messy and complicated, with obsolete configure tests (Windows).
It was also arguably broken, or at least had unstated environmental
requirements, if pgtypeslib code was called directly.
Instead, introduce PG_C_LOCALE to refer to the "C" locale as a locale_t
value. It maps to the special constant LC_C_LOCALE when defined by libc
(macOS, NetBSD), or otherwise uses a process-lifetime locale_t that is
allocated on first use, just as ECPG previously did itself. The new
replacement might be more widely useful. Then change the float parsing
and printing code to pass that to _l() functions where appropriate.
Unfortunately the portability of those functions is a bit complicated.
First, many obvious and useful _l() functions are missing from POSIX,
though most standard libraries define some of them anyway. Second,
although the thread-safe save/restore technique can be used to replace
the missing ones, Windows and NetBSD refused to implement standard
uselocale(). They might have a point: "wide scope" uselocale() is hard
to combine with other code and error-prone, especially in library code.
Luckily they have the _l() functions we want so far anyway. So we have
to be prepared for both ways of doing things:
1. In ECPG, use strtod_l() for parsing, and supply a port.h replacement
using uselocale() over a limited scope if missing.
2. Inside our own snprintf.c, use three different approaches to format
floats. For frontend code, call libc's snprintf_l(), or wrap libc's
snprintf() in uselocale() if it's missing. For backend code, snprintf.c
can keep assuming that the global locale's LC_NUMERIC is "C" and call
libc's snprintf() without change, for now.
(It might eventually be possible to call our in-tree Ryū routines to
display floats in snprintf.c, given the C-locale-always remit of our
in-tree snprintf(), but this patch doesn't risk changing anything that
complicated.)
Author: Thomas Munro <thomas.munro@gmail.com>
Reviewed-by: Peter Eisentraut <peter@eisentraut.org>
Reviewed-by: Tristan Partin <tristan@partin.io>
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://postgr.es/m/CWZBBRR6YA8D.8EHMDRGLCKCD%40neon.tech
We want to support a "noreturn" decoration on more compilers besides
just GCC-compatible ones, but for that we need to move the decoration
in front of the function declaration instead of either behind it or
wherever, which is the current style afforded by GCC-style attributes.
Also rename the macro to "pg_noreturn" to be similar to the C11
standard "noreturn".
pg_noreturn is now supported on all compilers that support C11 (using
_Noreturn), as well as GCC-compatible ones (using __attribute__, as
before), as well as MSVC (using __declspec). (When PostgreSQL
requires C11, the latter two variants can be dropped.)
Now, all supported compilers effectively support pg_noreturn, so the
extra code for !HAVE_PG_ATTRIBUTE_NORETURN can be dropped.
This also fixes a possible problem if third-party code includes
stdnoreturn.h, because then the current definition of
#define pg_attribute_noreturn() __attribute__((noreturn))
would cause an error.
Note that the C standard does not support a noreturn attribute on
function pointer types. So we have to drop these here. There are
only two instances at this time, so it's not a big loss. In one case,
we can make up for it by adding the pg_noreturn to a wrapper function
and adding a pg_unreachable(), in the other case, the latter was
already done before.
Reviewed-by: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://www.postgresql.org/message-id/flat/pxr5b3z7jmkpenssra5zroxi7qzzp6eswuggokw64axmdixpnk@zbwxuq7gbbcw
Previously, Meson could produce a warning about the use of 'deps' in ecpg:
WARNING: Project targets '>=0.54' but uses a feature introduced in '0.60.0': list.<plus>. The right-hand operand was not a list.
The right-hand operand of 'deps' should be a list. This commit fixes
the warning by wrapping it with square brackets.
This issue was introduced in commit 28f04984f0.
Author: Jacob Champion <jacob.champion@enterprisedb.com>
Discussion: https://postgr.es/m/CAOYmi+ks8wO06Ymxduw2h_eQJ_D4_jHGeyMK0P=p5Q3psnEdMA@mail.gmail.com
Since commit 757fb0e5a9, these
Informix-compat functions return 0 without changing the output
parameter. Initialize the output parameter before the test call, making
that obvious. Before this, the expected test output has been depending
on freed stack memory. "gcc -ftrivial-auto-var-init=pattern" revealed
that. Back-patch to v13 (all supported versions).
Discussion: https://postgr.es/m/20250106192748.cf.nmisch@google.com
This allows the RETURNING list of INSERT/UPDATE/DELETE/MERGE queries
to explicitly return old and new values by using the special aliases
"old" and "new", which are automatically added to the query (if not
already defined) while parsing its RETURNING list, allowing things
like:
RETURNING old.colname, new.colname, ...
RETURNING old.*, new.*
Additionally, a new syntax is supported, allowing the names "old" and
"new" to be changed to user-supplied alias names, e.g.:
RETURNING WITH (OLD AS o, NEW AS n) o.colname, n.colname, ...
This is useful when the names "old" and "new" are already defined,
such as inside trigger functions, allowing backwards compatibility to
be maintained -- the interpretation of any existing queries that
happen to already refer to relations called "old" or "new", or use
those as aliases for other relations, is not changed.
For an INSERT, old values will generally be NULL, and for a DELETE,
new values will generally be NULL, but that may change for an INSERT
with an ON CONFLICT ... DO UPDATE clause, or if a query rewrite rule
changes the command type. Therefore, we put no restrictions on the use
of old and new in any DML queries.
Dean Rasheed, reviewed by Jian He and Jeff Davis.
Discussion: https://postgr.es/m/CAEZATCWx0J0-v=Qjc6gXzR=KtsdvAE7Ow=D=mu50AgOe+pvisQ@mail.gmail.com
The ecpg command includes code to warn about unsupported COPY FROM STDIN
statements in input files. However, since commit 3d009e45bd,
this functionality has been broken due to a bug introduced in that commit,
causing ecpg to fail to detect the statement.
This commit resolves the issue, restoring ecpg's ability to detect
COPY FROM STDIN and issue a warning as intended.
Back-patch to all supported versions.
Author: Ryo Kanbayashi
Reviewed-by: Hayato Kuroda, Tom Lane
Discussion: https://postgr.es/m/CANOn0Ez_t5uDCUEV8c1YORMisJiU5wu681eEVZzgKwOeiKhkqQ@mail.gmail.com
Trying to clean up the code a bit while we're working on these files
for the reentrant scanner/pure parser patches. This cleanup only
touches the code sections after the second '%%' in each file, via a
manually-supervised and locally hacked up pgindent.
Redefine our exact width types with standard C99 types and macros,
including int64_t, INT64_MAX, INT64_C(), PRId64 etc. We were already
using <stdint.h> types in a few places.
One complication is that Windows' <inttypes.h> uses format strings like
"%I64d", "%I32", "%I" for PRI*64, PRI*32, PTR*PTR, instead of mapping to
other standardized format strings like "%lld" etc as seen on other known
systems. Teach our snprintf.c to understand them.
This removes a lot of configure clutter, and should also allow 64-bit
numbers and other standard types to be used in localized messages
without casting.
Reviewed-by: Peter Eisentraut <peter@eisentraut.org>
Discussion: https://postgr.es/m/ME3P282MB3166F9D1F71F787929C0C7E7B6312%40ME3P282MB3166.AUSP282.PROD.OUTLOOK.COM
The loops over cursor argument variables neglected to ever advance
"prevvar". The code would accidentally do the right thing anyway
when removing the first or second list entry, but if it had to
remove the third or later entry then it would also remove all
entries between there and the first entry. AFAICS this would
only matter for cursors that reference out-of-scope variables,
which is a weird Informix compatibility hack; between that and
the lack of impact for short lists, it's not so surprising that
nobody has complained. Nonetheless it's a pretty obvious bug.
It would have been more obvious if these loops used a more standard
coding style for chasing the linked lists --- this business with the
"prev" pointer sometimes pointing at the current list entry is
confusing and overcomplicated. So rather than just add a minimal
band-aid, I chose to rewrite the loops in the same style we use
elsewhere, where the "prev" pointer is NULL until we are dealing with
a non-first entry and we save the "next" pointer at the top of the
loop. (Two of the four loops touched here are not actually buggy,
but it seems better to make them all look alike.)
Coverity discovered this problem, but not until 2b41de4a5 added code
to free no-longer-needed arguments structs. With that, the incorrect
link updates are possibly touching freed memory, and it complained
about that. Nonetheless the list corruption hazard is ancient, so
back-patch to all supported branches.
The C standard says that sizeof(bool) is implementation-defined, but we
know of no current systems where it is not 1. The last known systems
seem to have been Apple macOS/PowerPC 10.5 and Microsoft Visual C++ 4,
both long defunct.
PostgreSQL has always required sizeof(bool) == 1 for the definition of
bool that it used, but previously it would define its own type if the
system-provided bool had a different size. That was liable to cause
memory layout problems when interacting with system and third-party
libraries on (by now hypothetical) computers with wider _Bool, and now
C23 has introduced a new problem by making bool a built-in datatype
(like C++), so the fallback code doesn't even compile. We could
probably work around that, but then we'd be writing new untested code
for a computer that doesn't exist.
Instead, delete the unreachable and C23-uncompilable fallback code, and
let existing static assertions fail if the system-provided bool is too
wide. If we ever get a problem report from a real system, then it will
be time to figure out what to do about it in a way that also works on
modern compilers.
Note on C++: Previously we avoided including <stdbool.h> or trying to
define a new bool type in headers that might be included by C++ code.
These days we might as well just include <stdbool.h> unconditionally:
it should be visible to C++11 but do nothing, just as in C23. We
already include <stdint.h> without C++ guards in c.h, and that falls
under the same C99-compatibility section of the C++11 standard as
<stdbool.h>, so let's remove the guards here too.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/3198438.1731895163%40sss.pgh.pa.us
Avoid leaking the prior value when updating the "connection"
state variable.
Ditto for ECPGstruct_sizeof. (It seems like this one ought to
be statement-local, but testing says it isn't, and I didn't
feel like diving deeper.)
The actual_type[] entries are statement-local, though, so
no need to mm_strdup() strings stored in them.
Likewise, sqlda variables are statement-local, so we can
loc_alloc them.
Also clean up sloppiness around management of the argsinsert and
argsresult lists.
progname changes are strictly to prevent valgrind from complaining
about leaked allocations.
With this, valgrind reports zero leakage in the ecpg preprocessor
for all of our ecpg regression test cases.
Discussion: https://postgr.es/m/2011420.1713493114@sss.pgh.pa.us
This didn't work earlier in the patch series (I think some of
the strings were ending up in data-type-related structures),
but apparently we're now clean enough for it. This considerably
reduces process-lifespan memory leakage.
Discussion: https://postgr.es/m/2011420.1713493114@sss.pgh.pa.us
ECPGfree_type() and related functions were quite incomplete
about removing subsidiary data structures. Possibly this is
because ecpg wasn't careful to make sure said data structures
always had their own storage. Previous patches in this series
cleaned up a lot of that, and I had to add a couple more
mm_strdup's here.
Also, ecpg.trailer tended to overwrite struct_member_list[struct_level]
without bothering to free up its previous contents, thus potentially
leaking a lot of struct-member-related storage. Add
ECPGfree_struct_member() calls at appropriate points.
Discussion: https://postgr.es/m/2011420.1713493114@sss.pgh.pa.us
This reverts commit 2cf91ccb73.
When using the old msvcrt.dll, MinGW would supply its own dummy version
of _configthreadlocale() that just returns -1 if you try to use it. For
a time we tolerated that to shut the build farm up. We would fall back
to code that was enough for the tests to pass, but it would surely have
risked crashing a real multithreaded program.
We don't need that kludge anymore, because we can count on ucrt. We
expect the real _configthreadlocale() to be present, and the ECPG tests
will now fail if it isn't. The workaround was dead code and it's time
to revert it.
(A later patch still under review proposes to remove this use of
_configthreadlocale() completely but we're unwinding this code in
steps.)
Reviewed-by: Peter Eisentraut <peter@eisentraut.org>
Discussion: https://postgr.es/m/d9e7731c-ca1b-477c-9298-fa51e135574a%40eisentraut.org
All modern Windows systems have _configthreadlocale(). It was first
introduced in msvcr80.dll from Visual Studio 2005. Historically, MinGW
was stuck on even older msvcrt.dll, but added its own dummy
implementation of the function when using msvcrt.dll years ago anyway,
effectively rendering the configure test useless. In practice we don't
encounter the dummy anymore because modern MinGW uses ucrt.
Reviewed-by: Peter Eisentraut <peter@eisentraut.org>
Discussion: https://postgr.es/m/CWZBBRR6YA8D.8EHMDRGLCKCD%40neon.tech
It was possible for the code to read out-of-bound data from the
"day_tab" table with some crafted input data. Let's treat these as
invalid input as the month number is incorrect.
A test is added to test this case with a check on the errno returned by
the decoding routine. A test close to the new one added in this commit
was testing for a failure, but did not look at the errno generated, so
let's use this commit to also change it, adding a check on the errno
returned by DecodeDateTime().
Like the other test scripts, dt_test should likely be expanded to
include more checks based on the errnos generated in these code paths.
This is left as future work.
This issue exists since 2e6f97560a, so backpatch all the way down.
Reported-by: Pavel Nekrasov
Author: Bruce Momjian, Pavel Nekrasov
Discussion: https://postgr.es/m/18614-6bbe00117352309e@postgresql.org
Backpatch-through: 12
Previously, ecpg_log() always called ECPGget_sqlca() to retrieve sqlca,
even though it was only needed for debug logging. This commit updates
ecpg_log() to call ECPGget_sqlca() only when debug logging is enabled.
Author: Yuto Sasaki
Reviewed-by: Alvaro Herrera, Tom Lane, Fujii Masao
Discussion: https://postgr.es/m/TY2PR01MB3628A85689649BABC9A1C6C3C1782@TY2PR01MB3628.jpnprd01.prod.outlook.com
Don't get confused by an unmatched right brace in the input.
(Previously, this led to discarding information about file-level
variables and then possibly crashing.)
Detect, rather than crash on, an attempt to index into a non-array
variable.
As before, in the absence of field complaints I'm not too
excited about back-patching these.
Per valgrind testing by Alexander Lakhin.
Discussion: https://postgr.es/m/a239aec2-6c79-5fc9-9272-cea41158a360@gmail.com
Avoid null-pointer crash when considering a cursor declaration
that's outside any C function (a case which is useless anyway).
Ensure a cursor for a prepared statement is marked as initially
not open. At worst, if we chanced to get not-already-zeroed memory
from malloc(), this oversight would result in failing to issue a
"cursor "foo" has been declared but not opened" warning that would
have been appropriate.
Avoid running off the end of the buffer when there are mismatched
square brackets following a variable name. This could lead to
SIGSEGV after reaching the end of memory.
Given the lack of field complaints, none of these seem to be worth
back-patching, but let's clean them up in HEAD.
Per valgrind testing by Alexander Lakhin.
Discussion: https://postgr.es/m/5f5bcecd-d7ec-b8c0-6c92-d1a7c6e0f639@gmail.com
Put the rule type at the start not the end, and put spaces
between the constitutent token names instead of smashing them
into an illegible mess. This has no functional impact but
I think it makes the rules a great deal more readable.
Discussion: https://postgr.es/m/1185216.1724001216@sss.pgh.pa.us
parse.pl contains several constant tables that describe tweaks
to be made to the backend grammar. In the same spirit as
00b0e7204, add cross-checks that each table entry is used at
least once (or exactly once if that's appropriate). This should
help catch cases where adjustments to the backend grammar cause
a table entry not to match as expected.
Per suggestion from Michael Paquier.
Discussion: https://postgr.es/m/ZsLVbjsc5x5Saesg@paquier.xyz
Careless string hacking caused parse.pl to transform gram.y's
declaration
%nonassoc IDENT PARTITION RANGE ROWS ...
into
%nonassoc IDENT
%nonassoc CSTRING PARTITION RANGE ROWS ...
It turns out that this has no semantic impact, because the
generated preproc.c is exactly the same either way (if you
inject a blank line to keep line numbers the same).
Nonetheless, given the great emphasis that the commentary in
gram.y places on keeping those other keywords at the same
precedence level as IDENT, this seems like foolishly risking ecpg
behaving differently from the core parser. Adjust the code so
that CSTRING is added to the precedence line without breaking it
into two lines.
Discussion: https://postgr.es/m/2157151.1713540065@sss.pgh.pa.us
Invent a notion of "local" storage that will automatically be
reclaimed at the end of each statement. Use this for location
strings as well as other visibly short-lived data within the parser.
Also, make cat_str and make_str return local storage and not free
their inputs, which allows dispensing with a whole lot of retail
mm_strdup calls. We do have to add some new ones in places where
a local-lifetime string needs to be added to a longer-lived data
structure, but on balance there are a lot less mm_strdup calls than
before.
In hopes of flushing out places where changes were necessary,
I changed YYLTYPE from "char *" to "const char *", which forced
const-ification of various function arguments that probably
should've been like that all along.
This still leaks somewhat more memory than v17, but that will be
cleaned up in future commits.
Discussion: https://postgr.es/m/2011420.1713493114@sss.pgh.pa.us
mm_alloc and mm_strdup were in type.c, which seems a completely
random choice. No doubt the original author thought two small
functions didn't deserve their own file. But I'm about to add
some more memory-management stuff beside them, so let's put them
in a less surprising place. This seems like a better home for
mmerror, mmfatal, and the cat_str/make_str family, too.
Discussion: https://postgr.es/m/2011420.1713493114@sss.pgh.pa.us
Most productions in the preprocessor grammar construct strings
representing SQL or C statements or fragments thereof. Instead
of returning these as <str> results of the productions, return
them as "location" values, taking advantage of Bison's flexibility
about what a location is. We aren't really giving up anything
thereby, since ecpg's error reports have always just given line
numbers, and that's tracked separately. The advantage of this
is that a single instance of the YYLLOC_DEFAULT macro can
perform all the work needed by the vast majority of productions,
including all the ones made automatically by parse.pl. This
avoids having large numbers of effectively-identical productions,
which tickles an optimization inefficiency in recent versions of
clang. (This patch reduces the compilation time for preproc.o
by more than 100-fold with clang 16, and is visibly helpful with
gcc too.) The compiled parser is noticeably smaller as well.
A disadvantage of this approach is that YYLLOC_DEFAULT is applied
before running the production's semantic action (if any). This
means it cannot use the method favored by cat_str() of free'ing
all the input strings; if the action needs to look at the input
strings, it'd be looking at dangling storage. As this stands,
therefore, it leaks memory like a sieve. This is already a big
patch though, and fixing the memory management seems like a
separable problem, so let's leave that for the next step.
(This does remove some free() calls that I'd have had to touch
anyway, in the expectation that the next step will manage
memory reclamation quite differently.)
Most of the changes here are mindless substitution of "@N" for
"$N" in grammar rules; see the changes to README.parser for
an explanation.
Discussion: https://postgr.es/m/2011420.1713493114@sss.pgh.pa.us
Remove a lot of cruft, clean up and document what's left.
This produces the same preproc.y output as before, except for
fewer blank lines. (It's not like we're making any attempt to
match the layout of gram.y, so I removed the one bit of logic
that seemed to have that in mind.)
Discussion: https://postgr.es/m/2011420.1713493114@sss.pgh.pa.us
As noted in the previous commit, check_rules.pl is now entirely
redundant with checks made by parse.pl, or would be if it weren't
for the places where it's wrong. It's a waste of build cycles
and maintenance effort, so remove it.
Discussion: https://postgr.es/m/2011420.1713493114@sss.pgh.pa.us
README.parser is the user's manual, such as it is, for parse.pl.
It's rather poorly written if you ask me; so try to improve it.
(More could be written here, but this at least covers the same
info in a more organized fashion.)
Also, the single solitary line of usage info in parse.pl itself
was a lie. Replace.
Add some error checks that the ecpg.addons entries meet the syntax
rules set forth in README.parser. One of them didn't, but
accidentally worked anyway because the logic in include_addon is
such that 'block' is the default behavior.
Also add a cross-check that each ecpg.addons entry is matched exactly
once in the backend grammar. This exposed that there are two dead
entries there --- they are dead because the %replace_types table in
parse.pl causes their nonterminals to be ignored altogether.
Removing them doesn't change the generated preproc.y file.
(This implies that check_rules.pl is completely worthless and should
be nuked: it adds build cycles and maintenance effort while failing
to reliably accomplish its one job of detecting dead rules. I'll
do that separately.)
Discussion: https://postgr.es/m/2011420.1713493114@sss.pgh.pa.us
The preprocessor really should not have done this to begin with.
The space after '&' was masked by ECPGconnect's skipping spaces
before option keywords, and the space before by dint of libpq
being (mostly) insensitive to trailing space in option values.
We fixed the one known problem with that in 920d51979. Hence
this patch is mostly cosmetic, and we'll just change it in HEAD.
Discussion: https://postgr.es/m/TY2PR01MB36286A7B97B9A15793335D18C1772@TY2PR01MB3628.jpnprd01.prod.outlook.com
