Horton

Thread

Commits

  1. libpq: Discard leading and trailing spaces for parameters and values in URIs

  2. ecpg: avoid adding whitespace around '&' in connection URLs.

  3. Parse libpq's "keepalives" option more like other integer options.

  1. [BUG FIX]Connection fails with whitespace after keepalives parameter value

    Yuto Sasaki (Fujitsu) <sasaki.yuto-00@fujitsu.com> — 2024-10-01T05:11:21Z

    Hi hackers,

I've discovered a bug in ECPG that causes database connection failures. This issue
appears to stem from libpq layer.

Found bug: The EXEC SQL CONNECT TO statement fails to connect to the database.
Specifically, the following code:

```c
EXEC SQL CONNECT TO tcp:postgresql://localhost:5432/postgres?keepalives=1
&keepalives_idle=1 USER yuto;
```

When precompiled and executed, this code fails to connect to the database.
Error message:

```
failed: keepalives parameter must be an integer
```

Steps to reproduce: The issue can be reproduced using the attached pgc file.

Root cause: The method for parsing the keepalives parameter in the useKeepalives
function of the libpq library is not appropriate. Specifically, it doesn't
account for whitespace following the numeric value.

Proposed fix: strtol() can be replaced with pqParseIntParam() like other paramters.
This skips whitespaces.

