Обсуждение: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

Поиск
Список
Период
Сортировка

Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Ranier Vilela
Дата:
Hi,

If a null locale is reached in these paths.
elog will dereference a null pointer.

best regards,

Ranier Vilela
Вложения

Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Robert Haas
Дата:
On Fri, Sep 1, 2023 at 11:47 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
> If a null locale is reached in these paths.
> elog will dereference a null pointer.

True. That's sloppy coding.

I don't know enough about this code to be sure whether the error
messages that you propose are for the best.

--
Robert Haas
EDB: http://www.enterprisedb.com



Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Ranier Vilela
Дата:


Em sex., 1 de set. de 2023 às 17:17, Robert Haas <robertmhaas@gmail.com> escreveu:
On Fri, Sep 1, 2023 at 11:47 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
> If a null locale is reached in these paths.
> elog will dereference a null pointer.

True. That's sloppy coding.

I don't know enough about this code to be sure whether the error
messages that you propose are for the best.
I tried to keep the same behavior as before.
Note that if the locale equals COLLPROVIDER_LIBC, 
the message to the user will be the same.

best regards,
Ranier Vilela

Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Michael Paquier
Дата:
On Sat, Sep 02, 2023 at 09:13:11AM -0300, Ranier Vilela wrote:
> I tried to keep the same behavior as before.
> Note that if the locale equals COLLPROVIDER_LIBC,
> the message to the user will be the same.

-    /* shouldn't happen */
-    elog(ERROR, "unsupported collprovider: %c", locale->provider);
+    elog(ERROR, "collprovider '%c' does not support pg_strnxfrm_prefix()",
+             locale->provider);

Based on what's written here, these messages could be better because
full sentences are not encouraged in error messages, even for
non-translated elogs:
https://www.postgresql.org/docs/current/error-style-guide.html

Perhaps something like "could not use collprovider %c: no support for
%s", where the function name is used, would be more consistent.
--
Michael

Вложения

Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Ranier Vilela
Дата:
Em dom., 3 de set. de 2023 às 22:01, Michael Paquier <michael@paquier.xyz> escreveu:
On Sat, Sep 02, 2023 at 09:13:11AM -0300, Ranier Vilela wrote:
> I tried to keep the same behavior as before.
> Note that if the locale equals COLLPROVIDER_LIBC,
> the message to the user will be the same.

-    /* shouldn't happen */
-    elog(ERROR, "unsupported collprovider: %c", locale->provider);
+    elog(ERROR, "collprovider '%c' does not support pg_strnxfrm_prefix()",
+             locale->provider);

Based on what's written here, these messages could be better because
full sentences are not encouraged in error messages, even for
non-translated elogs:
https://www.postgresql.org/docs/current/error-style-guide.html

Perhaps something like "could not use collprovider %c: no support for
%s", where the function name is used, would be more consistent.
Sure.
I have no objection to the wording of the message.
If there is consensus, I can send another patch.

best regards,
Ranier Vilela

Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Ranier Vilela
Дата:
Em seg., 4 de set. de 2023 às 11:27, Ranier Vilela <ranier.vf@gmail.com> escreveu:
Em dom., 3 de set. de 2023 às 22:01, Michael Paquier <michael@paquier.xyz> escreveu:
On Sat, Sep 02, 2023 at 09:13:11AM -0300, Ranier Vilela wrote:
> I tried to keep the same behavior as before.
> Note that if the locale equals COLLPROVIDER_LIBC,
> the message to the user will be the same.

-    /* shouldn't happen */
-    elog(ERROR, "unsupported collprovider: %c", locale->provider);
+    elog(ERROR, "collprovider '%c' does not support pg_strnxfrm_prefix()",
+             locale->provider);

Based on what's written here, these messages could be better because
full sentences are not encouraged in error messages, even for
non-translated elogs:
https://www.postgresql.org/docs/current/error-style-guide.html

Perhaps something like "could not use collprovider %c: no support for
%s", where the function name is used, would be more consistent.
Sure.
I have no objection to the wording of the message.
If there is consensus, I can send another patch.
I think no one objected.

v1 attached.

best regards,
Ranier Vilela
Вложения

Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Michael Paquier
Дата:
On Wed, Sep 06, 2023 at 07:57:03AM -0300, Ranier Vilela wrote:
> I think no one objected.

Looking closer, there is much more inconsistency in this file
depending on the routine called.  How about something like the v2
attached instead to provide more context in the error message about
the function called?  Let's say, when the provider is known, we could
use:
+       elog(ERROR, "unsupported collprovider (%s): %c",
+            "pg_strncoll", locale->provider);

