Created attachment 86792 [details] [review] fix patch There is a bug in file dbus-marshal-recursive-util.c: ... start_next_test ("All values in one big toplevel %d iteration\n", 1); { TestTypeNode *nodes[N_VALUES]; i = 0; while ((nodes[i] = value_generator (&i))) ; run_test_nodes (nodes, N_VALUES); for (i = 0; i < N_VALUES; i++) node_destroy (nodes[i]); } ... There is two problems in this code: 1) "nodes[0]" always uninitialized (because "i" is incremented from "0" to "1" in first call to "value_generator") 2) a write to "nodes[120]" is outside array bounds (again, because "i" increments) Solution: <patch attached> Now the incremented value of "i" is compensated and no null element written to array.
Comment on attachment 86792 [details] [review] fix patch FALSE. In language C, lvalue = rexpr means lvalue calculated first, so nodes[i] = value_enerator (&i) is just work as expected. This patch in fact doesn't change anything but a little more complicated. :-)
(In reply to comment #1) > Comment on attachment 86792 [details] [review] > fix patch > > FALSE. In language C, lvalue = rexpr means lvalue calculated first, so > nodes[i] = value_enerator (&i) is just work as expected. > > This patch in fact doesn't change anything but a little more complicated. :-) Hmm, Seems I was wrong, this will cause undefined behavior and your patch looks good because it inserts a sequence point between the two modifications. So this may work as the author's expectation in some compiler but fail in some others.
Comment on attachment 86792 [details] [review] fix patch looks good
This one is either MSVC-bug or undefined behavior: The mingw and msvc give different results. Test program: #include <stdio.h> int inc( int* i ){ *i = *i +1; return *i; } int main(void){ int array[2] = { -1 , -1 }; int i = 0; array[i] = inc(&i); printf("array[0] = %d\n",array[0]); printf("array[1] = %d\n",array[1]); return 0; } MSVC output (32 and 64 bit): array[0] = -1 array[1] = 1 MinGW output: array[0] = 1 array[1] = -1 So this patch can be treated as "compatibility" patch for removal of undefined behavior (because msvc bug?).
I think this is indeed undefined behaviour (read about "sequence points" for more details).
Comment on attachment 86792 [details] [review] fix patch Review of attachment 86792 [details] [review]: ----------------------------------------------------------------- > test bug fix1 That's not a commit message. Please see, for instance, <http://robots.thoughtbot.com/5-useful-tips-for-a-better-commit-message>. I'll change this to something more like this when I commit it: > make_and_run_test_nodes: avoid undefined behaviour > > In code that looks like n[i] = v(&i), where v alters the > value of i, C leaves it undefined whether the old or new > value of i is used to locate n[i]. Fix this by inserting > a sequence point to disambiguate the intended order. > > Bug: https://bugs.freedesktop.org/show_bug.cgi?id=69924 ::: dbus/dbus-marshal-recursive-util.c @@ +1778,5 @@ > > i = 0; > + while ((node = value_generator (&i))) > + { > + nodes[i-1]=node; Coding style: we leave spaces around operators, "nodes[i - 1] = node;". I'll fix that when I commit it.
Fixed in git for 1.6.20, 1.7.10.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.