File descriptor leak in ObjectMapper#writeValue with File

[fxha]

Describe the bug
Jackson will leak a file descriptor when using writeValue(File resultFile, Object value) in ObjectMapper and a failure occurs during closing the internal stream that's owned by Jackson (e.g. when flushing buffers). In this case, a file stream is created but the access to the file content is prohibited. This leads to the exceptional case that the stream is never closed and leaked. This bug may affect jackson.core but I think the core issues is located in databind's _writeValueAndClose due to poor resource handling.

Stack Trace:

writeBytes:-1, FileOutputStream (java.io)
write:354, FileOutputStream (java.io)
_flushBuffer:2171, UTF8JsonGenerator (com.fasterxml.jackson.core.json)
close:1214, UTF8JsonGenerator (com.fasterxml.jackson.core.json)
_writeValueAndClose:4573, ObjectMapper (com.fasterxml.jackson.databind)
writeValue:3763, ObjectMapper (com.fasterxml.jackson.databind)
lambda$testJacksonBug2$1:54, MainTest
execute:-1, 1778629809 (MainTest$$Lambda$338)
assertThrows:55, AssertThrows (org.junit.jupiter.api)
assertThrows:37, AssertThrows (org.junit.jupiter.api)
assertThrows:3082, Assertions (org.junit.jupiter.api)
testJacksonBug2:54, MainTest

Version information
v2.13.3

To Reproduce
This bug is a little bit tricky to reproduce but you can lock a file on Windows, where you can successfully create a file stream (with a valid file descriptor) but without access to any file content. On Unix, you need a lot of "luck" to get this case happening.

1. Create a demo Java project with both jackson core and databind on Windows!
2. Create a JUnit 5 test
3. Copy example code

class MainTest {
    @TempDir
    File tempDir;

    @Test
    void testJacksonBug2() throws IOException {
        Path filePath = Paths.get(tempDir.getAbsolutePath(), "example.json");
        try (FileOutputStream outputStream = new FileOutputStream(filePath.toString());
             FileChannel channel = outputStream.getChannel();
             FileLock lock = channel.lock()) {
            assertThrows(IOException.class, () -> new ObjectMapper().writeValue(filePath.toFile(), new JsonEntry()));
        }

        assertTrue(filePath.toFile().delete());
    }

    private static class JsonEntry {
        public int value = 1;
    }
}

You'll notice that the the deletion of the example file (last line) will fail but also that JUnit is unable to clearup the temporary directory because a file descriptor is still open. Everything is fine if you comment out the "writeValue"-line or write through the locked channel.

Expected behavior
The file stream should be correctly closed, regardless of whether an exception is thrown or not.

Additional context
This issue is related to #3455 but during close of UTF8JsonGenerator.

Workaround: use an owned stream with try-with-resources and passing the stream into Jackson.

[cowtowncoder]

Hmmh. First of all, thank you reporting this issue and providing a reproduction.
Unfortunately I cannot really use a Windows-only reproduction either myself (do not have a machine with Windows) or, more importantly, want the hassle of trying to figure out how to do CI with that.

But as to the issue itself, part I do not understand is why the OutputStream would not be closed. Relevant portions are:

    public void writeValue(File resultFile, Object value)
        throws IOException, StreamWriteException, DatabindException
    {
        _writeValueAndClose(createGenerator(resultFile, JsonEncoding.UTF8), value);
    }

    protected final void _writeValueAndClose(JsonGenerator g, Object value)
        throws IOException
    {
        SerializationConfig cfg = getSerializationConfig();
        // ... 
        try {
            _serializerProvider(cfg).serializeValue(g, value);
        } catch (Exception e) {
            ClassUtil.closeOnFailAndThrowAsIOE(g, e);
            return;
        }
        g.close();
    }

and it would seem that closing should occur.

So is this not the code path taken?

[fxha]

So is this not the code path taken?

Actually, this is the right code path but UTF8JsonGenerator#close in jackson.core is never completed. The stream isn't closed if UTF8JsonGenerator#_flushBuffer raise an exception:

public void close() throws IOException
{
    // ...

    _flushBuffer(); // !!! This will raise an IOException due to an I/O failure in "writeBytes"
// ----- Leak: we'll never reach the "_outputStream.close()" from below -----
    _outputTail = 0;

    // ...
    if (_outputStream != null) {
        if (_ioContext.isResourceManaged() || isEnabled(Feature.AUTO_CLOSE_TARGET)) {
            _outputStream.close();
        } else if (isEnabled(Feature.FLUSH_PASSED_TO_STREAM)) {
            // If we can't close it, we should at least flush
            _outputStream.flush();
        }
    }

    _releaseBuffers();
}

I found that ClassUtil.closeOnFailAndThrowAsIOE is used in other places.

protected final void _writeValueAndClose(JsonGenerator g, Object value)
    throws IOException
{
    // ...
    try {
        _serializerProvider(cfg).serializeValue(g, value);
    } catch (Exception e) {
        ClassUtil.closeOnFailAndThrowAsIOE(g, e); // <-- stream is closed on exception
        return;
    }
    g.close();
}

But as to the issue itself, part I do not understand is why the OutputStream would not be closed.

An exception can prevent the stream from been closed when writing the remaining buffers to disk.

[cowtowncoder]

That makes sense. At first I was looking at this:

    public static void closeOnFailAndThrowAsIOE(JsonGenerator g, Exception fail)
        throws IOException
    {
        // 04-Mar-2014, tatu: Let's try to prevent auto-closing of
        //    structures, which typically causes more damage.
        g.disable(JsonGenerator.Feature.AUTO_CLOSE_JSON_CONTENT);
        try {
            g.close();
        } catch (Exception e) {
            fail.addSuppressed(e);
        }
        throwIfIOE(fail);
        throwIfRTE(fail);
        throw new RuntimeException(fail);
    }

which would seem to not have issues. But then I realized that it is (as you correctly point out) the problem with JsonGenerator#close().

This is unfortunate because solving the problem becomes rather more complicated -- my first instinct here is to solve this in specific JsonGenerator implementation since it is owned by generator so that is its responsibility.
But that unfortunately means fixes all over various format backends to make it all work correctly.
An alternative would be for ObjectMapper (and ObjectWriter) to force closing, as they are ultimately constructing target/source. But that seems slightly wrong semantically.

Still, first things first; reproduction to show the issue and (once fixed) protect against regression.

[cowtowncoder]

Ok, I think this should now be fixed by:

FasterXML/jackson-core#764

which does add try-catch to JsonGenerator.close() implementations of jackson-core.

Please let me know if I missed something.