#2798 Memory error caused by redefining an array with the same name during its initialization

[Hiroki Motoyoshi]

A memory error occurs when a function block is invoked during array initialization, and within that block, an array with the same name as the one currently being initialized is redefined or undefined.

function $setup() <<end
  array tmp = [1]
  return 2
end

array tmp = [1, $setup(), 3]

print tmp

On my macOS environment, I got the following error message:

gnuplot(92785,0x11993f5c0) malloc: *** error for object 0x7fcded43d070: pointer being realloc'd was not allocated
gnuplot(92785,0x11993f5c0) malloc: *** set a breakpoint in malloc_error_break to debug
Abort trap: 6

[Ethan Merritt]

This is a more serious problem than one with function definitions (#2797).
Because arrays share a namespace with regular variables, the corruption also happens in a much more natural setting. It can arise for any array acces, not just initialization. I worry that a segfault from scenario below is more likely to come up, and less obvious to the user, than re-initializing a global array.

function $line_ab(x) << EOF
   A = 2
   B = 3
   return A*x + B
EOF

array A[3]
x = 123.45

A[1] = x
A[2] = $line_ab(x)
A[3] = 0

print A

Declaring A and B as local A = 2; local B = 3 would avoid the problem, but I think there are cases when it is reasonable for functions or function blocks to assign a value to a global variable before using it.

[Hiroki Motoyoshi]

Let's call the original post’s situation Case 1 and your situation Case 2.

These two cases are handled by different functions.
Case 1 is "parse_array_constant()", and Case 2 is "is_array_assignment()".

I created a patch to fix for Case 2 by modifing the function 'is_array_assignment()'.

In the following command,

A[2] = $line_ab(x)

after evaluating the right side, the fixed version detects whether the variable A is no longer an array or whether the original array pointer has been changed.

The following Case 3 also triggers a memory error:

Case.3

function $index(x) <<end
  A = 1
  return int(x)
end

array A[3]
A[$index(1)] = 1
print A

The patch can handle Case 3, too.

[Ethan Merritt]

Those patches are much better than anything I have come up with so far.

I have found a 4th problem case.

function $clobber_C() <<EOF
  array C = ["a", "b", "c"]
  return 3
EOF

array C = [1, 2, $clobber_C(), 4]
print C

I think the problem here is that the replacement array (the one that overwrites the original array) is too small to hold the fourth element of the assignment statement. This failure occurs during evaluation of const_express(), before the test for newvalue != oldvalue can be made.

/* Evaluate right side of assignment */
c_token += 2;
const_express(&newvalue);
/* Guard against replacement of destination array to other thing during evaluation. */
if (udv->udv_value.type != ARRAY ||
    udv->udv_value.v.value_array != orig_value_array) {
    int_error(c_token, "destination array has been replaced during evaluation");
}

[Hiroki Motoyoshi]

Cases 1 and 4 seem to involve essentially the same problem. I have created a patch that addresses both.

The issue lies in how the "parse_array_constant()" function handles array initialization. In the current implementation, each right-hand-side value is evaluated and assigned directly to the destination array one by one. This can cause unexpected behavior if the destination array is modified during the evaluation.

A safer approach would be:

• Build the entire result in a temporary C array.
• After construction, check whether the destination array has been modified.
• If it has not changed, copy the contents of the temporary array into the destination array.

This patch also addresses the following case:

Case 5

function $clobber_C() <<EOF
  C = 1
  return 3
EOF

array C = [1, 2, $clobber_C(), 4]
print C

[Ethan Merritt]

Looks pretty good. I've committed all three patches to 6.1

The third patch introduced a memory leak (elements array not free'd if an error occurs). I fixed it with respect to errors in parse_array_constant(), but there's still a leak if int_error is called during evaluation of const_express().

[Hiroki Motoyoshi]

I'm sorry.

The modification to parse_array_constant() in the patch 0002_parse_array_constant.patch was insufficient.

An improved patch file is attached.
Please apply it after the commit [aa95c30].

There was a bug in the section that copies data from the temporary array elements to array->v.value_array. I have modified it to perform an accurate copy using the number of elements from the initializer on the right-hand side. Additionally, there was another location where free(elements) was necessary when recovering from an error, so I have added that as well.

[Hiroki Motoyoshi]

Apologies, I overlooked the deallocation of 'elements' in the event of an error in the "0002_parse_array_constatnt.patch". Thank you for fixing and applying it.

I have found another case for this issue, which is triggered by function instead of function block after Patch#827.

Case 6

setup(dummy) = (A=1)
array A[3] = [1,2,3]
f(dummy) = (A[1]=setup(1))
print f(1)

The issue in Case.6 can be avoided within the f_assign() function by detecting variable substitution, just as we have done in previous cases.

A patch to do this is attached.

However, there is a caveat regarding this patch. When using the action table for evaluation, it is not possible to obtain information about the left-hand side (i.e., the destination array) before evaluating the right-hand side. As a result, we can accurately detect changes such as an array no longer being an array, but not when one array is replaced with another. Even so, in the case of array-to-array replacement, fortunately, the execution can proceed without triggering a fatal error.

Please take a look at the following script:

Case.7

function $setup(x) <<end
  array A[1]
  return -1
end

array A[3] = [1,2,3]
f(x) = (A[2]=$setup(1))
dummy = f(1)

In this case, a three-element array 'A' is replaced with a one-element array 'A' inside the "$setup()" function block. Since the script then attempts to assign a value to the 2nd element of the new array, an index error occurs. If 'A' were replaced with a larger array instead, the script would continue to run without any errors.

If such behavior is acceptable, the attached patch should be sufficient to handle the Case 6 and 7.