Обсуждение: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)
Hi,
If a null locale is reached in these paths.
elog will dereference a null pointer.
elog will dereference a null pointer.
best regards,
Ranier Vilela
Вложения
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
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,
Note that if the locale equals COLLPROVIDER_LIBC,
the message to the user will be the same.
best regards,
Ranier Vilela
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
Вложения
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.
If there is consensus, I can send another patch.
best regards,
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
Вложения
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
Вложения
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
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
Вложения
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
Вложения
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
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
Вложения
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
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
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
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
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
Вложения
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
Вложения
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
On Wed, Sep 13, 2023 at 08:14:11AM -0700, Jeff Davis wrote: > Looks good to me, thank you. Applied, then. Thanks. -- Michael
Вложения
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