Skip to content

HttpPostRequestDecoder may cause memory leak #11109

Closed

Description

@devilyard

set HttpDataFactory to use disk = false like below:

HttpDataFactory factory = new DefaultHttpDataFactory(false, CharsetUtil.UTF_8);

and then create an HttpPostRequestDecoder instance:

HttpPostRequestDecoder httpDecoder = new HttpPostRequestDecoder(factory, httpRequest);

after decoder is destroyed the reference count of httpRequest is incorrect.

in the method createAttribute of DefaultHttpDataFactory, MemoryAttribute is not added to HttpData list:

@Override
    public Attribute createAttribute(HttpRequest request, String name, long definedSize) {
        if (useDisk) {
            Attribute attribute = new DiskAttribute(name, definedSize, charset, baseDir, deleteOnExit);
            attribute.setMaxSize(maxSize);
            List<HttpData> list = getList(request);
            list.add(attribute);
            return attribute;
        }
        if (checkSize) {
            Attribute attribute = new MixedAttribute(name, definedSize, minSize, charset, baseDir, deleteOnExit);
            attribute.setMaxSize(maxSize);
            List<HttpData> list = getList(request);
            list.add(attribute);
            return attribute;
        }
        MemoryAttribute attribute = new MemoryAttribute(name, definedSize);
        attribute.setMaxSize(maxSize);
        return attribute;
    }

so on destroying

@Override
    public void cleanAllHttpData() {
        Iterator<Entry<HttpRequest, List<HttpData>>> i = requestFileDeleteMap.entrySet().iterator();
        while (i.hasNext()) {
            Entry<HttpRequest, List<HttpData>> e = i.next();

            // Calling i.remove() here will cause "java.lang.IllegalStateException: Entry was removed"
            // at e.getValue() below

            List<HttpData> list = e.getValue();
            for (HttpData data : list) {
                data.release();
            }

            i.remove();
        }
    }

the MemoryAttributes created above are not released.

I'm not sure if it is a bug.

Expected behavior

the reference count of httpRequest before decoding should equals to the one after decoder is destroyed.

Actual behavior

if the original reference count of http request is 1, and the http request contains 2 fields , then after destroying the reference count is 3 !

Steps to reproduce

Minimal yet complete reproducer code (or URL to code)

send a form :

public void testSubmitForm() throws IOException {
        String boundary = "---------------------------ABCDEFGHIJKLMNOPQRSTUVWXYZ";
        URL url = new URL("http://127.0.0.1:1084/test");
        HttpURLConnection connection = (HttpURLConnection) url.openConnection();
        connection.setConnectTimeout(5000);
        connection.setReadTimeout(30000);
        connection.setDoOutput(true);
        connection.setDoInput(true);
        connection.setUseCaches(false);
        connection.setRequestMethod("POST");
        connection.setRequestProperty("Connection", "Keep-Alive");
        connection.setRequestProperty("User-Agent", "Mozilla/5.0 (Windows; U; Windows NT 6.1; zh-CN; rv:1.9.2.6)");
        connection.setRequestProperty("Content-Type", "multipart/form-data; boundary=" + boundary);

        try (OutputStream out = new DataOutputStream(connection.getOutputStream())) {
            out.write(("\r\n--" + boundary + "\r\n").getBytes(StandardCharsets.UTF_8));
            out.write("Content-Disposition: form-data; name=\"username\"\r\n\r\n".getBytes(StandardCharsets.UTF_8));
            out.write("chinnsii".getBytes(StandardCharsets.UTF_8));

            out.write(("\r\n--" + boundary + "\r\n").getBytes(StandardCharsets.UTF_8));
            out.write("Content-Disposition: form-data; name=\"password\"\r\n\r\n".getBytes(StandardCharsets.UTF_8));
            out.write("chinnsii".getBytes(StandardCharsets.UTF_8));

            byte[] endData = ("\r\n--" + boundary + "--\r\n").getBytes();
            out.write(endData);
            out.flush();
        }

        try (BufferedReader reader = new BufferedReader(new InputStreamReader(connection.getInputStream()))) {
            String line;
            while ((line = reader.readLine()) != null) {
                System.out.println(line);
            }
        }
    }

decode to a hash map

