valgrind errors in FORMapart tests

[jodavies]

On this branch https://github.com/jodavies/form/tree/ci-formapart I have integrated the FORMapart tests into the CI.

Some of the tests fail under valgrind with

==7113== Thread 3:
==7113== Conditional jump or move depends on uninitialised value(s)
==7113==    at 0x295AC3: CheckWild (wildcard.c:1876)
==7113==    by 0x28AB5F: RunReplace (transform.c:1624)
==7113==    by 0x2918E6: RunTransform (transform.c:782)
==7113==    by 0x2317DA: Generator (proces.c:3874)
==7113==    by 0x23276B: Generator (proces.c:4245)
==7113==    by 0x23209A: Generator (proces.c:4075)
==7113==    by 0x23276B: Generator (proces.c:4245)
==7113==    by 0x23276B: Generator (proces.c:4245)
==7113==    by 0x23276B: Generator (proces.c:4245)
==7113==    by 0x23276B: Generator (proces.c:4245)
==7113==    by 0x23276B: Generator (proces.c:4245)
==7113==    by 0x19A50E: SymGen (function.c:616)

[tueda]

The following simplified example almost always gives the Valgrind error with tvorm -w1:

CF f;
S x;
L F = f(2*x);
id f(x?) = f(x);
transform f,replace(1,1)=(xarg_,xarg_);
.end

==1229906== Thread 2:
==1229906== Conditional jump or move depends on uninitialised value(s)
==1229906==    at 0x418E136: CheckWild (wildcard.c:1876)
==1229906==    by 0x41828B9: RunReplace (transform.c:1624)
==1229906==    by 0x4189C6F: RunTransform (transform.c:782)
==1229906==    by 0x4126586: Generator (proces.c:3874)
==1229906==    by 0x4127531: Generator (proces.c:4245)
==1229906==    by 0x4126E77: Generator (proces.c:4075)
==1229906==    by 0x4127531: Generator (proces.c:4245)
==1229906==    by 0x41A4ED0: RunThread (threads.c:1423)
==1229906==    by 0x5E6E15D: start_thread (in /home/linuxbrew/.linuxbrew/Cellar/glibc/2.39/lib/libc.so.6)
==1229906==    by 0x5EE4A5F: clone (in /home/linuxbrew/.linuxbrew/Cellar/glibc/2.39/lib/libc.so.6)

[tueda]

Somehow, the above simple example now doesn't show the Valgrind error in my environment with tvorm -w1, but does with tvorm -w4. It's subtle...

Working on this kind of "security" issue with American AI models is difficult because of this, but with some patience, GPT told me the issue is that (AT.WildMask,AN.WildValue,AN.NumWild) are set for wild[4] in RunReplace without setting WildStop. Indeed, valgrind --track-origins=yes shows the memory is allocated on the stack in RunReplace. I'm not 100% sure that setting it resolves all the issues.

[jodavies]

Setting WildStop to &wild[4] does look consistent with how it is set in if.c and pattern.c. But it is not clear to me why this is not an issue for tform -w1 or form also?

[jodavies]

Anyway, I think a fix like the following, is what we need. Later I'll test it with FORMapart and make a PR if everything works.

diff --git a/sources/transform.c b/sources/transform.c
index 58a8aab4..008c2e75 100644
--- a/sources/transform.c
+++ b/sources/transform.c
@@ -1618,6 +1618,7 @@ int RunReplace(PHEAD WORD *fun, WORD *args, WORD *info)
                                        wild[2] = WILDARGSYMBOL;
                                        wild[3] = 0;
                                        AN.WildValue = wild;
+                                       AN.WildStop = wild + wild[1];
                                        AT.WildMask = &mask;
                                        mask = 0;
                                        AN.NumWild = 1;
@@ -1694,6 +1695,7 @@ getthisone:;
                                        wild[2] = WILDARGINDEX+AM.OffsetIndex;
                                        wild[3] = 0;
                                        AN.WildValue = wild;