And when the provider is not known, we could use:
+       elog(ERROR, "unsupported collprovider (%s)", "pg_myfunc");

@Jeff (added now in CC), the refactoring done in d87d548c seems to be
at the origin of this confusion, because, before this commit, we never
generated this specific error for all these APIs where the locale is
undefined.  What is your take here?
--
Michael

Вложения

Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Jeff Davis
Дата:
On Fri, 2023-09-08 at 15:24 +0900, Michael Paquier wrote:
> Looking closer, there is much more inconsistency in this file
> depending on the routine called.  How about something like the v2
> attached instead to provide more context in the error message about
> the function called?  Let's say, when the provider is known, we could
> use:
> +       elog(ERROR, "unsupported collprovider (%s): %c",
> +            "pg_strncoll", locale->provider);

+1, thank you.

> And when the provider is not known, we could use:
> +       elog(ERROR, "unsupported collprovider (%s)", "pg_myfunc");

It's not that the provider is "not known" -- if locale is NULL, then
the provider is known to be COLLPROVIDER_LIBC. So perhaps we can
instead do something like:

   char provider = locale ? locale->provider : COLLPROVIDER_LIBC;

and then always follow the first error format?

[ Aside: it might be nice to refactor so that we used a pointer to a
special static struct rather than NULL, which would cut down on these
kinds of special cases. I had considered doing that before and didn't
get around to it. ]

> @Jeff (added now in CC), the refactoring done in d87d548c seems to be
> at the origin of this confusion, because, before this commit, we
> never
> generated this specific error for all these APIs where the locale is
> undefined.  What is your take here?

Agreed.

Regards,
    Jeff Davis




Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Ranier Vilela
Дата:
Em sex., 8 de set. de 2023 às 03:24, Michael Paquier <michael@paquier.xyz> escreveu:
On Wed, Sep 06, 2023 at 07:57:03AM -0300, Ranier Vilela wrote:
> I think no one objected.

Looking closer, there is much more inconsistency in this file
depending on the routine called.  How about something like the v2
attached instead to provide more context in the error message about
the function called?
+1
But as Jeff mentioned, when the locale is NULL, 
the provider is known to be COLLPROVIDER_LIBC.

I think we can use this to provide an error message,
when the locale is NULL.

What do you think about v3 attached?

best regards,
Ranier Vilela
Вложения

Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Michael Paquier
Дата:
On Sun, Sep 10, 2023 at 06:28:08PM -0300, Ranier Vilela wrote:
> +1
> But as Jeff mentioned, when the locale is NULL,
> the provider is known to be COLLPROVIDER_LIBC.
>
> I think we can use this to provide an error message,
> when the locale is NULL.
>
> What do you think about v3 attached?

This does not apply for me on HEAD, and it seems to me that the patch
has some parts that apply on top of v2 (or v1?) while others would
apply to HEAD.

Anyway, what you are suggesting to change compared to v2 is that:

+    /*
+     * if locale is NULL, then
+     * the provider is known to be COLLPROVIDER_LIBC
+     */
     if (!locale)
-        elog(ERROR, "unsupported collprovider");
+        elog(ERROR, "collprovider '%c' does not support (%s)",
+            COLLPROVIDER_LIBC, "pg_strxfrm_prefix");

I'm OK with enforcing COLLPROVIDER_LIBC in this path, but I also value
consistency across all the error messages of this file.  After
sleeping on it, and as that's a set of elogs, "unsupported
collprovider" is fine for me across the board as these should not be
user-visible.

This should be made consistent down to 16, I guess, but only after
16.0 is tagged as we are in release week.
--
Michael

Вложения

Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Jeff Davis
Дата:
On Mon, 2023-09-11 at 07:24 +0900, Michael Paquier wrote:
> I'm OK with enforcing COLLPROVIDER_LIBC in this path, but I also
> value
> consistency across all the error messages of this file.  After
> sleeping on it, and as that's a set of elogs, "unsupported
> collprovider" is fine for me across the board as these should not be
> user-visible.

That's fine with me.

Regards,
    Jeff Davis




Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Michael Paquier
Дата:
On Mon, Sep 11, 2023 at 12:15:49PM -0700, Jeff Davis wrote:
> That's fine with me.

Okay.  Then, please find attached a v4 for HEAD and REL_16_STABLE.
--
Michael

Вложения

Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Ranier Vilela
Дата:
Em seg., 11 de set. de 2023 às 21:03, Michael Paquier <michael@paquier.xyz> escreveu:
On Mon, Sep 11, 2023 at 12:15:49PM -0700, Jeff Davis wrote:
> That's fine with me.

