elharo opened a new issue, #344:
URL: https://github.com/apache/maven-filtering/issues/344
### 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):
```java
if (ch != getEscapeString().charAt(i) || ch == '\n' &&
!supportMultiLineFiltering) {
```
Missing: `|| ch == -1`
In the delimiter loop (line 246):
```java
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
```java
// 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
```java
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
```java
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
```java
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
```java
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
```java
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
```java
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
