Integer undeflow in fprintf in dsa.c
Hello hackers,
Using Svace* I think I've found a little bug in src/backend/utils/mmgr/dsa.c.
This bug is presented in REL_12_STABLE, REL_13_STABLE, REL_14_STABLE,
REL_15_STABLE, REL_16_STABLE and master. I see that it was introduced together
with dynamic shared memory areas in the commit 13df76a537cca3b8884911d8fdf7c89a457a8dd3.
I also see that at least two people have encountered this fprintf output.
(https://postgrespro.com/list/thread-id/2419512,
/messages/by-id/15e9501170d.e4b5a3858707.3339083113985275726@zohocorp.com)
fprintf(stderr,
" segment bin %zu (at least %d contiguous pages free):\n",
i, 1 << (i - 1));
In case i equals zero user will get "at least -2147483648 contiguous pages free".
I believe that this is a mistake, and fprintf should print "at least 0 contiguous pages free"
in case i equals zero.
The patch that has a fix of this is attached.
* - https://svace.pages.ispras.ru/svace-website/en/
Kind regards,
Ian Ilyasov.
Juniour Software Developer at Postgres Professional
Attachments:
Integer_underflow_fix_in_fprintf_in_dsa_c_.patchtext/x-patch; name=Integer_underflow_fix_in_fprintf_in_dsa_c_.patchDownload
Subject: [PATCH] Integer underflow fix in fprintf in dsa.c.
---
Index: src/backend/utils/mmgr/dsa.c
<+>UTF-8
===================================================================
diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c
--- a/src/backend/utils/mmgr/dsa.c (revision b78fa8547d02fc72ace679fb4d5289dccdbfc781)
+++ b/src/backend/utils/mmgr/dsa.c (date 1708426298001)
@@ -1107,7 +1107,7 @@
fprintf(stderr,
" segment bin %zu (at least %d contiguous pages free):\n",
- i, 1 << (i - 1));
+ i, i != 0 ? 1 << (i - 1) : 0);
segment_index = area->control->segment_bins[i];
while (segment_index != DSA_SEGMENT_INDEX_NONE)
{
On 20 Feb 2024, at 12:28, Ильясов Ян <ianilyasov@outlook.com> wrote:
fprintf(stderr,
" segment bin %zu (at least %d contiguous pages free):\n",
i, 1 << (i - 1));In case i equals zero user will get "at least -2147483648 contiguous pages free".
That does indeed seem like an oversight.
I believe that this is a mistake, and fprintf should print "at least 0 contiguous pages free"
in case i equals zero.
The message "at least 0 contiguous pages free" reads a bit nonsensical though,
wouldn't it be preferrable to check for i being zero and print a custom message
for that case? Something like the below untested sketch?
+ if (i == 0)
+ fprintf(stderr,
+ " segment bin %zu (no contiguous free pages):\n", i);
+ else
+ fprintf(stderr,
+ " segment bin %zu (at least %d contiguous pages free):\n",
+ i, 1 << (i - 1));--
Daniel Gustafsson
On Tue, Feb 20, 2024 at 5:30 PM Daniel Gustafsson <daniel@yesql.se> wrote:
The message "at least 0 contiguous pages free" reads a bit nonsensical though,
wouldn't it be preferrable to check for i being zero and print a custom message
for that case? Something like the below untested sketch?+ if (i == 0) + fprintf(stderr, + " segment bin %zu (no contiguous free pages):\n", i); + else + fprintf(stderr, + " segment bin %zu (at least %d contiguous pages free):\n", + i, 1 << (i - 1));
That does seem reasonable. However, this is just debugging code, so it
also probably isn't necessary to sweat anything too much.
--
Robert Haas
EDB: http://www.enterprisedb.com
Sorry for not answering quickly.
Thank you for your comments.
I attached a patch to the letter with changes to take into account Daniel Gustafsson's comment.
Kind regards,
Ian Ilyasov.
Juniour Software Developer at Postgres Professional
Attachments:
Integer_underflow_fix_in_fprintf_in_dsa.patchtext/x-patch; name=Integer_underflow_fix_in_fprintf_in_dsa.patchDownload
Subject: [PATCH] Integer underflow fix in fprintf in dsa.c.
---
Index: src/backend/utils/mmgr/dsa.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c
--- a/src/backend/utils/mmgr/dsa.c (revision b78fa8547d02fc72ace679fb4d5289dccdbfc781)
+++ b/src/backend/utils/mmgr/dsa.c (date 1708440995707)
@@ -1105,9 +1105,13 @@
{
dsa_segment_index segment_index;
- fprintf(stderr,
- " segment bin %zu (at least %d contiguous pages free):\n",
- i, 1 << (i - 1));
+ if (i == 0)
+ fprintf(stderr,
+ " segment bin %zu (no contiguous free pages):\n", i);
+ else
+ fprintf(stderr,
+ " segment bin %zu (at least %d contiguous pages free):\n",
+ i, 1 << (i - 1));
segment_index = area->control->segment_bins[i];
while (segment_index != DSA_SEGMENT_INDEX_NONE)
{
On 20 Feb 2024, at 17:13, Ilyasov Ian <ianilyasov@outlook.com> wrote:
Sorry for not answering quickly.
There is no need for any apology, there is no obligation to answer within any
specific timeframe.
I attached a patch to the letter with changes to take into account Daniel Gustafsson's comment.
Looks good on a quick skim, I'll take care of this shortly.
--
Daniel Gustafsson