Date: Sun, 21 Jan 2024 16:13:20 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Abhijit Menon-Sen <ams@toroid.org>,
	Michael Paquier <michael@paquier.xyz>,
	Robert Haas <robertmhaas@gmail.com>, pgsql-hackers@postgresql.org
Subject: Re: introduce dynamic shared memory registry
Message-ID: <20240121221320.GA1682397@nathanxps13>
References: 
 <CALj2ACVaFuUys-Fkq8SQDpTOJnmaqO6C8RshbYtbW-DJD7LbAA@mail.gmail.com>
 <20240112172152.GA3803561@nathanxps13>
 <ZaF6UpYImGqVIhVp@toroid.org>
 <20240112194555.GA3804945@nathanxps13>
 <20240113214124.GA4162765@nathanxps13>
 <CALj2ACVKFPm7nCrHMko7JJpsFJU23abzYyMS8S2Pz8r4M=rW7w@mail.gmail.com>
 <20240116160748.GB100542@nathanxps13>
 <CALj2ACWuiAm7Rr13PFfaiQfOHUoCbJOJvU72jyLnj4xfoEOAEg@mail.gmail.com>
 <20240119204636.GA1178173@nathanxps13>
 <3348869.1705854106@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="ZPt4rx8FFjLCG7dd"
Content-Disposition: inline
In-Reply-To: <3348869.1705854106@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/20240121221320.GA1682397%40nathanxps13>
Precedence: bulk


--ZPt4rx8FFjLCG7dd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sun, Jan 21, 2024 at 11:21:46AM -0500, Tom Lane wrote:
> Coverity complained about this:
> 
> *** CID 1586660:  Null pointer dereferences  (NULL_RETURNS)
> /srv/coverity/git/pgsql-git/postgresql/src/backend/storage/ipc/dsm_registry.c: 185 in GetNamedDSMSegment()
> 179     	}
> 180     	else if (!dsm_find_mapping(entry->handle))
> 181     	{
> 182     		/* Attach to existing segment. */
> 183     		dsm_segment *seg = dsm_attach(entry->handle);
> 184     
>>>>     CID 1586660:  Null pointer dereferences  (NULL_RETURNS)
>>>>     Dereferencing a pointer that might be "NULL" "seg" when calling "dsm_pin_mapping".
> 185     		dsm_pin_mapping(seg);
> 186     		ret = dsm_segment_address(seg);
> 187     	}
> 188     	else
> 189     	{
> 190     		/* Return address of an already-attached segment. */
> 
> I think it's right --- the comments for dsm_attach explicitly
> point out that a NULL return is possible.  You need to handle
> that scenario in some way other than SIGSEGV.

Oops.  I've attached an attempt at fixing this.  I took the opportunity to
clean up the surrounding code a bit.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

--ZPt4rx8FFjLCG7dd
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0001-fix-coverity-complaint.patch"

From f4c1c7a7ce8bccf7251e384f895f34beb33f839e Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Sun, 21 Jan 2024 16:05:16 -0600
Subject: [PATCH v1 1/1] fix coverity complaint

---
 src/backend/storage/ipc/dsm_registry.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.c
index ac11f51375..c178173653 100644
--- a/src/backend/storage/ipc/dsm_registry.c
+++ b/src/backend/storage/ipc/dsm_registry.c
@@ -177,19 +177,22 @@ GetNamedDSMSegment(const char *name, size_t size,
 				(errmsg("requested DSM segment size does not match size of "
 						"existing segment")));
 	}
-	else if (!dsm_find_mapping(entry->handle))
+	else
 	{
-		/* Attach to existing segment. */
-		dsm_segment *seg = dsm_attach(entry->handle);
+		dsm_segment *seg = dsm_find_mapping(entry->handle);
+
+		/* If the existing segment is not already attached, attach it now. */
+		if (seg == NULL)
+		{
+			seg = dsm_attach(entry->handle);
+			if (seg == NULL)
+				elog(ERROR, "could not map dynamic shared memory segment");
+
+			dsm_pin_mapping(seg);
+		}
 
-		dsm_pin_mapping(seg);
 		ret = dsm_segment_address(seg);
 	}
-	else
-	{
-		/* Return address of an already-attached segment. */
-		ret = dsm_segment_address(dsm_find_mapping(entry->handle));
-	}
 
 	dshash_release_lock(dsm_registry_table, entry);
 	MemoryContextSwitchTo(oldcontext);
-- 
2.25.1


--ZPt4rx8FFjLCG7dd--