From: Matthew Fernandez <matthew.fernandez@gmail.com>
Date: Thu, 24 Nov 2022 21:32:58 +0000 (-0800)
Subject: ortho init_query_structure: separate assignment statements
X-Git-Tag: 7.0.4~2^2~10
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b98683cf6e1c45fedf6a39f9fcf24691383eec1a;p=graphviz

ortho init_query_structure: separate assignment statements

This looks like a pure readability change, but is actually preparation for an
upcoming commit. A future commit alters `newnode` to reallocate `qs->data`.
Without the current change, there is no sequence point between the index into
`qs->data` at the start of these lines and the reallocation (in future)
performed by the function call at the end of these lines. This is undefined
behavior with respect to the C standard.

This is quite a subtle problem. I initially did not spot it and was only alerted
by failures under CentOS 7, which encountered nodes with an invalid `nodetype`.
This problem was not reproducible on any other platform, even under ASan and
UBSan. What I believe was going on here is that the combination of the C
compiler and the Glibc on CentOS 7 is the only environment we have which
accidentally leveraged this undefined behavior, by sequencing these operations
as indexing into `qs->data` and _then_ performing the reallocation, resulting in
a write to invalid heap memory.

Gitlab: #56
---

diff --git a/lib/ortho/trapezoid.c b/lib/ortho/trapezoid.c
index 2716075a4..370018982 100644
--- a/lib/ortho/trapezoid.c
+++ b/lib/ortho/trapezoid.c
@@ -151,7 +151,7 @@ static bool _less_than (pointf *v0, pointf *v1)
 
 static int
 init_query_structure(int segnum, segment_t *seg, traps_t *tr, qnodes_t *qs) {
-  int i1, i2, i3, i4, i5, i6, i7, root;
+  int i1, root;
   int t1, t2, t3, t4;
   segment_t *s = &seg[segnum];
 
@@ -160,29 +160,35 @@ init_query_structure(int segnum, segment_t *seg, traps_t *tr, qnodes_t *qs) {
   _max(&qs->data[i1].yval, &s->v0, &s->v1); /* root */
   root = i1;
 
-  qs->data[i1].right = i2 = newnode(qs);
+  int i2 = newnode(qs);
+  qs->data[i1].right = i2;
   qs->data[i2].nodetype = T_SINK;
   qs->data[i2].parent = i1;
 
-  qs->data[i1].left = i3 = newnode(qs);
+  int i3 = newnode(qs);
+  qs->data[i1].left = i3;
   qs->data[i3].nodetype = T_Y;
   _min(&qs->data[i3].yval, &s->v0, &s->v1); /* root */
   qs->data[i3].parent = i1;
 
-  qs->data[i3].left = i4 = newnode(qs);
+  int i4 = newnode(qs);
+  qs->data[i3].left = i4;
   qs->data[i4].nodetype = T_SINK;
   qs->data[i4].parent = i3;
 
-  qs->data[i3].right = i5 = newnode(qs);
+  int i5 = newnode(qs);
+  qs->data[i3].right = i5;
   qs->data[i5].nodetype = T_X;
   qs->data[i5].segnum = segnum;
   qs->data[i5].parent = i3;
 
-  qs->data[i5].left = i6 = newnode(qs);
+  int i6 = newnode(qs);
+  qs->data[i5].left = i6;
   qs->data[i6].nodetype = T_SINK;
   qs->data[i6].parent = i5;
 
-  qs->data[i5].right = i7 = newnode(qs);
+  int i7 = newnode(qs);
+  qs->data[i5].right = i7;
   qs->data[i7].nodetype = T_SINK;
   qs->data[i7].parent = i5;