[PATCH] Use DatumGetUInt32() for dsm_attach() in test_shm_mq_main()
Hi,
The attached patch fixes a minor type mismatch in `test_shm_mq_main()`.
The argument passed to `dsm_attach()` is expected to be a `uint32`, but the
code currently uses `DatumGetInt32()` to extract it from the `Datum`
argument. This can lead to incorrect behavior when the high bit is set, as
'unable to map dynamic shared memory segment'.
This patch changes it to use `DatumGetUInt32()` to match the expected type
and ensure correctness.
Thanks,
Jianghua Yang
Attachments:
0001-Fix-type-mismatch-in-dsm_attach-argument-by-using-Da.patchapplication/octet-stream; name=0001-Fix-type-mismatch-in-dsm_attach-argument-by-using-Da.patchDownload
From 734c2a28c905326bcf3e018fb9a984ca3b0f2e64 Mon Sep 17 00:00:00 2001
From: Jianghua Yang <yjhjstz@gmail.com>
Date: Thu, 26 Jun 2025 12:47:11 -0700
Subject: [PATCH] Fix type mismatch in dsm_attach() argument by using
DatumGetUInt32()
The argument passed to dsm_attach() represents a dynamic shared memory
segment handle, which is defined as a uint32. The existing code incorrectly
used DatumGetInt32(), which may lead to unexpected behavior if the value
exceeds the range of a signed 32-bit integer.
---
src/test/modules/test_shm_mq/worker.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/test/modules/test_shm_mq/worker.c b/src/test/modules/test_shm_mq/worker.c
index 96cd304dbbc..c1d321b69a4 100644
--- a/src/test/modules/test_shm_mq/worker.c
+++ b/src/test/modules/test_shm_mq/worker.c
@@ -77,7 +77,7 @@ test_shm_mq_main(Datum main_arg)
* exit, which is fine. If there were a ResourceOwner, it would acquire
* ownership of the mapping, but we have no need for that.
*/
- seg = dsm_attach(DatumGetInt32(main_arg));
+ seg = dsm_attach(DatumGetUInt32(main_arg));
if (seg == NULL)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
--
2.39.5 (Apple Git-154)
On Thu, Jun 26, 2025 at 12:51:10PM -0700, Jianghua Yang wrote:
The argument passed to `dsm_attach()` is expected to be a `uint32`, but the
code currently uses `DatumGetInt32()` to extract it from the `Datum`
argument. This can lead to incorrect behavior when the high bit is set, as
'unable to map dynamic shared memory segment'.
I'm not sure this actually causes any problems in practice because
dsm_attach() treats its argument as unsigned. In any case, I've never seen
this test fail like that, and presumably the high bit is sometimes set
because the handle is generated with a PRNG.
Nevertheless, I see no point in using the wrong macro. I'll plan on
committing/back-patching this shortly.
--
nathan
Hi,
Just to follow up — in our production system (pg_cron extension),
we’ve encountered real issues caused by passing a `Datum` to
`dsm_attach()` using `DatumGetInt32()` instead of `DatumGetUInt32()`.
Here's a sample of the errors observed in our logs:
ERROR: unable to map dynamic shared memory segment
WARNING: one or more background workers failed to start
These errors trace back to failures in `dsm_attach()`, where the
segment handle value was incorrectly interpreted due to sign extension
from `int32`.
The patch proposed earlier resolves this issue by correctly using
`DatumGetUInt32()`.
Thanks,
Jianghua yang
Nathan Bossart <nathandbossart@gmail.com> 于2025年6月26日周四 13:31写道：
Show quoted text
On Thu, Jun 26, 2025 at 12:51:10PM -0700, Jianghua Yang wrote:
The argument passed to `dsm_attach()` is expected to be a `uint32`, but
the
code currently uses `DatumGetInt32()` to extract it from the `Datum`
argument. This can lead to incorrect behavior when the high bit is set,as
'unable to map dynamic shared memory segment'.
I'm not sure this actually causes any problems in practice because
dsm_attach() treats its argument as unsigned. In any case, I've never seen
this test fail like that, and presumably the high bit is sometimes set
because the handle is generated with a PRNG.Nevertheless, I see no point in using the wrong macro. I'll plan on
committing/back-patching this shortly.--
nathan
On Thu, Jun 26, 2025 at 01:46:10PM -0700, Jianghua Yang wrote:
Just to follow up - in our production system (pg_cron extension),
we�ve encountered real issues caused by passing a `Datum` to
`dsm_attach()` using `DatumGetInt32()` instead of `DatumGetUInt32()`.Here's a sample of the errors observed in our logs:
ERROR: unable to map dynamic shared memory segment
WARNING: one or more background workers failed to startThese errors trace back to failures in `dsm_attach()`, where the
segment handle value was incorrectly interpreted due to sign extension
from `int32`.
I think there might be something else going on. I added a debug log in
test_shm_mq, and it looks like it regularly uses handles with the high bit
set. I also wrote a test program and consulted the C standard, which seem
to confirm that passing a signed integer to a function with an unsigned
parameter leaves the high bit set.
--
nathan
Nathan Bossart <nathandbossart@gmail.com> writes:
On Thu, Jun 26, 2025 at 01:46:10PM -0700, Jianghua Yang wrote:
These errors trace back to failures in `dsm_attach()`, where the
segment handle value was incorrectly interpreted due to sign extension
from `int32`.
I think there might be something else going on.
I agree with Nathan: the patch you proposed is purely cosmetic.
I don't object to it, but you need to dig deeper because this
will not resolve any actual behavioral problem.
regards, tom lane