+                                       AN.WildStop = wild + wild[1];
                                        AT.WildMask = &mask;
                                        mask = 0;
                                        AN.NumWild = 1;
@@ -1715,6 +1717,7 @@ getthisone:;
                                        wild[2] = WILDARGVECTOR+AM.OffsetVector;
                                        wild[3] = 0;
                                        AN.WildValue = wild;
+                                       AN.WildStop = wild + wild[1];
                                        AT.WildMask = &mask;
                                        mask = 0;
                                        AN.NumWild = 1;
@@ -1742,6 +1745,7 @@ getthisone:;
                                        wild[2] = WILDARGFUN;
                                        wild[3] = 0;
                                        AN.WildValue = wild;
+                                       AN.WildStop = wild + wild[1];
                                        AT.WildMask = &mask;
                                        mask = 0;
                                        AN.NumWild = 1;

[tueda]

Using GDB's awatch command on AN.WildValue and AN.WildStop in the example above, we can see that these variables are accessed as follows:

(1) WRITE  WildValue  pattern.c:179     TestMatch      AN.WildValue = w = m + SUBEXPSIZE;
(1) WRITE  WildStop   pattern.c:182     TestMatch      AN.WildStop = m;
    READ   WildValue  wildcard.c:1522   ClearWild      n = (AN.WildValue[-SUBEXPSIZE+1]-SUBEXPSIZE)/4;		/* Number of wildcards */
    READ   WildValue  wildcard.c:1527   ClearWild      w = AN.WildValue;
    READ   WildStop   pattern.c:319     TestMatch      while ( w < AN.WildStop ) {
    READ   WildStop   pattern.c:319     TestMatch      while ( w < AN.WildStop ) {
    READ   WildValue  findpat.c:1081    FindRest       m = AN.WildValue;
    READ   WildValue  function.c:1646   ScanFunctions  m = AN.WildValue;
    READ   WildValue  function.c:883    MatchFunction  m = AN.WildValue;
    READ   WildValue  wildcard.c:1801   CheckWild      w = AN.WildValue;
    READ   WildStop   wildcard.c:1869   CheckWild      if ( s >= AN.WildStop || *s != SETTONUM )
    READ   WildStop   wildcard.c:2148   CheckWild      if ( w < AN.WildStop && ( *w == FROMSET || *w == SETTONUM ) ) {
    READ   WildValue  wildcard.c:1554   AddWild        w = AN.WildValue;
    READ   WildValue  wildcard.c:1522   ClearWild      n = (AN.WildValue[-SUBEXPSIZE+1]-SUBEXPSIZE)/4;		/* Number of wildcards */
    READ   WildValue  wildcard.c:1527   ClearWild      w = AN.WildValue;
    READ   WildValue  findpat.c:1081    FindRest       m = AN.WildValue;
(2) WRITE  WildValue  transform.c:1620  RunReplace     AN.WildValue = wild;
    READ   WildValue  wildcard.c:1801   CheckWild      w = AN.WildValue;
(3) READ   WildStop   wildcard.c:1876   CheckWild      if ( s >= AN.WildStop || *s != SETTONUM )
(4) READ   WildStop   wildcard.c:2148   CheckWild      if ( w < AN.WildStop && ( *w == FROMSET || *w == SETTONUM ) ) {

At (1), WildValue and WildStop are set to point into the heap. At (2), WildValue is set to point into the stack. Usually, stack addresses are higher than heap addresses, so s >= AN.WildStop at (3) and !(w < AN.WildStop) at (4). As a result, we fortunately take the correct branch, leading to the correct result. But it seems that running under Valgrind may change this memory layout in multithreaded runs, so I got the warning, depending on this memory layout.

[tueda]

This comment:

form/sources/transform.c

Lines 1608 to 1609 in 82407dd

	This means that we have to set op the special variables
	(AT.WildMask,AN.WildValue,AN.NumWild)

should also be updated to include AN.WildStop? Or is it obvious that it should be changed too? (It was missed here, though.)