Okay.  Then, please find attached a v4 for HEAD and REL_16_STABLE.
LGTM.

best regards,
Ranier Vilela

Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Jeff Davis
Дата:
On Tue, 2023-09-12 at 09:03 +0900, Michael Paquier wrote:
> On Mon, Sep 11, 2023 at 12:15:49PM -0700, Jeff Davis wrote:
> > That's fine with me.
>
> Okay.  Then, please find attached a v4 for HEAD and REL_16_STABLE.

One question: would it make sense to use __func__?

Other than that, looks good. Thank you.

Regards,
    Jeff Davis




Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Ranier Vilela
Дата:
Em ter., 12 de set. de 2023 às 17:51, Jeff Davis <pgsql@j-davis.com> escreveu:
On Tue, 2023-09-12 at 09:03 +0900, Michael Paquier wrote:
> On Mon, Sep 11, 2023 at 12:15:49PM -0700, Jeff Davis wrote:
> > That's fine with me.
>
> Okay.  Then, please find attached a v4 for HEAD and REL_16_STABLE.

One question: would it make sense to use __func__?
According to the msvc documentation, __func__ requires C++11.

I think that is not a good idea.

best regards,
Ranier Vilela

Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Kyotaro Horiguchi
Дата:
At Tue, 12 Sep 2023 09:03:27 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Mon, Sep 11, 2023 at 12:15:49PM -0700, Jeff Davis wrote:
> > That's fine with me.
> 
> Okay.  Then, please find attached a v4 for HEAD and REL_16_STABLE.

For example, they result in the following message:

ERROR:  unsupported collprovider (pg_strcoll): i

Even if it is an elog message, I believe we can make it clearer. The
pg_strcoll seems like a collation privier at first glance. Not sure
about others, though, I would spell it as the follows instead:

ERROR:  unsupported collprovider in pg_strcoll: i
ERROR:  unsupported collprovider in pg_strcoll(): i

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Michael Paquier
Дата:
On Tue, Sep 12, 2023 at 09:40:04PM -0300, Ranier Vilela wrote:
> I think that is not a good idea.

Hm?  We already use __func__ across the tree even on Windows and
nobody has complained about that.  Using a macro for the elog()
generated would be slightly more elegant, actually.
--
Michael

Вложения

Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Michael Paquier
Дата:
On Wed, Sep 13, 2023 at 09:59:22AM +0900, Kyotaro Horiguchi wrote:
> At Tue, 12 Sep 2023 09:03:27 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> > On Mon, Sep 11, 2023 at 12:15:49PM -0700, Jeff Davis wrote:
> > > That's fine with me.
> >
> > Okay.  Then, please find attached a v4 for HEAD and REL_16_STABLE.
>
> For example, they result in the following message:
>
> ERROR:  unsupported collprovider (pg_strcoll): i
>
> Even if it is an elog message, I believe we can make it clearer. The
> pg_strcoll seems like a collation privier at first glance. Not sure
> about others, though, I would spell it as the follows instead:
>
> ERROR:  unsupported collprovider in pg_strcoll: i
> ERROR:  unsupported collprovider in pg_strcoll(): i

Hmm.  I see your point, one could be confused that the function name
is the provider with this wording.  How about that instead:
 ERROR:  unsupported collprovider for %s: %c

I've hidden that in a macro that uses __func__ as Jeff has suggested.

What do you think?
--
Michael

Вложения

Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Jeff Davis
Дата:
On Wed, 2023-09-13 at 11:48 +0900, Michael Paquier wrote:
> Hmm.  I see your point, one could be confused that the function name
> is the provider with this wording.  How about that instead:
>  ERROR:  unsupported collprovider for %s: %c
>
> I've hidden that in a macro that uses __func__ as Jeff has suggested.
>
> What do you think?

Looks good to me, thank you.

Regards,
    Jeff Davis




Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Michael Paquier
Дата:
On Wed, Sep 13, 2023 at 08:14:11AM -0700, Jeff Davis wrote:
> Looks good to me, thank you.

Applied, then.  Thanks.
--
Michael

Вложения

Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)

От
Ranier Vilela
Дата:
Em qua., 13 de set. de 2023 às 22:32, Michael Paquier <michael@paquier.xyz> escreveu:
On Wed, Sep 13, 2023 at 08:14:11AM -0700, Jeff Davis wrote:
> Looks good to me, thank you.

Applied, then.  Thanks.
Thank you Michael, for the commit.

best regards,
Ranier Vilela