vermaseren / form

The FORM project for symbolic manipulation of very big expressions
GNU General Public License v3.0
982 stars 118 forks source link

Memory error with macros with arguments #434

Closed tueda closed 1 year ago

tueda commented 1 year ago

This is found during the discussion of #433. The following code leads to a Valgrind error with the current master branch:

#do i=1,5
  #define x`i'
#enddo
#define var(a) "(b)"
#show
#message `var(1)'
.end
FORM 4.3.0 (Dec 23 2022, v4.3.0-10-g6cc038c) 64-bits  Run: Mon Mar  6 13:36:12 2023
    #do i=1,5
      #define x`i'
    #enddo
    #define var(a) "(b)"
    #show
#The preprocessor variables:
0: VERSION_ = "4"
1: SUBVERSION_ = "3"
2: DATE_ = "Mon Mar  6 13:36:12 2023"
3: random_ = "________"
4: optimminvar_ = "0"
5: optimmaxvar_ = "0"
6: OLDNUMEXTRASYMBOLS_ = "0"
7: optimvalue_ = "0"
8: optimscheme_ = "0"
9: tolower_ = "0"
10: toupper_ = "0"
11: SYSTEMERROR_ = "0"
12: PID_ = "30607"
13: PARALLELTASK_ = "0"
14: NPARALLELTASKS_ = "1"
15: NAME_ = "test.frm"
16: NTHREADS_ = "1"
17: CMODULE_ = "1"
18: x1 = "1"
19: x2 = "1"
20: x3 = "1"
21: x4 = "1"
22: x5 = "1"
23: var = "(b)"
==30607== Invalid read of size 4
==30607==    at 0x4BABE5: GetChar (pre.c:370)
==30607==    by 0x4BAD6F: LoadInstruction (pre.c:1204)
==30607==    by 0x4BB3EA: PreProInstruction (pre.c:1125)
==30607==    by 0x4BC0C4: PreProcessor (pre.c:936)
==30607==    by 0x4F723D: main (startup.c:1647)
==30607==  Address 0x59367f0 is 944 bytes inside a block of size 960 free'd
==30607==    at 0x4E07C2B: free (in /home/linuxbrew/.linuxbrew/Cellar/valgrind/3.19.0/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==30607==    by 0x50DB06: M_free (tools.c:2449)
==30607==    by 0x50E7BD: FromList (tools.c:2791)
==30607==    by 0x4B59C2: PutPreVar (pre.c:668)
==30607==    by 0x4BAC0F: GetChar (pre.c:375)
==30607==    by 0x4BAD6F: LoadInstruction (pre.c:1204)
==30607==    by 0x4BB3EA: PreProInstruction (pre.c:1125)
==30607==    by 0x4BC0C4: PreProcessor (pre.c:936)
==30607==    by 0x4F723D: main (startup.c:1647)
==30607==  Block was alloc'd at
==30607==    at 0x4E05154: malloc (in /home/linuxbrew/.linuxbrew/Cellar/valgrind/3.19.0/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==30607==    by 0x50DA97: Malloc1 (tools.c:2322)
==30607==    by 0x50E770: FromList (tools.c:2786)
==30607==    by 0x4B59C2: PutPreVar (pre.c:668)
==30607==    by 0x4F5945: StartVariables (startup.c:1160)
==30607==    by 0x4F712D: main (startup.c:1590)
==30607==
    #message `var(1)'
~~~(b)
    .end
  0.06 sec out of 0.07 sec
==30607==
==30607== HEAP SUMMARY:
==30607==     in use at exit: 2,310,956,848 bytes in 125 blocks
==30607==   total heap usage: 167 allocs, 42 frees, 2,311,074,730 bytes allocated
==30607==
==30607== LEAK SUMMARY:
==30607==    definitely lost: 0 bytes in 0 blocks
==30607==    indirectly lost: 0 bytes in 0 blocks
==30607==      possibly lost: 0 bytes in 0 blocks
==30607==    still reachable: 2,310,956,848 bytes in 125 blocks
==30607==         suppressed: 0 bytes in 0 blocks
==30607== Rerun with --leak-check=full to see details of leaked memory
==30607==
==30607== For lists of detected and suppressed errors, rerun with: -s
==30607== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

The point is that the var macro has the index 23, which is $12 \times 2^n - 1$ with $n \in \mathbb{Z}_{\ge0}$. The magic number 12 comes from how the FromList function extends the list (the initial capacity is 12; the comment says 10 but that is wrong).

tueda commented 1 year ago

The following patch makes this problem manifest and gives a segfault.

diff --git a/sources/tools.c b/sources/tools.c
index 3279d07..f392291 100644
--- a/sources/tools.c
+++ b/sources/tools.c
@@ -2788,7 +2788,15 @@ VOID *FromList(LIST *L)
                        i = ( L->num * L->size ) / sizeof(int);
                        old = (int *)L->lijst; newL = (int *)newlist;
                        while ( --i >= 0 ) *newL++ = *old++;
-                       if ( L->lijst ) M_free(L->lijst,"L->lijst FromList");
+                       if ( L->lijst ) {
+                               // Before freeing the memory block, we mess up its content.
+                               // This must not be a problem.
+                               char *p = (char *)L->lijst;
+                               for (int i = 0; i < L->num * L->size; i++) {
+                                       *p++ = 'x'; // non-zero
+                               }
+                               M_free(L->lijst,"L->lijst FromList");
+                       }
                }
                L->lijst = newlist;
        }
tueda commented 1 year ago

When PutPreVar at line 372 or 375 extends the list of the preprocessor variables, the pointer p to a preprocessor variable in the current list will be invalid.

https://github.com/vermaseren/form/blob/6cc038c6bc9e318ed2971dd99c9d64990b72f4ad/sources/pre.c#L370-L381