On 2014-04-30 17:39:27 +0200, Andres Freund wrote:
> Hi,
>
> Coverity flagged a couple of issues that seem to worth addressing by
> changing the code instead of ignoring them:
>
> 01) heap_xlog_update() looks to coverity as if it could trigger a NULL
> pointer dereference. That's because it thinks that oldtup.t_data is
> NULL if XLR_BKP_BLOCK(0) while reconstructing incremental
> tuples. That fortunately can't happen as incremental updates are
> only performed if the tuples are on the same page.
> Add an Assert().
> 02) Be a bit more consistent in what type to use for a size
> variable. Inconsequential, but more consistent.
> 03) Don't leak memory after connection aborts in pg_recvlogical.
> 04) Use a sensible parameter for memset() when randomizing memory in
> reorderbuffer. Inconsequential.
>
> Could somebody please pick these up?
That might be easier with actual patches...
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 67a566a57b66e1e47430f9b74fc82228e1c9130f Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Wed, 30 Apr 2014 17:18:55 +0200
Subject: [PATCH 1/4] Assert that pre/post-fix updated tuples are on the same
page during replay.
If they were not 'oldtup.t_data' would be dereferenced while set to
NULL in case of a full page image for block 0.
Do so primarily to silenence coverity; but also to make sure this
prerequisite isn't changed without adapting the replay routine as that
would appear to work in many cases.
---
src/backend/access/heap/heapam.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index a047632..336fbb0 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -8115,11 +8115,13 @@ newsame:;
if (xlrec->flags & XLOG_HEAP_PREFIX_FROM_OLD)
{
+ Assert(samepage);
memcpy(&prefixlen, recdata, sizeof(uint16));
recdata += sizeof(uint16);
}
if (xlrec->flags & XLOG_HEAP_SUFFIX_FROM_OLD)
{
+ Assert(samepage);
memcpy(&suffixlen, recdata, sizeof(uint16));
recdata += sizeof(uint16);
}
--
1.8.3.251.g1462b67
>From 43c3c46cf5b35296df2f1121940997cdd419eb4e Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Wed, 30 Apr 2014 17:27:06 +0200
Subject: [PATCH 2/4] Use Size instead of uint32 to measure ... size in
snapbuild.c.
Silences coverity and is more consistent with other functions in the
same file.
---
src/backend/replication/logical/snapbuild.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index c462e90..36034db 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1431,7 +1431,7 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn)
char path[MAXPGPATH];
int ret;
struct stat stat_buf;
- uint32 sz;
+ Size sz;
Assert(lsn != InvalidXLogRecPtr);
Assert(builder->last_serialized_snapshot == InvalidXLogRecPtr ||
--
1.8.3.251.g1462b67
>From c41faf9047e0d133b0cc233e6d7ca0be4e54e589 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Wed, 30 Apr 2014 17:29:04 +0200
Subject: [PATCH 3/4] Don't leak memory after connection aborts in
pg_recvlogical.
Noticed by coverity.
---
src/bin/pg_basebackup/pg_recvlogical.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 8141ba3..fe902cf 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -547,9 +547,6 @@ StreamLog(void)
}
PQclear(res);
- if (copybuf != NULL)
- PQfreemem(copybuf);
-
if (outfd != -1 && strcmp(outfile, "-") != 0)
{
int64 t = feGetCurrentTimestamp();
@@ -563,6 +560,11 @@ StreamLog(void)
}
outfd = -1;
error:
+ if (copybuf != NULL)
+ {
+ PQfreemem(copybuf);
+ copybuf = NULL;
+ }
destroyPQExpBuffer(query);
PQfinish(conn);
conn = NULL;
--
1.8.3.251.g1462b67
>From 84c8333f02abd3f973b3009d02dcbecc880ce909 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Wed, 30 Apr 2014 17:35:15 +0200
Subject: [PATCH 4/4] Pass sensible value to memset() when randomizing
reorderbuffer's tuple slab.
This is entirely harmless, but still wrong. Noticed by coverity.
---
src/backend/replication/logical/reorderbuffer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 4493930..0b21be9 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -463,7 +463,7 @@ ReorderBufferGetTupleBuf(ReorderBuffer *rb)
tuple = slist_container(ReorderBufferTupleBuf, node,
slist_pop_head_node(&rb->cached_tuplebufs));
#ifdef USE_ASSERT_CHECKING
- memset(tuple, 0xdeadbeef, sizeof(ReorderBufferTupleBuf));
+ memset(tuple, 0xa9, sizeof(ReorderBufferTupleBuf));
#endif
}
else
--
1.8.3.251.g1462b67
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
