FasterXML / FasterXML/jackson-core

File/Path generator construction can leak or bypass decorated output cleanup

Open
#1,711 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
2.4k
Forks
928
Avg merge
2d 18h
Merged PRs (30d)
24

Description

## Description

While reviewing the recent failed-construction cleanup changes in `TextualTSFactory` and `BinaryTSFactory`, I noticed two remaining lifecycle gaps in the `File`/`Path` generator paths.

I added regression tests for these cases and confirmed that they currently fail on `3.x`.

### 1. Output stream is opened before `IOContext` creation

For example, `TextualTSFactory.createGenerator(File, JsonEncoding)` currently starts with:

```java
final OutputStream out = _fileOutputStream(f);
final IOContext ioCtxt = _createContext(_createContentReference(f), true, enc);

try {
// generator construction
} catch (RuntimeException e) {
_releaseOnFailedConstruction(ioCtxt, e);
_closeOnFailedConstruction(out, e);
throw e;
}
```

If `_fileOutputStream(f)` succeeds but `_createContext(...)` fails, the exception occurs before entering the protected construction block.

At that point the factory-created `OutputStream` has already been opened, but it is not closed.

The lifecycle is roughly:

```text
open OutputStream

create IOContext

failure

cleanup block is never entered

OutputStream remains open
```

This reproduces for both the `File` and `Path` generator paths.

One possible way to avoid this gap is to create the `IOContext` before opening the output stream, and then perform output resource acquisition inside the protected construction block.

One implementation detail to watch is `BinaryTSFactory.createGenerator(File, ...)`, which currently builds the content reference from the opened `OutputStream`. If the stream open is moved inside the guarded block, that path may need to use the `File` as the content reference instead, matching the textual factory.

### 2. Decorated output is not closed when later construction fails

There is another ownership transition after output decoration.

Currently the decorated stream can be passed directly into generator construction:

```java
_createUTF8Generator(writeCtxt, ioCtxt, _decorate(ioCtxt, out))
```

If the `OutputDecorator` successfully returns a wrapper and generator construction subsequently fails, the failure handler closes the original `out`:

```java
_closeOnFailedConstruction(out, e);
```

but not the decorated output returned by the `OutputDecorator`.

For a decorator that owns additional state or resources which are released by its `close()` implementation, closing only the underlying stream bypasses the wrapper's cleanup.

The lifecycle in this case is:

```text
factory OutputStream

OutputDecorator

decorated OutputStream

generator construction fails

original OutputStream closed
decorated OutputStream not closed
```

Once decoration succeeds, the decorated output is the resource passed downstream and should be the resource cleaned up if later generator construction fails.

If decoration itself fails before returning a wrapper, the original factory-created output remains the resource that needs to be closed.

### Reproduction

I added regression tests covering:

- `File` output when `IOContext` creation fails
- `Path` output when `IOContext` creation fails
- decorated `File` output when later generator construction fails

All three currently fail on `3.x` with the expected cleanup assertions.

```text
Tests run: 4, Failures: 3
```

### Affected paths

Affected paths appear to include:

- `TextualTSFactory.createGenerator(File, JsonEncoding)`
- `TextualTSFactory.createGenerator(Path, JsonEncoding)`
- `BinaryTSFactory.createGenerator(File, JsonEncoding)`
- `BinaryTSFactory.createGenerator(Path, JsonEncoding)`

### Expected behavior

`File`/`Path` generator construction should clean up the resource currently owned by the construction path at every failure point:

- If `IOContext` creation fails, no factory-created output stream should be left open.
- If output stream creation fails after an `IOContext` has been created, the context should be released.
- If output decoration fails, the original factory-created output stream should be closed.
- If decoration succeeds but later generator construction fails, the decorated output should be closed.
- `IOContext` instances should be released on failed construction.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.