Map<String, Object> convert(FullHttpRequest httpRequest) throws IOException {
        HttpDataFactory factory = new DefaultHttpDataFactory(false, CharsetUtil.UTF_8);
        HttpPostRequestDecoder httpDecoder = new HttpPostRequestDecoder(factory, httpRequest);
        try {
            httpDecoder.setDiscardThreshold(0);
            Map<String, Object> params = new HashMap<>();
            List<InterfaceHttpData> InterfaceHttpDataList = httpDecoder.getBodyHttpDatas();
            for (InterfaceHttpData data : InterfaceHttpDataList) {
                if (data != null) {
                    if (InterfaceHttpData.HttpDataType.FileUpload == data.getHttpDataType()) {
                        FileUpload fileUpload = (FileUpload) data;
                        byte[] bytes = fileUpload.get();
                        if (!fileUpload.isInMemory()) {
                            fileUpload.delete();
                        }
                        params.put(data.getName(), bytes);
                    }
                    if (data.getHttpDataType() == InterfaceHttpData.HttpDataType.Attribute) {
                        Attribute attribute = (Attribute) data;
                        params.put(attribute.getName(), attribute.getValue());
                    }
                }
            }
            return params;
        } finally {
            System.out.println("74------------->" + httpRequest.refCnt());
            httpDecoder.destroy();
            System.out.println("76------------>" + httpRequest.refCnt());
        }
}

where 74--------------> is 4, and 76----------------->is 3

Netty version

4.1.51.Final

JVM version (e.g. java -version)

1.8

OS version (e.g. uname -a)

MAC

Activity

  1. aepedraza commented on Mar 24, 2021

    @aepedraza

    Hi, I'm trying to reproduce your code in a unit test but I'm not finding ContentConverterException nor UploadedFile. My version is 4.1.61.Final-SNAPSHOT (currently the last in branch 4.1) Are those some "theoretical classes" or is this in a previous version?

    Thanks in advance for your time

    Disclaimer: This may be my first contribution to the project. Please forgive my newbie questions

  2. devilyard commented on Mar 24, 2021

    @devilyard
    Author

    Hi, I'm trying to reproduce your code in a unit test but I'm not finding ContentConverterException nor UploadedFile. My version is 4.1.61.Final-SNAPSHOT (currently the last in branch 4.1) Are those some "theoretical classes" or is this in a previous version?

    Thanks in advance for your time

    Disclaimer: This may be my first contribution to the project. Please forgive my newbie questions

    thank you for replying. sorry for my posting, UploadedFile and ContentConverterException is my local definition, I will modify the code.

  3. aepedraza commented on Mar 27, 2021

    @aepedraza

    Hi @devilyard I'm terribly sorry for not being able to read your code correctly, but file variable is not being resolved in convert(), did you meant bytes declared before and not being used after?

    byte[] bytes = fileUpload.get();
    if (!fileUpload.isInMemory()) {
        fileUpload.delete();
    }
    params.put(data.getName(), file);
    
  4. fredericBregier commented on Mar 27, 2021

    @fredericBregier
    Member

    The API says:

     /**
       * Remove all InterfaceHttpData from virtual File storage from clean list for all requests
       */
      void cleanAllHttpData();
    

    So MemoryAttribute is not concerned by this method.

    Just for note, for both Memory or Disk or Mixed mode factories, the release has to be done as:

      for (InterfaceHttpData httpData: decoder.getBodyHttpDatas()) {
          httpData.release();
          factory.removeHttpDataFromClean(request, httpData);
      }
      factory.cleanAllHttpData();
      decoder.destroy();
    
  5. jameskleeh commented on Apr 6, 2021

    @jameskleeh
    Contributor

    Relates to #8640

  6. fredericBregier commented on Apr 6, 2021

    @fredericBregier
    Member

    @jameskleeh Yes and no... It is the current behavior.
    I give the right way to clean up, whatever Memory, Disk or Mixed mode in my last comment.

  7. aepedraza commented on Apr 7, 2021

    @aepedraza

    @fredericBregier So, should this issue be discarded?

  8. fredericBregier commented on Apr 7, 2021

    @fredericBregier
    Member

    @aepedraza I think so.
    Note however on your example code:

     byte[] bytes = fileUpload.get(); // This get all content from File if any to memory (could lead to Out Of Memory)
     if (!fileUpload.isInMemory()) {
         fileUpload.delete();
     }
     params.put(data.getName(), file) // I don't know to what file point to?
    

    I would recommend to free Memory, Mixed, Disk based factory as the following, once all stuffs are done.

     for (InterfaceHttpData httpData: decoder.getBodyHttpDatas()) {
           httpData.release();
           factory.removeHttpDataFromClean(request, httpData);
     }
     factory.cleanAllHttpData();
     decoder.destroy();
    
  9. chrisvest commented on Apr 8, 2021

    @chrisvest
    Member

    @devilyard (ignore my previous comment – the web page was out of date when I posted) Does Frederics help solve the issue you are seeing? If so, then I think we can close this issue.

  10. devilyard commented on Apr 8, 2021

    @devilyard
    Author

    @devilyard (ignore my previous comment – the web page was out of date when I posted) Does Frederics help solve the issue you are seeing? If so, then I think we can close this issue.

    yes, i tried the latest 4. 1. 62 version, the issue is fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions