Thread: [PATCH] Improve AtSubCommit_childXids

Hi,
Surely that "s->nChildXids > 0", protects s->childXids to be NULL!
But, when we exchange the test (s->nChildXids > 0) by (s->childXids != NULL), I believe we have the same protection,
because,if "s->childXids" is not NULL, "s->nChildXids" is > 0, naturally. 

That way we can improve the function and avoid calling and setting unnecessarily!

Bonus: silent compiler warning potential null pointer derenferencing.

Best regards,
Ranier Vilela

--- \dll\postgresql-12.0\a\backend\access\transam\xact.c    Mon Sep 30 17:06:55 2019
+++ xact.c    Wed Nov 13 13:03:28 2019
@@ -1580,20 +1580,20 @@
      */
     s->parent->childXids[s->parent->nChildXids] = XidFromFullTransactionId(s->fullTransactionId);

-    if (s->nChildXids > 0)
+    if (s->childXids != NULL) {
         memcpy(&s->parent->childXids[s->parent->nChildXids + 1],
                s->childXids,
                s->nChildXids * sizeof(TransactionId));
+        /* Release child's array to avoid leakage */
+        pfree(s->childXids);

+        /* We must reset these to avoid double-free if fail later in commit */
+        s->childXids = NULL;
+        s->nChildXids = 0;
+        s->maxChildXids = 0;
+    }
+    Assert(s->nChildXids == 0 && s->maxChildXids == 0);
     s->parent->nChildXids = new_nChildXids;
-
-    /* Release child's array to avoid leakage */
-    if (s->childXids != NULL)
-        pfree(s->childXids);
-    /* We must reset these to avoid double-free if fail later in commit */
-    s->childXids = NULL;
-    s->nChildXids = 0;
-    s->maxChildXids = 0;
 }

 /* ----------------------------------------------------------------

Hi,

On 2019-11-13 16:18:46 +0000, Ranier Vilela wrote:
> Surely that "s->nChildXids > 0", protects s->childXids to be NULL!
> But, when we exchange the test (s->nChildXids > 0) by (s->childXids != NULL), I believe we have the same protection,
because,if "s->childXids" is not NULL, "s->nChildXids" is > 0, naturally.
 
> 
> That way we can improve the function and avoid calling and setting unnecessarily!

Why is this an improvement? And what setting are we removing? You mean
that we reset nChildXids, even if it's already 0? Hard to see how that
matters.


> Bonus: silent compiler warning potential null pointer derenferencing.

Which compiler issues a warning here?

Greetings,

Andres Freund

"Why is this an improvement? And what setting are we removing? You mean
that we reset nChildXids, even if it's already 0? Hard to see how that
matters."

The orginal function, ever set ChildXidsm, nChildXidsa and maxChildXids.
See at lines 1594, 1595, 1596, even if it's already 0!

The test (nChildXids > 0), possibly works, but, may confuse when do use
memcpy function soon after, and access one pointer that below, is checked by NULL.
How hard to see this?

Original file:
    if (s->nChildXids > 0)
        memcpy(&s->parent->childXids[s->parent->nChildXids + 1],
               s->childXids, // s->childXids null pointer potential dereferencing
               s->nChildXids * sizeof(TransactionId));

    s->parent->nChildXids = new_nChildXids;

    /* Release child's array to avoid leakage */
    if (s->childXids != NULL) // Check null pointer!
        pfree(s->childXids);
    /* We must reset these to avoid double-free if fail later in commit */
    s->childXids = NULL; // ever set to 0
    s->nChildXids = 0;  // ever set to 0
    s->maxChildXids = 0; // ever set to 0

best regards,
Ranier Vilela
________________________________________
De: Andres Freund <andres@anarazel.de>
Enviado: quarta-feira, 13 de novembro de 2019 17:10
Para: Ranier Vilela
Cc: PostgreSQL Hackers
Assunto: Re: [PATCH] Improve AtSubCommit_childXids

Hi,

On 2019-11-13 16:18:46 +0000, Ranier Vilela wrote:
> Surely that "s->nChildXids > 0", protects s->childXids to be NULL!
> But, when we exchange the test (s->nChildXids > 0) by (s->childXids != NULL), I believe we have the same protection,
because,if "s->childXids" is not NULL, "s->nChildXids" is > 0, naturally. 
>
> That way we can improve the function and avoid calling and setting unnecessarily!

Why is this an improvement? And what setting are we removing? You mean
that we reset nChildXids, even if it's already 0? Hard to see how that
matters.


> Bonus: silent compiler warning potential null pointer derenferencing.

Which compiler issues a warning here?

Greetings,

Andres Freund

Hi,

On this list we quote inline, and trim quoted messages to the relevant
parts...

On 2019-11-13 17:40:27 +0000, Ranier Vilela wrote:
> "Why is this an improvement? And what setting are we removing? You mean
> that we reset nChildXids, even if it's already 0? Hard to see how that
> matters."
> 
> The orginal function, ever set ChildXidsm, nChildXidsa and maxChildXids.
> See at lines 1594, 1595, 1596, even if it's already 0!

So? It's easier to reason about that way anyway, and it's just about
free, because the cacheline is already touched.


> The test (nChildXids > 0), possibly works, but, may confuse when do use
> memcpy function soon after, and access one pointer that below, is checked by NULL.
> How hard to see this?

But they don't necessarily have to mean the same. One is about the array
being allocated, and one is about the number of actual xids in
there. The memcpy cares about the number of xids in it. The free cares
about whether memory is allocated.

Greetings,

Andres Freund