Skip to content

Commit

Permalink
Fix double release of form attributes
Browse files Browse the repository at this point in the history
Attributes returned by `postRequestDecoder.next()` are released twice when the decoder is destroyed:
- Once because it's stored in the `bodyListHttpData` list
- A second time because the `HttpDataFactory` keeps a copy around.
This means that even if user code retains the attribute in question once, it may still be collected.

In the test case, this issue is visible because the attribute is only parsed to a map later downstream in an IO thread, when the `FormDataHttpContentProcessor` has already completed.

This patch uses `removeHttpDataFromClean` to avoid the duplicate release for HTTP data where it could happen. `removeHttpDataFromClean` is a no-op for data that isn't created by the `HttpDataFactory`, so this patch doesn't break in those cases.

This patch risks exposing downstream buffer leaks: If the user code doesn't properly release attributes it retains, they would previously still have been collected because of this duplicate release. After this change, they may leak, potentially even leaving files on disk. However, imo this patch implements the "right" behavior, and we should deal with other issues as they appear.

I've done some testing, but it's inconclusive. I ran `./gradlew check` with and without this patch, and in both cases, attribute files were retained in `/tmp`. Can't tell whether this patch introduced new leaks.

Addresses #6705
  • Loading branch information
yawkat committed Jan 3, 2022
1 parent fcac0ee commit f589e56
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,13 @@ protected void onData(ByteBufHolder message) {
case Attribute:
Attribute attribute = (Attribute) data;
messages.add(attribute);
postRequestDecoder.removeHttpDataFromClean(attribute);
break;
case FileUpload:
FileUpload fileUpload = (FileUpload) data;
if (fileUpload.isCompleted()) {
messages.add(fileUpload);
postRequestDecoder.removeHttpDataFromClean(fileUpload);
}
break;
default:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package io.micronaut.http.server.netty.binding

import io.micronaut.context.ApplicationContext
import io.micronaut.http.HttpRequest
import io.micronaut.http.HttpResponse
import io.micronaut.http.HttpStatus
import io.micronaut.http.MediaType
import io.micronaut.http.annotation.Body
import io.micronaut.http.annotation.Controller
import io.micronaut.http.annotation.Post
import io.micronaut.http.client.HttpClient
import io.micronaut.runtime.server.EmbeddedServer
import reactor.core.publisher.Flux
import spock.lang.Issue
import spock.lang.PendingFeature
import spock.lang.Specification

class FormDataDiskSpec extends Specification {
@Issue('https://github.com/micronaut-projects/micronaut-core/issues/6705')
@PendingFeature
void "test parsing form to map with disk attributes"() {
given:
def server = (EmbeddedServer) ApplicationContext.run(EmbeddedServer, [
'micronaut.server.multipart.disk': true,
'micronaut.server.multipart.mixed': true,
'micronaut.server.thread-selection': 'IO',
'netty.resource-leak-detector-level': 'paranoid'
])
def client = server.applicationContext.createBean(HttpClient, server.URI)

when:
HttpResponse<?> response = Flux.from(client.exchange(HttpRequest.POST('/form-disk/object', [
name:"Fred"
]).contentType(MediaType.APPLICATION_FORM_URLENCODED_TYPE), String)).blockFirst()

then:
response.status == HttpStatus.OK
response.body.isPresent()
response.body.get() == '{"name":"Fred"}'

cleanup:
server.stop()
client.stop()
}

@Controller(value = '/form-disk', consumes = MediaType.APPLICATION_FORM_URLENCODED)
static class FormController {
@Post('/object')
Object object(@Body Object object) {
object
}
}
}

0 comments on commit f589e56

Please sign in to comment.