Fix potential overflow risks from wcscpy and sprintf

On 06.06.25 22:50, Yan Haibo wrote:

This change stems from a recent static code analysis, which identified a
minor potential overflow issue. I would appreciate it if someone could
review the fix at their convenience.

Please provide more detail in each case what the issue is and how you
are proposing to fix it.

Note that PostgreSQL code is regularly run through various static
analysis tools, so if the issue were super-obvious, it would probably
have been noticed by now.

Thank you. Peter. It seems the patch may have been lost during our earlier communication, so I’ve reattached it here.
I hope it comes through correctly this time.
Best regards,
Haibo

________________________________
发件人: Peter Eisentraut <peter@eisentraut.org>
发送时间: 2025年6月11日 00:43
收件人: Yan Haibo <haibo.yan@hotmail.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
主题: Re: Fix potential overflow risks from wcscpy and sprintf

On 06.06.25 22:50, Yan Haibo wrote:

This change stems from a recent static code analysis, which identified a
minor potential overflow issue. I would appreciate it if someone could
review the fix at their convenience.

Please provide more detail in each case what the issue is and how you
are proposing to fix it.

Note that PostgreSQL code is regularly run through various static
analysis tools, so if the issue were super-obvious, it would probably
have been noticed by now.

Thank you. Peter. It seems the patch may have been lost during our earlier communication, so I’ve reattached it here.
I hope it comes through correctly this time.
Best regards,
Haibo

________________________________
发件人: Peter Eisentraut <peter@eisentraut.org>
发送时间: 2025年6月11日 00:43
收件人: Yan Haibo <haibo.yan@hotmail.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
主题: Re: Fix potential overflow risks from wcscpy and sprintf

On 06.06.25 22:50, Yan Haibo wrote:

This change stems from a recent static code analysis, which identified a
minor potential overflow issue. I would appreciate it if someone could
review the fix at their convenience.

Please provide more detail in each case what the issue is and how you
are proposing to fix it.

Note that PostgreSQL code is regularly run through various static
analysis tools, so if the issue were super-obvious, it would probably
have been noticed by now.

Yan Haibo <haibo.yan@hotmail.com> writes:

Thank you. Peter. It seems the patch may have been lost during our earlier communication, so I¡¯ve reattached it here.
I hope it comes through correctly this time.

Thanks for the patch.

Using wcsncpy in search_locale_enum() seems fine, assuming it exists
on Windows (note that code is Windows-only, possibly explaining why
we've not seen other static-analysis reports). I doubt there's any
actual bug there, since we're relying on Windows' own
LOCALE_NAME_MAX_LENGTH constant; but I agree the chain of reasoning
is kind of long. (But shouldn't you write LOCALE_NAME_MAX_LENGTH
not LOCALE_NAME_MAX_LENGTH - 1?)

I'm unexcited about the guc.c changes. There is visibly no bug
there. The only reason to change it would be if we were going
to introduce a strict project policy against using sprintf(),
which we're not likely to given that there are hundreds of other
occurrences in our code base. I don't see a reason to think
that these three are more pressing than the others.

regards, tom lane

Thank you. Tom. I agree that fixing the sprintf usage is not well-timed at the moment, so I’ve removed that change.
Regarding the use of wcsncpy with LOCALE_NAME_MAX_LENGTH - 1, it is a precaution in case the input string is not null-terminated.
Thanks again,
Haibo

________________________________
发件人: Tom Lane <tgl@sss.pgh.pa.us>
发送时间: 2025年6月16日 11:28
收件人: Yan Haibo <haibo.yan@hotmail.com>
抄送: Peter Eisentraut <peter@eisentraut.org>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
主题: Re: 回复: Fix potential overflow risks from wcscpy and sprintf

Yan Haibo <haibo.yan@hotmail.com> writes:

Thank you. Peter. It seems the patch may have been lost during our earlier communication, so I¡¯ve reattached it here.
I hope it comes through correctly this time.

Thanks for the patch.

Using wcsncpy in search_locale_enum() seems fine, assuming it exists
on Windows (note that code is Windows-only, possibly explaining why
we've not seen other static-analysis reports). I doubt there's any
actual bug there, since we're relying on Windows' own
LOCALE_NAME_MAX_LENGTH constant; but I agree the chain of reasoning
is kind of long. (But shouldn't you write LOCALE_NAME_MAX_LENGTH
not LOCALE_NAME_MAX_LENGTH - 1?)

I'm unexcited about the guc.c changes. There is visibly no bug
there. The only reason to change it would be if we were going
to introduce a strict project policy against using sprintf(),
which we're not likely to given that there are hundreds of other
occurrences in our code base. I don't see a reason to think
that these three are more pressing than the others.

regards, tom lane

Yan Haibo <haibo.yan@hotmail.com> writes:

Regarding the use of wcsncpy with LOCALE_NAME_MAX_LENGTH - 1, it is a precaution in case the input string is not null-terminated.

I don't think it's a "precaution". I think it's introducing a real
bug (that is, failure on a locale name of exactly the max allowed
length) to prevent a hypothetical bug.

regards, tom lane

Thank you, Tom. You’re absolutely right—this change is not necessary. I’ve updated the patch accordingly.
Best regards,
Haibo

________________________________
发件人: Tom Lane <tgl@sss.pgh.pa.us>
发送时间: 2025年6月16日 12:46
收件人: Yan Haibo <haibo.yan@hotmail.com>
抄送: Peter Eisentraut <peter@eisentraut.org>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
主题: Re: 回复: 回复: Fix potential overflow risks from wcscpy and sprintf

Yan Haibo <haibo.yan@hotmail.com> writes:

Regarding the use of wcsncpy with LOCALE_NAME_MAX_LENGTH - 1, it is a precaution in case the input string is not null-terminated.

I don't think it's a "precaution". I think it's introducing a real
bug (that is, failure on a locale name of exactly the max allowed
length) to prevent a hypothetical bug.

regards, tom lane