SpEL's `InlineList` is cached as a mutable list in compiled mode

[Nephery]

The Issue

SpEL's inline list syntax correctly creates and caches immutable constant lists while in interpreted mode.

However, once the expression is compiled, the list which is returned by the compiled code is now unexpectedly mutable. And because the reference to this list is cached and always returned for every subsequent execution, modifications to that list would persist between executions of the expression.

Seen in: Spring Framework 7.0.8, but looking at the commit history for InlineList, the bug may have been present for a long time.

Suggested Fix

Have the compiled code generated by InlineList wrap the newly created list in Collections.unmodifiableList() before assigning it to the private static final instance variable. Something similar to what was done for interpretted mode.

Reproduction

Here's a simple test class to demonstrate the bug:

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.util.List;
import org.assertj.core.api.InstanceOfAssertFactories;
import org.junit.jupiter.api.Test;
import org.springframework.asm.MethodVisitor;
import org.springframework.expression.Expression;
import org.springframework.expression.spel.CodeFlow;
import org.springframework.expression.spel.SpelCompilerMode;
import org.springframework.expression.spel.SpelParserConfiguration;
import org.springframework.expression.spel.ast.InlineList;
import org.springframework.expression.spel.standard.SpelExpressionParser;

class InlineListCompilationMutabilityTests {


  /**
   * Test case to demonstrate that list creation is correctly immutable and constant when in interpreted mode 
   * as documented by
   * https://docs.spring.io/spring-framework/reference/core/expressions/language-ref/inline-lists.html
   */
  @Test
  void interpretedConstantList_isUnmodifiable() {
    SpelExpressionParser parser = new SpelExpressionParser();
    Expression expression = parser.parseExpression("{1, 2, 3}");

    @SuppressWarnings("unchecked")
    List<Object> value = (List<Object>) assertThat(expression.getValue())
        .asInstanceOf(InstanceOfAssertFactories.LIST)
        .containsExactly(1, 2, 3)
        .actual();

    assertThatThrownBy(() -> value.add("new-value"))
        .isInstanceOf(UnsupportedOperationException.class);

    assertThat(expression.getValue())
        .isSameAs(value)
        .asInstanceOf(InstanceOfAssertFactories.LIST)
        .containsExactly(1, 2, 3);
  }

  /**
   * Test case to demonstrate that compiled expression unexpectedly returns a mutable list.
   * Furthermore, this mutable list is stored in a static final variable as per
   * {@link InlineList#generateCode(MethodVisitor, CodeFlow)}, meaning that modifications to that
   * list are leaked and persisted to every subsequent execution of the "constant" list creation. 
   */
  @Test
  void compiledConstantList_isModifiable() {
    SpelExpressionParser parser = new SpelExpressionParser(new SpelParserConfiguration(
        SpelCompilerMode.IMMEDIATE, null));
    Expression expression = parser.parseExpression("{1, 2, 3}");

    // prime expression until expression compilation triggers
    expression.getValue();
    expression.getValue();
    expression.getValue();

    @SuppressWarnings("unchecked")
    List<Object> value = (List<Object>) assertThat(expression.getValue())
        .asInstanceOf(InstanceOfAssertFactories.LIST)
        .containsExactly(1, 2, 3)
        .actual();

    // Should have been a constant list, which should have caused List.add() to
    // throw UnsupportedOperationException, but it did not, mutating the list.
    value.add("new-value");
    assertThat(value).containsExactly(1, 2, 3, "new-value");

    // compiled bytecode (InlineList) returns a plain ArrayList held in a static final field,
    // without unmodifiable wrap.
    // The compiled expression's static final field is shared across every evaluation,
    // so a mutation via one evaluation is observed by the next.
    assertThat(expression.getValue())
        .isSameAs(value)
        .asInstanceOf(InstanceOfAssertFactories.LIST)
        // Leaked "new-value" modification to future executions of the expression
        .containsExactly(1, 2, 3, "new-value");
  }

}

[sbrannen]

Hi @Nephery,

Good catch! 👍

I've implemented and pushed a fix which will be included in 7.0.9.