Another LLM Grabbag

[elharo]

Affected version

HEAD

Bug description

Bug Analysis: Apache Maven Filtering

These were all pulled out by throwing the code at an LLM. This issue lists multiple independent bugs. If anyone wants to work on any of these, note it here and open a new issue just for that bug. Be careful that some of these are likely false reports, though closing those is also useful work.

Critical

1. Missing EOF checks in MultiDelimiterInterpolatorFilterReaderLineEnding.read()

File: src/main/java/org/apache/maven/shared/filtering/MultiDelimiterInterpolatorFilterReaderLineEnding.java

Lines 223 and 246 — The escape-detection loop (line 223) and delimiter-detection loop (line 246) are missing ch == -1 EOF checks. The equivalent single-delimiter class InterpolatorFilterReaderLineEnding correctly includes these checks (lines 206 and 221).

In the escape loop (line 223):

if (ch != getEscapeString().charAt(i) || ch == '\n' && !supportMultiLineFiltering) {

Missing: || ch == -1

In the delimiter loop (line 246):

if (ch != begin.charAt(i) || ch == '\n' && !supportMultiLineFiltering) {

Missing: || ch == -1

When EOF (-1) is encountered during escape string or begin-token matching, (char) ch casts -1 to 0xFFFF, which is appended to the key StringBuilder as a garbage character. In the escape loop this is partially mitigated because the mismatch branch clears the key (key.setLength(0)), but the garbage still appears in the key transiently and the key.append((char) ch) at line 221 runs before any check. In the delimiter loop the garbage char is silently consumed.

Impact: When a file ends during a begin-token or escape-string sequence, the reader can output corrupted data (garbage character inserted) or skip bytes.

High

2. No-op backup/restore in BaseFilter.getDefaultFilterWrappers()

File: src/main/java/org/apache/maven/shared/filtering/BaseFilter.java, lines 78–81

// backup values
boolean supportMultiLineFiltering = request.isSupportMultiLineFiltering();
request.setSupportMultiLineFiltering(supportMultiLineFiltering);

This reads the supportMultiLineFiltering flag from the request and immediately writes the same value back. The variable supportMultiLineFiltering is never used again. The comment says "backup values" but there is no corresponding restore. This appears to be leftover dead code from an incomplete refactoring.

3. MavenResourcesExecution.copyOf() drops multiple fields

File: src/main/java/org/apache/maven/shared/filtering/MavenResourcesExecution.java, lines 428–450

The copyOf() method omits the following fields:

• flatten (line 113) — controls flattened directory output
• propertiesEncoding (line 57) — encoding for .properties files
• delimiters (inherited from AbstractMavenFilteringRequest, line 75) — custom expression delimiter specifications
• interpolatorCustomizer (inherited from AbstractMavenFilteringRequest, line 84) — custom interpolator consumer

The new MavenResourcesExecution() default constructor calls initDefaults(), which sets delimiters to ["${*}", "@"]. If a caller had configured custom delimiters or the other missing fields, the copy loses them silently. This means any code path that creates a defensive copy (e.g., mavenResourcesExecution == null ? new MavenResourcesExecution() : mavenResourcesExecution.copyOf() in BaseFilter line 65) will produce an incomplete copy.

4. setEscapeString(null) silently ignored, state retained

File: src/main/java/org/apache/maven/shared/filtering/AbstractFilterReaderLineEnding.java, lines 67–73

public void setEscapeString(String escapeString) {
    // TODO NPE if escapeString is null ?
    if (escapeString != null && !escapeString.isEmpty()) {
        this.escapeString = escapeString;
        this.useEscape = true;
        calculateMarkLength();
    }
}

The code's own TODO acknowledges the problem. When escapeString is null:

• this.escapeString retains its previous value (not cleared)
• this.useEscape remains true if it was previously set from an earlier non-null call
• calculateMarkLength() is not called, leaving markLength potentially wrong

A caller who sets escape string to non-null, then later sets it to null to disable escaping, will find escaping still active with the old string. The field should be cleared and useEscape set to false for null/empty input.

Medium

5. Broken Windows path regex in getRelativeFilePath()

File: src/main/java/org/apache/maven/shared/filtering/FilteringUtils.java, lines 184 and 187

if (toPath.matches("^\\[a-zA-Z]:")) {
    toPath = toPath.substring(1);
}
if (fromPath.matches("^\\[a-zA-Z]:")) {
    fromPath = fromPath.substring(1);
}

The regex ^\\[a-zA-Z]: has a double-escaped backslash before the character class [a-zA-Z], making \\[ match a literal [ character at the start. The intended regex is ^[a-zA-Z]: — a drive letter followed by a colon at the start of the path. This regex will never match a Windows absolute path like C:\foo, so the leading-slash stripping logic never executes. This is dead code.

On Windows this could cause incorrect normalization of paths like /C:/foo where the leading / should be stripped.

6. BoundedReader recreated on every read() call

File: src/main/java/org/apache/maven/shared/filtering/MultiDelimiterInterpolatorFilterReaderLineEnding.java, line 207

BoundedReader in = new BoundedReader(this.in, markLength);

A new BoundedReader is created on every single read() invocation. The constructor calls this.in.mark(markLength) on the shared BufferedReader, setting a new mark at the current stream position each time. This is:

• Inefficient — allocates a new wrapper object and resets the mark on every character read
• Fragile — if the underlying reader's mark buffer is insufficient between calls, reset() silently fails

The BoundedReader was designed for use within a single logical "read operation" but is being used as a disposable per-character wrapper.

Low

7. PropertyUtils.getPropertyValue() NPE on absent key

File: src/main/java/org/apache/maven/shared/filtering/PropertyUtils.java, line 184

String v = p.getProperty(k);
// ...
while ((idx = v.indexOf("${")) >= 0) {

If the key k does not exist in properties p, v is null and v.indexOf("${") throws NullPointerException. The calling code (line 101–104 of loadPropertyFile) iterates over fileProps.keySet() and passes each key with combinedProps which should contain that key. However, the recursive resolution at line 199–201 could extract a key name (nk) not present in combinedProps — though that path is guarded by null checks (lines 221–222). The initial call is safe in practice, but the method lacks a null guard for v at the entry point.

8. setDelimiterSpecs() does not reset markLength base

File: src/main/java/org/apache/maven/shared/filtering/MultiDelimiterInterpolatorFilterReaderLineEnding.java, lines 133–141

public AbstractFilterReaderLineEnding setDelimiterSpecs(Set<String> specs) {
    delimiters.clear();
    for (String spec : specs) {
        delimiters.add(DelimiterSpecification.parse(spec));
        markLength += spec.length() * 2;
    }
    return this;
}

markLength is accumulated on top of the current value rather than recalculated from base (255 + escapeString.length). If setDelimiterSpecs is called multiple times, markLength grows unbounded. Currently this is safe because setEscapeString() always follows in Wrapper.getReader() and calls calculateMarkLength() which resets the value. However, this inter-call dependency is fragile and undocumented.

9. escapeWindowsPath() can produce ambiguous output

File: src/main/java/org/apache/maven/shared/filtering/FilteringUtils.java, lines 81–100

The regex ^(.*)[a-zA-Z]:\\\\(.*) only matches absolute Windows paths containing a drive letter. Relative paths with backslashes (e.g., src\main\java) are never escaped. The single/double backslash deduplication logic at line 90 (if (val.indexOf('\\', end + 1) == end + 1)) also means that C:\foo\bar and C:\foo\\bar both produce the same output C:\\\\foo\\\\bar — information is lost about whether the original had single or double backslashes.

10. Resource fields are uninitialized nullable defaults

File: src/main/java/org/apache/maven/shared/filtering/Resource.java, lines 31–36

All six fields default to null rather than sensible empty values. The getExtension() method in DefaultMavenResourcesFiltering handles the edge case where resource.getDirectory() might be null (line 179), but every consumer must null-check these fields. For example, Resource.getIncludes() returns null by default, not an empty list, requiring null checks at all call sites like DefaultMavenResourcesFiltering.setupScanner() line 410.

11. BaseFilter.loadProperties() skips null/empty filter file entries without warning

File: src/main/java/org/apache/maven/shared/filtering/BaseFilter.java, line 192

if (filterFile == null || filterFile.trim().isEmpty()) {
    // skip empty file name
    continue;
}

Null or empty entries in the filters list are silently skipped. If a caller accidentally includes an empty string in their filter list (e.g., from a malformed POM), no warning or error is logged. The problem is silently masked.

[elharo]

Individual issues have been filed for bugs 3–11:

• MavenResourcesExecution.copyOf() drops multiple fields #350 — MavenResourcesExecution.copyOf() drops multiple fields
• setEscapeString(null) silently ignored, previous state retained #351 — setEscapeString(null) silently ignored
• Broken Windows path regex in FilteringUtils.getRelativeFilePath() #352 — Broken Windows path regex in getRelativeFilePath()
• BoundedReader recreated on every read() call #353 — BoundedReader recreated on every read() call
• PropertyUtils.getPropertyValue() NPE on absent key #358 — PropertyUtils.getPropertyValue() NPE on absent key
• setDelimiterSpecs() accumulates markLength instead of recalculating #354 — setDelimiterSpecs() accumulates markLength
• escapeWindowsPath() can produce ambiguous output and misses relative paths #355 — escapeWindowsPath() can produce ambiguous output
• Resource fields default to null instead of sensible empty values #356 — Resource fields default to null
• BaseFilter.loadProperties() silently skips null/empty filter file entries #357 — BaseFilter.loadProperties() silently skips null/empty entries

Bugs 1 and 2 were already filed separately.