Fix potential overflow risks from wcscpy and sprintf
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.
Thank you for your time and support.
Best regards,
Haibo
Attachments:
0001-Mitigate-potential-overflow-risks-from-wcscpy-and-sp.patchapplication/octet-stream; name=0001-Mitigate-potential-overflow-risks-from-wcscpy-and-sp.patchDownload
From b40cdd229902b40a6f4bb09177996ce995af8525 Mon Sep 17 00:00:00 2001
From: Haibo Yan <haibo.yan@apple.com>
Date: Fri, 6 Jun 2025 12:39:13 -0700
Subject: [PATCH] Mitigate potential overflow risks from wcscpy and sprintf
The use of wcscpy and sprintf for copying user-supplied input into buffers
is inherently unsafe and can lead to buffer overflows. This commit replaces
wcscpy with wcsncpy and sprintf with snprintf to ensure proper bounds
checking and mitigate potential overflow vulnerabilities.
---
src/backend/utils/adt/pg_locale.c | 4 ++--
src/backend/utils/misc/guc.c | 18 +++++++++---------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index f5e31c433a0..e5c64d81de3 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -929,7 +929,7 @@ search_locale_enum(LPWSTR pStr, DWORD dwFlags, LPARAM lparam)
{
if (_wcsicmp(argv[0], test_locale) == 0)
{
- wcscpy(argv[1], pStr);
+ wcsncpy(argv[1], pStr, LOCALE_NAME_MAX_LENGTH - 1);
*argv[2] = (wchar_t) 1;
return FALSE;
}
@@ -952,7 +952,7 @@ search_locale_enum(LPWSTR pStr, DWORD dwFlags, LPARAM lparam)
{
if (_wcsicmp(argv[0], test_locale) == 0)
{
- wcscpy(argv[1], pStr);
+ wcsncpy(argv[1], pStr, LOCALE_NAME_MAX_LENGTH - 1);
*argv[2] = (wchar_t) 1;
return FALSE;
}
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 667df448732..927c3e52ee2 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1818,9 +1818,9 @@ SelectConfigFiles(const char *userDoption, const char *progname)
}
else if (configdir)
{
- fname = guc_malloc(FATAL,
- strlen(configdir) + strlen(CONFIG_FILENAME) + 2);
- sprintf(fname, "%s/%s", configdir, CONFIG_FILENAME);
+ size_t len = strlen(configdir) + strlen(CONFIG_FILENAME) + 2;
+ fname = guc_malloc(FATAL, len);
+ snprintf(fname, len, "%s/%s", configdir, CONFIG_FILENAME);
fname_is_malloced = false;
}
else
@@ -1921,9 +1921,9 @@ SelectConfigFiles(const char *userDoption, const char *progname)
}
else if (configdir)
{
- fname = guc_malloc(FATAL,
- strlen(configdir) + strlen(HBA_FILENAME) + 2);
- sprintf(fname, "%s/%s", configdir, HBA_FILENAME);
+ size_t len = strlen(configdir) + strlen(HBA_FILENAME) + 2;
+ fname = guc_malloc(FATAL, len);
+ snprintf(fname, len, "%s/%s", configdir, HBA_FILENAME);
fname_is_malloced = false;
}
else
@@ -1952,9 +1952,9 @@ SelectConfigFiles(const char *userDoption, const char *progname)
}
else if (configdir)
{
- fname = guc_malloc(FATAL,
- strlen(configdir) + strlen(IDENT_FILENAME) + 2);
- sprintf(fname, "%s/%s", configdir, IDENT_FILENAME);
+ size_t len = strlen(configdir) + strlen(IDENT_FILENAME) + 2;
+ fname = guc_malloc(FATAL, len);
+ snprintf(fname, len, "%s/%s", configdir, IDENT_FILENAME);
fname_is_malloced = false;
}
else
--
2.49.0
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.
Attachments:
0001-Mitigate-potential-overflow-risks-from-wcscpy-and-sp.patchapplication/octet-stream; name=0001-Mitigate-potential-overflow-risks-from-wcscpy-and-sp.patchDownload
From b40cdd229902b40a6f4bb09177996ce995af8525 Mon Sep 17 00:00:00 2001
From: Haibo Yan <haibo.yan@apple.com>
Date: Fri, 6 Jun 2025 12:39:13 -0700
Subject: [PATCH] Mitigate potential overflow risks from wcscpy and sprintf
The use of wcscpy and sprintf for copying user-supplied input into buffers
is inherently unsafe and can lead to buffer overflows. This commit replaces
wcscpy with wcsncpy and sprintf with snprintf to ensure proper bounds
checking and mitigate potential overflow vulnerabilities.
---
src/backend/utils/adt/pg_locale.c | 4 ++--
src/backend/utils/misc/guc.c | 18 +++++++++---------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index f5e31c433a0..e5c64d81de3 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -929,7 +929,7 @@ search_locale_enum(LPWSTR pStr, DWORD dwFlags, LPARAM lparam)
{
if (_wcsicmp(argv[0], test_locale) == 0)
{
- wcscpy(argv[1], pStr);
+ wcsncpy(argv[1], pStr, LOCALE_NAME_MAX_LENGTH - 1);
*argv[2] = (wchar_t) 1;
return FALSE;
}
@@ -952,7 +952,7 @@ search_locale_enum(LPWSTR pStr, DWORD dwFlags, LPARAM lparam)
{
if (_wcsicmp(argv[0], test_locale) == 0)
{
- wcscpy(argv[1], pStr);
+ wcsncpy(argv[1], pStr, LOCALE_NAME_MAX_LENGTH - 1);
*argv[2] = (wchar_t) 1;
return FALSE;
}
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 667df448732..927c3e52ee2 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1818,9 +1818,9 @@ SelectConfigFiles(const char *userDoption, const char *progname)
}
else if (configdir)
{
- fname = guc_malloc(FATAL,
- strlen(configdir) + strlen(CONFIG_FILENAME) + 2);
- sprintf(fname, "%s/%s", configdir, CONFIG_FILENAME);
+ size_t len = strlen(configdir) + strlen(CONFIG_FILENAME) + 2;
+ fname = guc_malloc(FATAL, len);
+ snprintf(fname, len, "%s/%s", configdir, CONFIG_FILENAME);
fname_is_malloced = false;
}
else
@@ -1921,9 +1921,9 @@ SelectConfigFiles(const char *userDoption, const char *progname)
}
else if (configdir)
{
- fname = guc_malloc(FATAL,
- strlen(configdir) + strlen(HBA_FILENAME) + 2);
- sprintf(fname, "%s/%s", configdir, HBA_FILENAME);
+ size_t len = strlen(configdir) + strlen(HBA_FILENAME) + 2;
+ fname = guc_malloc(FATAL, len);
+ snprintf(fname, len, "%s/%s", configdir, HBA_FILENAME);
fname_is_malloced = false;
}
else
@@ -1952,9 +1952,9 @@ SelectConfigFiles(const char *userDoption, const char *progname)
}
else if (configdir)
{
- fname = guc_malloc(FATAL,
- strlen(configdir) + strlen(IDENT_FILENAME) + 2);
- sprintf(fname, "%s/%s", configdir, IDENT_FILENAME);
+ size_t len = strlen(configdir) + strlen(IDENT_FILENAME) + 2;
+ fname = guc_malloc(FATAL, len);
+ snprintf(fname, len, "%s/%s", configdir, IDENT_FILENAME);
fname_is_malloced = false;
}
else
--
2.49.0
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.
Attachments:
0001-Mitigate-potential-overflow-risks-from-wcscpy-and-sp.patchapplication/octet-stream; name=0001-Mitigate-potential-overflow-risks-from-wcscpy-and-sp.patchDownload
From b40cdd229902b40a6f4bb09177996ce995af8525 Mon Sep 17 00:00:00 2001
From: Haibo Yan <haibo.yan@apple.com>
Date: Fri, 6 Jun 2025 12:39:13 -0700
Subject: [PATCH] Mitigate potential overflow risks from wcscpy and sprintf
The use of wcscpy and sprintf for copying user-supplied input into buffers
is inherently unsafe and can lead to buffer overflows. This commit replaces
wcscpy with wcsncpy and sprintf with snprintf to ensure proper bounds
checking and mitigate potential overflow vulnerabilities.
---
src/backend/utils/adt/pg_locale.c | 4 ++--
src/backend/utils/misc/guc.c | 18 +++++++++---------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index f5e31c433a0..e5c64d81de3 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -929,7 +929,7 @@ search_locale_enum(LPWSTR pStr, DWORD dwFlags, LPARAM lparam)
{
if (_wcsicmp(argv[0], test_locale) == 0)
{
- wcscpy(argv[1], pStr);
+ wcsncpy(argv[1], pStr, LOCALE_NAME_MAX_LENGTH - 1);
*argv[2] = (wchar_t) 1;
return FALSE;
}
@@ -952,7 +952,7 @@ search_locale_enum(LPWSTR pStr, DWORD dwFlags, LPARAM lparam)
{
if (_wcsicmp(argv[0], test_locale) == 0)
{
- wcscpy(argv[1], pStr);
+ wcsncpy(argv[1], pStr, LOCALE_NAME_MAX_LENGTH - 1);
*argv[2] = (wchar_t) 1;
return FALSE;
}
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 667df448732..927c3e52ee2 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1818,9 +1818,9 @@ SelectConfigFiles(const char *userDoption, const char *progname)
}
else if (configdir)
{
- fname = guc_malloc(FATAL,
- strlen(configdir) + strlen(CONFIG_FILENAME) + 2);
- sprintf(fname, "%s/%s", configdir, CONFIG_FILENAME);
+ size_t len = strlen(configdir) + strlen(CONFIG_FILENAME) + 2;
+ fname = guc_malloc(FATAL, len);
+ snprintf(fname, len, "%s/%s", configdir, CONFIG_FILENAME);
fname_is_malloced = false;
}
else
@@ -1921,9 +1921,9 @@ SelectConfigFiles(const char *userDoption, const char *progname)
}
else if (configdir)
{
- fname = guc_malloc(FATAL,
- strlen(configdir) + strlen(HBA_FILENAME) + 2);
- sprintf(fname, "%s/%s", configdir, HBA_FILENAME);
+ size_t len = strlen(configdir) + strlen(HBA_FILENAME) + 2;
+ fname = guc_malloc(FATAL, len);
+ snprintf(fname, len, "%s/%s", configdir, HBA_FILENAME);
fname_is_malloced = false;
}
else
@@ -1952,9 +1952,9 @@ SelectConfigFiles(const char *userDoption, const char *progname)
}
else if (configdir)
{
- fname = guc_malloc(FATAL,
- strlen(configdir) + strlen(IDENT_FILENAME) + 2);
- sprintf(fname, "%s/%s", configdir, IDENT_FILENAME);
+ size_t len = strlen(configdir) + strlen(IDENT_FILENAME) + 2;
+ fname = guc_malloc(FATAL, len);
+ snprintf(fname, len, "%s/%s", configdir, IDENT_FILENAME);
fname_is_malloced = false;
}
else
--
2.49.0
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
Attachments:
0001-Mitigate-potential-overflow-risks-from-wcscpy.patchapplication/octet-stream; name=0001-Mitigate-potential-overflow-risks-from-wcscpy.patchDownload
From bc311905d21de00d04556a13cdcc3ae2b13c846d Mon Sep 17 00:00:00 2001
From: Haibo Yan <haibo.yan@apple.com>
Date: Mon, 16 Jun 2025 12:14:10 -0700
Subject: [PATCH] Mitigate potential overflow risks from wcscpy
The use of wcscpy for copying user-supplied input into buffers is
inherently unsafe and can lead to buffer overflows. This commit
replaces wcscpy with wcsncpy to ensure proper bounds checking and
mitigate potential overflow vulnerabilities.
---
src/backend/utils/adt/pg_locale.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index a858f27cadc..31eff4e3477 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -928,7 +928,7 @@ search_locale_enum(LPWSTR pStr, DWORD dwFlags, LPARAM lparam)
{
if (_wcsicmp(argv[0], test_locale) == 0)
{
- wcscpy(argv[1], pStr);
+ wcsncpy(argv[1], pStr, LOCALE_NAME_MAX_LENGTH - 1);
*argv[2] = (wchar_t) 1;
return FALSE;
}
@@ -951,7 +951,7 @@ search_locale_enum(LPWSTR pStr, DWORD dwFlags, LPARAM lparam)
{
if (_wcsicmp(argv[0], test_locale) == 0)
{
- wcscpy(argv[1], pStr);
+ wcsncpy(argv[1], pStr, LOCALE_NAME_MAX_LENGTH - 1);
*argv[2] = (wchar_t) 1;
return FALSE;
}
--
2.49.0
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
Attachments:
0001-Mitigate-potential-overflow-risks-from-wcscpy.patchapplication/octet-stream; name=0001-Mitigate-potential-overflow-risks-from-wcscpy.patchDownload
From 3b018a1e75c42fe79474c540863190c37c24db3f Mon Sep 17 00:00:00 2001
From: Haibo Yan <haibo.yan@apple.com>
Date: Mon, 16 Jun 2025 20:07:12 -0700
Subject: [PATCH] Mitigate potential overflow risks from wcscpy
The use of wcscpy for copying user-supplied input into buffers is
inherently unsafe and can lead to buffer overflows. This commit
replaces wcscpy with wcsncpy to ensure proper bounds checking and
mitigate potential overflow vulnerabilities.
---
src/backend/utils/adt/pg_locale.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index a858f27cadc..13670e3918b 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -928,7 +928,7 @@ search_locale_enum(LPWSTR pStr, DWORD dwFlags, LPARAM lparam)
{
if (_wcsicmp(argv[0], test_locale) == 0)
{
- wcscpy(argv[1], pStr);
+ wcsncpy(argv[1], pStr, LOCALE_NAME_MAX_LENGTH);
*argv[2] = (wchar_t) 1;
return FALSE;
}
@@ -951,7 +951,7 @@ search_locale_enum(LPWSTR pStr, DWORD dwFlags, LPARAM lparam)
{
if (_wcsicmp(argv[0], test_locale) == 0)
{
- wcscpy(argv[1], pStr);
+ wcsncpy(argv[1], pStr, LOCALE_NAME_MAX_LENGTH);
*argv[2] = (wchar_t) 1;
return FALSE;
}
--
2.49.0