Fix condition in shm_toc and remove unused function shm_toc_freespace.
Hi, hackers
Some conditions in shm_toc_insert and shm_toc_allocate are bogus, like:
if (toc_bytes + nbytes > total_bytes || toc_bytes + nbytes < toc_bytes)
Remove the condition `toc_bytes + nbytes < toc_bytes` and take a sizeof(shm_entry) into account in shm_toc_allocate though
shm_toc_allocate does that too.
/* Check for memory exhaustion and overflow. */
- if (toc_bytes + nbytes > total_bytes || toc_bytes + nbytes < toc_bytes)
+ if (toc_bytes + sizeof(shm_toc_entry) + nbytes > total_bytes)
{
SpinLockRelease(&toc->toc_mutex);shm_toc_freespace is introduced with shm_toc by original commit 6ddd5137b2, but is not used since then, so remove it.
Regards,
Zhang Mingli
Attachments:
v0-0001-Fix-condition-in-shm_toc-and-remove-unused-functi.patchapplication/octet-streamDownload
From 8d127504d0b4d9a909add9b235f36d40b3ce7f6a Mon Sep 17 00:00:00 2001
From: Zhang Mingli <avamingli@gmail.com>
Date: Thu, 12 Jan 2023 14:11:43 +0800
Subject: [PATCH v0] Fix condition in shm_toc and remove unused function
shm_toc_freespace.
Fix bogus condition in shm_toc_insert and shm_toc_allocate.
Take a sizeof(shm_entry) into account when shm_toc_allocate.
Remove unused function shm_toc_freespace.
---
src/backend/storage/ipc/shm_toc.c | 26 +-------------------------
src/include/storage/shm_toc.h | 1 -
2 files changed, 1 insertion(+), 26 deletions(-)
diff --git a/src/backend/storage/ipc/shm_toc.c b/src/backend/storage/ipc/shm_toc.c
index 0cd824400f..37bd0dd25f 100644
--- a/src/backend/storage/ipc/shm_toc.c
+++ b/src/backend/storage/ipc/shm_toc.c
@@ -110,7 +110,7 @@ shm_toc_allocate(shm_toc *toc, Size nbytes)
+ allocated_bytes;
/* Check for memory exhaustion and overflow. */
- if (toc_bytes + nbytes > total_bytes || toc_bytes + nbytes < toc_bytes)
+ if (toc_bytes + sizeof(shm_toc_entry) + nbytes > total_bytes)
{
SpinLockRelease(&toc->toc_mutex);
ereport(ERROR,
@@ -124,29 +124,6 @@ shm_toc_allocate(shm_toc *toc, Size nbytes)
return ((char *) toc) + (total_bytes - allocated_bytes - nbytes);
}
-/*
- * Return the number of bytes that can still be allocated.
- */
-Size
-shm_toc_freespace(shm_toc *toc)
-{
- volatile shm_toc *vtoc = toc;
- Size total_bytes;
- Size allocated_bytes;
- Size nentry;
- Size toc_bytes;
-
- SpinLockAcquire(&toc->toc_mutex);
- total_bytes = vtoc->toc_total_bytes;
- allocated_bytes = vtoc->toc_allocated_bytes;
- nentry = vtoc->toc_nentry;
- SpinLockRelease(&toc->toc_mutex);
-
- toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry);
- Assert(allocated_bytes + BUFFERALIGN(toc_bytes) <= total_bytes);
- return total_bytes - (allocated_bytes + BUFFERALIGN(toc_bytes));
-}
-
/*
* Insert a TOC entry.
*
@@ -191,7 +168,6 @@ shm_toc_insert(shm_toc *toc, uint64 key, void *address)
/* Check for memory exhaustion and overflow. */
if (toc_bytes + sizeof(shm_toc_entry) > total_bytes ||
- toc_bytes + sizeof(shm_toc_entry) < toc_bytes ||
nentry >= PG_UINT32_MAX)
{
SpinLockRelease(&toc->toc_mutex);
diff --git a/src/include/storage/shm_toc.h b/src/include/storage/shm_toc.h
index 7a2f8e9934..5f2abcc613 100644
--- a/src/include/storage/shm_toc.h
+++ b/src/include/storage/shm_toc.h
@@ -30,7 +30,6 @@ typedef struct shm_toc shm_toc;
extern shm_toc *shm_toc_create(uint64 magic, void *address, Size nbytes);
extern shm_toc *shm_toc_attach(uint64 magic, void *address);
extern void *shm_toc_allocate(shm_toc *toc, Size nbytes);
-extern Size shm_toc_freespace(shm_toc *toc);
extern void shm_toc_insert(shm_toc *toc, uint64 key, void *address);
extern void *shm_toc_lookup(shm_toc *toc, uint64 key, bool noError);
--
2.34.1
Import Notes
Reply to msg id not found: f937934f-5b6c-402c-a9ab-605bd07dcba9@SparkReference msg id not found: f937934f-5b6c-402c-a9ab-605bd07dcba9@Spark
Hi,
On Jan 12, 2023, 14:34 +0800, Zhang Mingli <zmlpostgres@gmail.com>, wrote:
Hi, hackers
Some conditions in shm_toc_insert and shm_toc_allocate are bogus, like:
if (toc_bytes + nbytes > total_bytes || toc_bytes + nbytes < toc_bytes)
Remove the condition `toc_bytes + nbytes < toc_bytes` and take a sizeof(shm_entry) into account in shm_toc_allocate though
shm_toc_allocate does that too.
shm_toc_insert does that too, and we can report error earlier.
Regards,
Zhang Mingli
On Thu, Jan 12, 2023 at 2:50 PM Zhang Mingli <zmlpostgres@gmail.com> wrote:
On Jan 12, 2023, 14:34 +0800, Zhang Mingli <zmlpostgres@gmail.com>, wrote:
Some conditions in shm_toc_insert and shm_toc_allocate are bogus, like:
if (toc_bytes + nbytes > total_bytes || toc_bytes + nbytes < toc_bytes)
Remove the condition `toc_bytes + nbytes < toc_bytes` and take a
sizeof(shm_entry) into account in shm_toc_allocate though
shm_toc_allocate does that too.shm_toc_insert does that too, and we can report error earlier.
I don't think we should consider sizeof(shm_toc_entry) in the 'if'
condition in shm_toc_allocate, as this function is not in charge of
allocating a new TOC entry. That's what shm_toc_insert does.
Other parts of this patch look good to me.
Thanks
Richard
Hi,
Regards,
Zhang Mingli
On Jan 12, 2023, 16:54 +0800, Richard Guo <guofenglinux@gmail.com>, wrote:
On Thu, Jan 12, 2023 at 2:50 PM Zhang Mingli <zmlpostgres@gmail.com> wrote:
On Jan 12, 2023, 14:34 +0800, Zhang Mingli <zmlpostgres@gmail.com>, wrote:
Some conditions in shm_toc_insert and shm_toc_allocate are bogus, like:
if (toc_bytes + nbytes > total_bytes || toc_bytes + nbytes < toc_bytes)Remove the condition `toc_bytes + nbytes < toc_bytes` and take a sizeof(shm_entry) into account in shm_toc_allocate though
shm_toc_allocate does that too.shm_toc_insert does that too, and we can report error earlier.
I don't think we should consider sizeof(shm_toc_entry) in the 'if'
condition in shm_toc_allocate, as this function is not in charge of
allocating a new TOC entry. That's what shm_toc_insert does.
Thanks for review.
Make sense.
Even reserve a sizeof(shm_toc_entry) when shm_toc_allocate, it cloud happen that there is no space when shm_toc_insert
in case of other processes may take space after that.
Patch updated.
Attachments:
v1-0001-Fix-condition-in-shm_toc-and-remove-unused-functi.patchapplication/octet-streamDownload
From 766d20b666b75ef74767d2e9e3cf3c872c4e7521 Mon Sep 17 00:00:00 2001
From: Zhang Mingli <avamingli@gmail.com>
Date: Thu, 12 Jan 2023 14:11:43 +0800
Subject: [PATCH v1] Fix condition in shm_toc and remove unused function
shm_toc_freespace.
Fix bogus condition in shm_toc_insert and shm_toc_allocate.
Remove unused function shm_toc_freespace.
---
src/backend/storage/ipc/shm_toc.c | 26 +-------------------------
src/include/storage/shm_toc.h | 1 -
2 files changed, 1 insertion(+), 26 deletions(-)
diff --git a/src/backend/storage/ipc/shm_toc.c b/src/backend/storage/ipc/shm_toc.c
index 0cd824400f..ed16947d4c 100644
--- a/src/backend/storage/ipc/shm_toc.c
+++ b/src/backend/storage/ipc/shm_toc.c
@@ -110,7 +110,7 @@ shm_toc_allocate(shm_toc *toc, Size nbytes)
+ allocated_bytes;
/* Check for memory exhaustion and overflow. */
- if (toc_bytes + nbytes > total_bytes || toc_bytes + nbytes < toc_bytes)
+ if (toc_bytes + nbytes > total_bytes)
{
SpinLockRelease(&toc->toc_mutex);
ereport(ERROR,
@@ -124,29 +124,6 @@ shm_toc_allocate(shm_toc *toc, Size nbytes)
return ((char *) toc) + (total_bytes - allocated_bytes - nbytes);
}
-/*
- * Return the number of bytes that can still be allocated.
- */
-Size
-shm_toc_freespace(shm_toc *toc)
-{
- volatile shm_toc *vtoc = toc;
- Size total_bytes;
- Size allocated_bytes;
- Size nentry;
- Size toc_bytes;
-
- SpinLockAcquire(&toc->toc_mutex);
- total_bytes = vtoc->toc_total_bytes;
- allocated_bytes = vtoc->toc_allocated_bytes;
- nentry = vtoc->toc_nentry;
- SpinLockRelease(&toc->toc_mutex);
-
- toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry);
- Assert(allocated_bytes + BUFFERALIGN(toc_bytes) <= total_bytes);
- return total_bytes - (allocated_bytes + BUFFERALIGN(toc_bytes));
-}
-
/*
* Insert a TOC entry.
*
@@ -191,7 +168,6 @@ shm_toc_insert(shm_toc *toc, uint64 key, void *address)
/* Check for memory exhaustion and overflow. */
if (toc_bytes + sizeof(shm_toc_entry) > total_bytes ||
- toc_bytes + sizeof(shm_toc_entry) < toc_bytes ||
nentry >= PG_UINT32_MAX)
{
SpinLockRelease(&toc->toc_mutex);
diff --git a/src/include/storage/shm_toc.h b/src/include/storage/shm_toc.h
index 7a2f8e9934..5f2abcc613 100644
--- a/src/include/storage/shm_toc.h
+++ b/src/include/storage/shm_toc.h
@@ -30,7 +30,6 @@ typedef struct shm_toc shm_toc;
extern shm_toc *shm_toc_create(uint64 magic, void *address, Size nbytes);
extern shm_toc *shm_toc_attach(uint64 magic, void *address);
extern void *shm_toc_allocate(shm_toc *toc, Size nbytes);
-extern Size shm_toc_freespace(shm_toc *toc);
extern void shm_toc_insert(shm_toc *toc, uint64 key, void *address);
extern void *shm_toc_lookup(shm_toc *toc, uint64 key, bool noError);
--
2.34.1