Thread: Fix inappropriate uses of atol()
I noticed (during [0]) to some uses of the function atol() seem inappropriate. Either they assume that sizeof(long)==8 and so might truncate data if not, or they are gratuitous because the surrounding code does not use the long type. This patch fixes these, by using atoll() or atoi() instead. (There are still some atol() calls left after this, which seemed ok to me.) In the past, Windows didn't have atoll(), but the online documentation appears to indicate that this now works in VS 2015 and later, which is what we support at the moment. The Cirrus CI passes. [0]: https://www.postgresql.org/message-id/flat/5d216d1c-91f6-4cbe-95e2-b4cbd930520c@ewie.name
Attachment
On 03/08/2024 14:04, Peter Eisentraut wrote:
> I noticed (during [0]) to some uses of the function atol() seem
> inappropriate. Either they assume that sizeof(long)==8 and so might
> truncate data if not, or they are gratuitous because the surrounding
> code does not use the long type. This patch fixes these, by using
> atoll() or atoi() instead. (There are still some atol() calls left
> after this, which seemed ok to me.)
>
> In the past, Windows didn't have atoll(), but the online documentation
> appears to indicate that this now works in VS 2015 and later, which is
> what we support at the moment. The Cirrus CI passes.
+1 except for this one:
> diff --git a/src/interfaces/ecpg/preproc/ecpg.trailer b/src/interfaces/ecpg/preproc/ecpg.trailer
> index b2aa44f36dd..8ac1c5c9eda 100644
> --- a/src/interfaces/ecpg/preproc/ecpg.trailer
> +++ b/src/interfaces/ecpg/preproc/ecpg.trailer
> @@ -217,7 +217,7 @@ char_variable: cvariable
> enum ECPGttype type = p->type->type;
>
> /* If we have just one character this is not a string */
> - if (atol(p->type->size) == 1)
> + if (atoi(p->type->size) == 1)
> mmerror(PARSE_ERROR, ET_ERROR, "invalid data type");
> else
> {
>
In principle you can have an array larger than INT_MAX. However, this is
a pretty weak test anyway. I think this is what the error is meant for:
EXEC SQL BEGIN DECLARE SECTION;
char connstr;
EXEC SQL END DECLARE SECTION;
EXEC SQL CONNECT TO :connstr;
This also produces the error, which seems fine:
EXEC SQL BEGIN DECLARE SECTION;
char connstr[1];
EXEC SQL END DECLARE SECTION;
EXEC SQL CONNECT TO :connstr;
This also produces the error, which does not seem good (if you replace 1
with 2 here, it works):
EXEC SQL BEGIN DECLARE SECTION;
char connstr[1+100];
EXEC SQL END DECLARE SECTION;
EXEC SQL CONNECT TO :connstr;
You can work around that with:
#define LEN (1 + 100)
EXEC SQL BEGIN DECLARE SECTION;
char connstr[LEN];
EXEC SQL END DECLARE SECTION;
EXEC SQL CONNECT TO :connstr;
The grammar currently balks on arrays larger than INT_MAX, giving a
"syntax error", which I don't think is correct, because at least my C
compiler accepts it in a non-ecpg context:
EXEC SQL BEGIN DECLARE SECTION;
char connstr[2147483648];
EXEC SQL END DECLARE SECTION;
EXEC SQL CONNECT TO :connstr;
Overall I think we should just leave this as it is. If we want to do
something here, would be good to address those cases that are currently
bogus, but it's probably not worth the effort.
--
Heikki Linnakangas
Neon (https://neon.tech)
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 03/08/2024 14:04, Peter Eisentraut wrote:
>> I noticed (during [0]) to some uses of the function atol() seem
>> inappropriate.
> +1 except for this one:
>> /* If we have just one character this is not a string */
>> - if (atol(p->type->size) == 1)
>> + if (atoi(p->type->size) == 1)
>> mmerror(PARSE_ERROR, ET_ERROR, "invalid data type");
How about
- if (atol(p->type->size) == 1)
+ if (strcmp(p->type->size, "1") == 0)
? I've not actually tested, but this should catch the cases the
warning is meant to catch while not complaining about any of the
examples you give. I'm not sure if leading/trailing spaces
would fool it (i.e., "char foo[ 1 ];"). But even if they do,
that doesn't seem disastrous.
regards, tom lane
On 03/08/2024 18:20, Tom Lane wrote: > Heikki Linnakangas <hlinnaka@iki.fi> writes: >> On 03/08/2024 14:04, Peter Eisentraut wrote: >>> I noticed (during [0]) to some uses of the function atol() seem >>> inappropriate. > >> +1 except for this one: > >>> /* If we have just one character this is not a string */ >>> - if (atol(p->type->size) == 1) >>> + if (atoi(p->type->size) == 1) >>> mmerror(PARSE_ERROR, ET_ERROR, "invalid data type"); > > How about > > - if (atol(p->type->size) == 1) > + if (strcmp(p->type->size, "1") == 0) > > ? I've not actually tested, but this should catch the cases the > warning is meant to catch while not complaining about any of the > examples you give. Makes sense. > I'm not sure if leading/trailing spaces > would fool it (i.e., "char foo[ 1 ];"). But even if they do, > that doesn't seem disastrous. Right. There are many ways to fool it in that direction, e.g. #define ONE 1 char foo[ONE]; I'm actually not even sure if it's intentional to throw the error even with "char[1]". It makes sense to give an error on "char", but who says that "char[1]" isn't a valid string? Not a very useful string, because it can only hold the empty string, but a string nevertheless, and sometimes an empty string is exactly what you want. If we can easily distinguish between "char" and any array of char here, might be best to accept the all arrays regardless of the length. -- Heikki Linnakangas Neon (https://neon.tech)
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> I'm actually not even sure if it's intentional to throw the error even
> with "char[1]". It makes sense to give an error on "char", but who says
> that "char[1]" isn't a valid string?
I agree that that behavior looks more like an implementation artifact
than anything else.
> If we can easily distinguish between "char" and any array of char here,
> might be best to accept the all arrays regardless of the length.
The data structure is so poorly documented that I'm hesitant to try
to do that. It might work to test for type == ECPGt_array, but then
why is the immediately following code explicitly allowing for both
that case and not-array? I'm also fairly unsure how ECPGt_string
fits in here. If this were an important point then it might be
worth trying to reverse-engineer all this, but right now I have
better things to do.
regards, tom lane
On 03.08.24 16:07, Heikki Linnakangas wrote:
> On 03/08/2024 14:04, Peter Eisentraut wrote:
>> I noticed (during [0]) to some uses of the function atol() seem
>> inappropriate. Either they assume that sizeof(long)==8 and so might
>> truncate data if not, or they are gratuitous because the surrounding
>> code does not use the long type. This patch fixes these, by using
>> atoll() or atoi() instead. (There are still some atol() calls left
>> after this, which seemed ok to me.)
>>
>> In the past, Windows didn't have atoll(), but the online documentation
>> appears to indicate that this now works in VS 2015 and later, which is
>> what we support at the moment. The Cirrus CI passes.
> +1 except for this one:
>
>> diff --git a/src/interfaces/ecpg/preproc/ecpg.trailer
>> b/src/interfaces/ecpg/preproc/ecpg.trailer
>> index b2aa44f36dd..8ac1c5c9eda 100644
>> --- a/src/interfaces/ecpg/preproc/ecpg.trailer
>> +++ b/src/interfaces/ecpg/preproc/ecpg.trailer
>> @@ -217,7 +217,7 @@ char_variable: cvariable
>> enum ECPGttype type = p->type->type;
>>
>> /* If we have just one character this is not a string */
>> - if (atol(p->type->size) == 1)
>> + if (atoi(p->type->size) == 1)
>> mmerror(PARSE_ERROR, ET_ERROR, "invalid data type");
>> else
>> {
I committed it without this hunk.