Concerns:
1. This fix may limit parameter values to integers only.
   Example: keepalives=2147483648 (values larger than INT_MAX can't be read)
2. Whitespace after '=' that was previously not read will now be read.
   Example: keepalives= 1 (previously couldn't be read, now read as 1)
3. This issue was introduced in previous versions (as far as I could verify,
   from 9.0), so we may need to consider backpatching if necessary.

These concerns warrant further discussion.

I'd appreciate your review of this patch and these concerns. Thank you.

  2. Re: [BUG FIX]Connection fails with whitespace after keepalives parameter value

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-10-01T16:10:22Z

    
On 2024/10/01 14:11, Yuto Sasaki (Fujitsu) wrote:
> Hi hackers,
> I've discovered a bug in ECPG that causes database connection failures. This issue
> appears to stem from libpq layer.
> Found bug: The EXEC SQL CONNECT TO statement fails to connect to the database.
> Specifically, the following code:
> ```c
> EXEC SQL CONNECT TO tcp:postgresql://localhost:5432/postgres?keepalives=1
> &keepalives_idle=1 USER yuto;
> ```
> When precompiled and executed, this code fails to connect to the database.
> Error message:
> ```
> failed: keepalives parameter must be an integer
> ```
> Steps to reproduce: The issue can be reproduced using the attached pgc file.
> Root cause: The method for parsing the keepalives parameter in the useKeepalives
> function of the libpq library is not appropriate. Specifically, it doesn't
> account for whitespace following the numeric value.

Is a connection URL with whitespace, like "tcp:postgresql://localhost:5432/postgres?keepalives=1 & ...",
considered valid? If not, the issue seems to be that ecpg adds unnecessary whitespace
to the connection URL, especially after the "&" character.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  3. Re: [BUG FIX]Connection fails with whitespace after keepalives parameter value

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-01T16:29:15Z

    Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> On 2024/10/01 14:11, Yuto Sasaki (Fujitsu) wrote:
>> Root cause: The method for parsing the keepalives parameter in the useKeepalives
>> function of the libpq library is not appropriate. Specifically, it doesn't
>> account for whitespace following the numeric value.

> Is a connection URL with whitespace, like "tcp:postgresql://localhost:5432/postgres?keepalives=1 & ...",
> considered valid? If not, the issue seems to be that ecpg adds unnecessary whitespace
> to the connection URL, especially after the "&" character.

I agree with Sasaki-san that useKeepalives seems rather bogus: almost
every other place in fe-connect.c uses pqParseIntParam rather than
calling strtol directly, so why not this one?  We might have some
work to do in ecpg as well, though.

			regards, tom lane

  4. Re: [BUG FIX]Connection fails with whitespace after keepalives parameter value

    Michael Paquier <michael@paquier.xyz> — 2024-10-02T02:35:19Z

    On Tue, Oct 01, 2024 at 12:29:15PM -0400, Tom Lane wrote:
> Fujii Masao <masao.fujii@oss.nttdata.com> writes:
>> Is a connection URL with whitespace, like "tcp:postgresql://localhost:5432/postgres?keepalives=1 & ...",
>> considered valid? If not, the issue seems to be that ecpg adds unnecessary whitespace
>> to the connection URL, especially after the "&" character.
> 
> I agree with Sasaki-san that useKeepalives seems rather bogus: almost
> every other place in fe-connect.c uses pqParseIntParam rather than
> calling strtol directly, so why not this one?

Yes, it is a mistake to not use pqParseIntParam(), or
parse_int_param() depending on the branch.  This stuff has been
introduced by 4f4061b2dde1, where I've spent some time making sure
that leading and trailing whitespaces are discarded in this routine.

See also these examples where whitespaces are OK in a connection URL:
https://www.postgresql.org/message-id/20191021024020.GF1542%40paquier.xyz
--
Michael

  5. Re: [BUG FIX]Connection fails with whitespace after keepalives parameter value

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-10-02T09:40:08Z

    
On 2024/10/02 11:35, Michael Paquier wrote:
> On Tue, Oct 01, 2024 at 12:29:15PM -0400, Tom Lane wrote:
>> Fujii Masao <masao.fujii@oss.nttdata.com> writes:
>>> Is a connection URL with whitespace, like "tcp:postgresql://localhost:5432/postgres?keepalives=1 & ...",
>>> considered valid? If not, the issue seems to be that ecpg adds unnecessary whitespace
>>> to the connection URL, especially after the "&" character.
>>
>> I agree with Sasaki-san that useKeepalives seems rather bogus: almost
>> every other place in fe-connect.c uses pqParseIntParam rather than
>> calling strtol directly, so why not this one?

I have no objection to improving the handling of the keepalives parameter.
OTOH, I think ecpg might have an issue when converting the connection URI.


> Yes, it is a mistake to not use pqParseIntParam(), or
> parse_int_param() depending on the branch.  This stuff has been
> introduced by 4f4061b2dde1, where I've spent some time making sure
> that leading and trailing whitespaces are discarded in this routine.
> 
> See also these examples where whitespaces are OK in a connection URL:
> https://www.postgresql.org/message-id/20191021024020.GF1542%40paquier.xyz

For example, ecpg converts:

EXEC SQL CONNECT TO tcp:postgresql://localhost:5432/postgres?keepalives=1&keepalives_idle=1&keepalives_interval=1&keepalives_count=2 USER postgres;

into:

{ ECPGconnect(__LINE__, 0, "tcp:postgresql://localhost:5432/postgres?keepalives=1 & keepalives_idle=1 & keepalives_interval=1 & keepalives_count=2" , "postgres" , NULL , NULL, 0);


In the converted URI, whitespace is added before and after the ? character.
In my quick test, ECPGconnect() seems to handle this without error,
but when I tried the same URI in psql, it returned an error:

$ psql "postgresql://localhost:5432/postgres?keepalives=1 & keepalives_idle=1 & keepalives_interval=1 & keepalives_count=2"
psql: error: invalid URI query parameter: " keepalives_idle"


It seems that libpq may consider this URI invalid.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  6. Re: [BUG FIX]Connection fails with whitespace after keepalives parameter value

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-02T21:39:31Z

    Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> On 2024/10/02 11:35, Michael Paquier wrote:
>> On Tue, Oct 01, 2024 at 12:29:15PM -0400, Tom Lane wrote:
>>> I agree with Sasaki-san that useKeepalives seems rather bogus: almost
>>> every other place in fe-connect.c uses pqParseIntParam rather than
>>> calling strtol directly, so why not this one?

> I have no objection to improving the handling of the keepalives parameter.
> OTOH, I think ecpg might have an issue when converting the connection URI.

I went ahead and pushed Sasaki-san's patch.  I think anything we might
do in ecpg is probably just cosmetic and wouldn't get back-patched.

> In the converted URI, whitespace is added before and after the ? character.
> In my quick test, ECPGconnect() seems to handle this without error,
> but when I tried the same URI in psql, it returned an error:
> $ psql "postgresql://localhost:5432/postgres?keepalives=1 & keepalives_idle=1 & keepalives_interval=1 & keepalives_count=2"
> psql: error: invalid URI query parameter: " keepalives_idle"

Interesting.  This is unhappy about the space before a parameter name,
not the space after a parameter value, so it's a different issue.
But it's weird that ecpg takes it while libpq doesn't.  Could libecpg
be modifying/reassembling the URI string?  I didn't look.

			regards, tom lane

  7. Re: [BUG FIX]Connection fails with whitespace after keepalives parameter value

    Michael Paquier <michael@paquier.xyz> — 2024-10-03T00:42:00Z

    On Wed, Oct 02, 2024 at 05:39:31PM -0400, Tom Lane wrote:
> Interesting.  This is unhappy about the space before a parameter name,
> not the space after a parameter value, so it's a different issue.

conninfo_uri_parse_options() parses the URI as a set of option/values,
where conninfo_uri_parse_params.  If we were to be careful about
trailing and leading whitespaces for the parameter names, we need to
be careful about the special JDBC cases for "ssl" and "requiressl",
meaning that we should add more logic in conninfo_uri_decode() to
discard these.  That would apply a extra layer of sanity into the
values as well.

> But it's weird that ecpg takes it while libpq doesn't.  Could libecpg
> be modifying/reassembling the URI string?  I didn't look.

ECPGconnect() has some custom logic to discard trailing and leading
spaces: 
    /* Skip spaces before keyword */
    for (token1 = str; *token1 == ' '; token1++)
[...]
    token1[e] = '\0'; //skips trailing spaces.

The argument for libpq where we could be consistent is appealing.  How
about lifting things in libpq like the attached?  I wouldn't backpatch
that, but we have tests for URIs and I didn't break anything.
--
Michael

  8. Re: [BUG FIX]Connection fails with whitespace after keepalives parameter value

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-03T15:57:16Z

    I poked into the ecpg end of this and found that the extra space
is coming from one production in ecpg.trailer that's carelessly
using cat_str (which inserts spaces) instead of makeN_str
(which doesn't).  So it's pretty trivial to fix, as attached.

I do not think we could rip out ECPGconnect's logic to remove the
spaces at runtime, because that would break existing ecpg
applications until they're recompiled.  It might be worth adding
a comment there about why it's being done, though.

I don't have a strong opinion one way or the other about whether
we should make libpq permissive about extra spaces (as per
Michael's patch).  I guess you could argue that all of these
fixes are consistent with the principle of "be conservative
with what you send and liberal with what you accept".  But at
most I'd fix these remaining things in HEAD.

			regards, tom lane

  9. Re: [BUG FIX]Connection fails with whitespace after keepalives parameter value

    Michael Paquier <michael@paquier.xyz> — 2024-10-03T23:39:39Z

    On Thu, Oct 03, 2024 at 11:57:16AM -0400, Tom Lane wrote:
> I don't have a strong opinion one way or the other about whether
> we should make libpq permissive about extra spaces (as per
> Michael's patch).  I guess you could argue that all of these
> fixes are consistent with the principle of "be conservative
> with what you send and liberal with what you accept".  But at
> most I'd fix these remaining things in HEAD.

Removing this extra whitespace from the ECPG strings sounds good here.

FWIW, my argument about doing this in libpq is not really related to
ECPG: it feels inconsistent to apply one rule for the parameters and a
different one for the values in URIs.  So I'd be OK to see how this
goes on as a HEAD-only change.
--
Michael

  10. Re: [BUG FIX]Connection fails with whitespace after keepalives parameter value

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-04T00:12:28Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Oct 03, 2024 at 11:57:16AM -0400, Tom Lane wrote:
>> I don't have a strong opinion one way or the other about whether
>> we should make libpq permissive about extra spaces (as per
>> Michael's patch).  I guess you could argue that all of these
>> fixes are consistent with the principle of "be conservative
>> with what you send and liberal with what you accept".  But at
>> most I'd fix these remaining things in HEAD.

> Removing this extra whitespace from the ECPG strings sounds good here.

> FWIW, my argument about doing this in libpq is not really related to
> ECPG: it feels inconsistent to apply one rule for the parameters and a
> different one for the values in URIs.  So I'd be OK to see how this
> goes on as a HEAD-only change.

OK, if there's no objections let's push both remaining patches
to HEAD only.

			regards, tom lane

  11. Re: [BUG FIX]Connection fails with whitespace after keepalives parameter value

    Yuto Sasaki (Fujitsu) <sasaki.yuto-00@fujitsu.com> — 2024-10-04T02:59:08Z

    The patch looks good to me.

The URI parsing process has become more stringent, allowing for accurate
handling of leading and trailing whitespace. Additionally, a feature has
been implemented to detect errors when extraneous data is present at the
end of the URI. The proper functioning of these changes has been verified
through newly added test cases. I've also tested by removing the whitespace
handling code from ECPGConnect() and applying your patch to libpq, which
worked successfully.



________________________________
差出人: Michael Paquier
送信: 2024 年 10 月 3 日 (木曜日) 9:42
宛先: Tom Lane
Cc: Fujii Masao; Sasaki, Yuto/佐佐木 悠人; pgsql-hackers@lists.postgresql.org
件名: Re: [BUG FIX]Connection fails with whitespace after keepalives parameter value

On Wed, Oct 02, 2024 at 05:39:31PM -0400, Tom Lane wrote:
> Interesting.  This is unhappy about the space before a parameter name,
> not the space after a parameter value, so it's a different issue.

conninfo_uri_parse_options() parses the URI as a set of option/values,
where conninfo_uri_parse_params.  If we were to be careful about
trailing and leading whitespaces for the parameter names, we need to
be careful about the special JDBC cases for "ssl" and "requiressl",
meaning that we should add more logic in conninfo_uri_decode() to
discard these.  That would apply a extra layer of sanity into the
values as well.

> But it's weird that ecpg takes it while libpq doesn't.  Could libecpg
> be modifying/reassembling the URI string?  I didn't look.

ECPGconnect() has some custom logic to discard trailing and leading
spaces:
    /* Skip spaces before keyword */
    for (token1 = str; *token1 == ' '; token1++)
[...]
    token1[e] = '\0'; //skips trailing spaces.

The argument for libpq where we could be consistent is appealing.  How
about lifting things in libpq like the attached?  I wouldn't backpatch
that, but we have tests for URIs and I didn't break anything.
--
Michael

  12. Re: [BUG FIX]Connection fails with whitespace after keepalives parameter value

    Michael Paquier <michael@paquier.xyz> — 2024-10-04T03:19:36Z

    On Thu, Oct 03, 2024 at 08:12:28PM -0400, Tom Lane wrote:
> OK, if there's no objections let's push both remaining patches
> to HEAD only.

WFM.
--
Michael

  13. Re: [BUG FIX]Connection fails with whitespace after keepalives parameter value

    Michael Paquier <michael@paquier.xyz> — 2024-10-06T09:35:16Z

    On Thu, Oct 03, 2024 at 08:12:28PM -0400, Tom Lane wrote:
> OK, if there's no objections let's push both remaining patches
> to HEAD only.

Done as of f22e84df1dea and 430ce189fc45.
--
Michael

  14. Re: [BUG FIX]Connection fails with whitespace after keepalives parameter value

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-10-07T16:19:59Z

    
On 2024/10/06 18:35, Michael Paquier wrote:
> On Thu, Oct 03, 2024 at 08:12:28PM -0400, Tom Lane wrote:
>> OK, if there's no objections let's push both remaining patches
>> to HEAD only.
> 
> Done as of f22e84df1dea and 430ce189fc45.

Commit 430ce189fc45 unexpectedly caused psql to report the error
"error: trailing data found" when a connection URI contains
a whitespace, e.g., in a parameter value. For example,
the following command used to work but no longer does after this commit:

     $ psql -d "postgresql://localhost:5432/postgres?application_name=a b"

I'm not sure if this URI format is valid (according to RFC 3986), though.


+	for (const char *s = q; *s == ' '; s++)
+	{
+		q++;
+		continue;
+	}

Is the "continue" really necessary? Also could we simplify it like this?

     for (; *q == ' '; q++);

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  15. Re: [BUG FIX]Connection fails with whitespace after keepalives parameter value

    Michael Paquier <michael@paquier.xyz> — 2024-10-07T23:31:27Z

    On Tue, Oct 08, 2024 at 01:19:59AM +0900, Fujii Masao wrote:
> Commit 430ce189fc45 unexpectedly caused psql to report the error
> "error: trailing data found" when a connection URI contains
> a whitespace, e.g., in a parameter value. For example,
> the following command used to work but no longer does after this commit:
> 
>     $ psql -d "postgresql://localhost:5432/postgres?application_name=a b"
> 
> I'm not sure if this URI format is valid (according to RFC 3986),
> though.

I may be missing something, of course, but I am under the impression
that ' ' is invalid, meaning that you should use "a%20b" here to get
what you want as %20 would would be translated to a space character.

> Is the "continue" really necessary? Also could we simplify it like this?
> 
>     for (; *q == ' '; q++);

Sure, it could be changed this way.
--
Michael