public inbox for [email protected]  

help / color / mirror / Atom feed

From: Heikki Linnakangas <[email protected]>
To: Peter Eisentraut <[email protected]>
To: pgsql-hackers <[email protected]>
Subject: Re: Fix inappropriate uses of atol()
Date: Sat, 3 Aug 2024 17:07:46 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>

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)

view thread (3+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: Fix inappropriate uses of atol()
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox