Repository: netty/netty
Issue: HttpPostRequestDecoder doesn't work if last disposition delimiter doesn't end with CRLF

Some Adobe Flash based uploaders such as SWFUpload doesn't end the last disposition delimiter with CRLF. 

> SWFUpload sends this:
> ------------ei4ae0GI3Ef1gL6Ij5gL6gL6ei4KM7--
> 
> instead of this:
> ------------ei4ae0GI3Ef1gL6Ij5gL6gL6ei4KM7--\r\n

So multipart uploads always fail with an "ErrorDataDecoderException: No Multipart delimiter found". 

We can make it work with some tricks, such as this:

``` java
    private static final HttpChunk CRLF = 
        new DefaultHttpChunk(copiedBuffer("\r\n", Charset.forName("UTF-8")));
    decoder.offer(CRLF);
    decoder.offer(HttpChunk.LAST_CHUNK);
```

Always offering the CRLF before the LAST_CHUNK, and we transform all HttpRequests in chunked requests to make this work.

I found that the problem is in readline method (line 1247 of HttpPostRequestDecoder.java) "private String readLine() throws 
NotEnoughDataDecoderException":

``` java
    if (nextByte == HttpConstants.CR) {
        if (sao.pos < sao.limit) {
            nextByte = sao.bytes[sao.pos ++];
               if (nextByte == HttpConstants.LF) {
                  sao.setReadPosition(0);
                  return sb.toString();
               }
        } else {
            sb.append((char) nextByte);
        }
    } else ....
```

If the LF is not found, the end of the method runs:

``` java
   undecodedChunk.readerIndex(readerIndex);
   throw new NotEnoughDataDecoderException();
```

Maybe it would be better to look for the last "--" after the delimiter "------------ei4ae0GI3Ef1gL6Ij5gL6gL6ei4KM7" instead of the CRLF.


Issue comments:
I understand your concern, but according to RFC 1867, chapter 5.9 :

```
5.9 CRLF used as line separator

  As with all MIME transmissions, CRLF is used as the separator for
  lines in a POST of the data in multipart/form-data.
```

So my guess is that the Adobe Flash Uploader code is incorrect... ?
The issue is that if we look for only the last '--' then what happens with conforming upload where there is a 'crlf' after the last '--'... ? We will have then issues for all other clients/servers since those 2 caracters will not be ignored...
I don't see yet a good way of doing...

Perhaps look for the CRLF. If that's not found, back up a few characters and look for the -- as well?

Of course, you'd have to log a warning stating that an application is not conforming to the standards ;)

@CruzBishop +1

OK, I will try later today to see what I can propose...

I propose a fix for issue #434 to provide the ability to stop even if no CRLF is provided since some clients as Adobe Flash will not finish the closing delimiter with a CRLF.

Note however that there is one "hole" in the proposal:
- if one chunk is enough to fullfill the closing delimiter but not enough to get the CRLF
- but the next chunk will provide this CRLF
  => the code may be wrong (while it should ignore the next CRLF from the next chunk).

If you have a better idea (the code shows 2 // FIXME as comments)

I figured the problem would be the multipart upload implementation of the Flash Uploader. But I see you've finally found a solution. Great! Just in one day!

I suppose it is not fair that servers have to fix client pitfalls.

So, thanks a lot.


Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/org/jboss/netty/example/http/upload/HttpUploadServerHandler.java b/src/main/java/org/jboss/netty/example/http/upload/HttpUploadServerHandler.java
index 4e844afdfbf..5a23e729f60 100644
--- a/src/main/java/org/jboss/netty/example/http/upload/HttpUploadServerHandler.java
+++ b/src/main/java/org/jboss/netty/example/http/upload/HttpUploadServerHandler.java
@@ -360,9 +360,10 @@ private void writeResponse(Channel channel) {
             CookieEncoder cookieEncoder = new CookieEncoder(true);
             for (Cookie cookie: cookies) {
                 cookieEncoder.addCookie(cookie);
+                response.addHeader(HttpHeaders.Names.SET_COOKIE, cookieEncoder
+                        .encode());
+                cookieEncoder = new CookieEncoder(true);
             }
-            response.addHeader(HttpHeaders.Names.SET_COOKIE, cookieEncoder
-                    .encode());
         }
         // Write the response.
         ChannelFuture future = channel.write(response);
diff --git a/src/main/java/org/jboss/netty/handler/codec/http/multipart/HttpPostRequestDecoder.java b/src/main/java/org/jboss/netty/handler/codec/http/multipart/HttpPostRequestDecoder.java
index 30826d90f9f..324c9c6639a 100644
--- a/src/main/java/org/jboss/netty/handler/codec/http/multipart/HttpPostRequestDecoder.java
+++ b/src/main/java/org/jboss/netty/handler/codec/http/multipart/HttpPostRequestDecoder.java
@@ -894,7 +894,7 @@ private InterfaceHttpData findMultipartDelimiter(String delimiter,
         skipOneLine();
         String newline;
         try {
-            newline = readLine();
+            newline = readDelimiter(delimiter);
         } catch (NotEnoughDataDecoderException e) {
             undecodedChunk.readerIndex(readerIndex);
             return null;
@@ -1281,6 +1281,202 @@ private String readLine() throws NotEnoughDataDecoderException {
         throw new NotEnoughDataDecoderException();
     }
 
+	/**
+     * Read one line up to --delimiter or --delimiter-- and if existing the CRLF or LF
+     * Read one line up to --delimiter or --delimiter-- and if existing the CRLF or LF.
+     * Note that CRLF or LF are mandatory for opening delimiter (--delimiter) but not for
+     * closing delimiter (--delimiter--) since some clients does not include CRLF in this case.
+     * 
+     * @param delimiter of the form --string, such that '--' is already included
+     * @return the String from one line as the delimiter searched (opening or closing)
+     * @throws NotEnoughDataDecoderException Need more chunks and
+     *   reset the readerInder to the previous value
+     */
+    private String readDelimiterStandard(String delimiter) throws NotEnoughDataDecoderException {
+        int readerIndex = undecodedChunk.readerIndex();
+        try {
+            StringBuilder sb = new StringBuilder(64);
+            int delimiterPos = 0;
+            int len = delimiter.length();
+            while (undecodedChunk.readable() && delimiterPos < len) {
+                byte nextByte = undecodedChunk.readByte();
+                if (nextByte == delimiter.charAt(delimiterPos)) {
+                	delimiterPos++;
+                    sb.append((char) nextByte);
+                } else {
+                	// delimiter not found so break here !
+                	undecodedChunk.readerIndex(readerIndex);
+                    throw new NotEnoughDataDecoderException();
+                }
+            }
+            // Now check if either opening delimiter or closing delimiter
+            if (undecodedChunk.readable()) {
+            	byte nextByte = undecodedChunk.readByte();
+                // first check for opening delimiter
+                if (nextByte == HttpConstants.CR) {
+                    nextByte = undecodedChunk.readByte();
+                    if (nextByte == HttpConstants.LF) {
+                        return sb.toString();
+                    } else {
+                    	// error since CR must be followed by LF
+                    	// delimiter not found so break here !
+                    	undecodedChunk.readerIndex(readerIndex);
+                        throw new NotEnoughDataDecoderException();
+                    }
+                } else if (nextByte == HttpConstants.LF) {
+                    return sb.toString();
+                } else if (nextByte == '-') {
+                    sb.append((char) nextByte);
+                    // second check for closing delimiter
+                    nextByte = undecodedChunk.readByte();
+                    if (nextByte == '-') {
+                        sb.append((char) nextByte);
+                        // now try to find if CRLF or LF there
+                        if (undecodedChunk.readable()) {
+                            nextByte = undecodedChunk.readByte();
+                            if (nextByte == HttpConstants.CR) {
+                                nextByte = undecodedChunk.readByte();
+                                if (nextByte == HttpConstants.LF) {
+                                    return sb.toString();
+                                } else {
+                                	// error CR without LF
+                                	// delimiter not found so break here !
+                                	undecodedChunk.readerIndex(readerIndex);
+                                    throw new NotEnoughDataDecoderException();
+                                }
+                            } else if (nextByte == HttpConstants.LF) {
+                                return sb.toString();
+                            } else {
+                            	// No CRLF but ok however (Adobe Flash uploader)
+                            	// minus 1 since we read one char ahead but should not
+                            	undecodedChunk.readerIndex(undecodedChunk.readerIndex()-1);
+                                return sb.toString();
+                            }
+                        }
+                        // FIXME what do we do here? 
+                        // either considering it is fine, either waiting for more data to come?
+                        // lets try considering it is fine...
+                        return sb.toString();
+                    }
+                	// only one '-' => not enough
+                    // whatever now => error since incomplete
+                }
+            }
+        } catch (IndexOutOfBoundsException e) {
+            undecodedChunk.readerIndex(readerIndex);
+            throw new NotEnoughDataDecoderException(e);
+        }
+        undecodedChunk.readerIndex(readerIndex);
+        throw new NotEnoughDataDecoderException();
+    }
+    /**
+     * Read one line up to --delimiter or --delimiter-- and if existing the CRLF or LF.
+     * Note that CRLF or LF are mandatory for opening delimiter (--delimiter) but not for
+     * closing delimiter (--delimiter--) since some clients does not include CRLF in this case.
+     * 
+     * @param delimiter of the form --string, such that '--' is already included
+     * @return the String from one line as the delimiter searched (opening or closing)
+     * @throws NotEnoughDataDecoderException Need more chunks and
+     * reset the readerInder to the previous value
+     */
+    private String readDelimiter(String delimiter) throws NotEnoughDataDecoderException {
+        SeekAheadOptimize sao = null;
+        try {
+            sao = new SeekAheadOptimize(undecodedChunk);
+        } catch (SeekAheadNoBackArrayException e1) {
+            return readDelimiterStandard(delimiter);
+        }
+        int readerIndex = undecodedChunk.readerIndex();
+        int delimiterPos = 0;
+        int len = delimiter.length();
+        try {
+            StringBuilder sb = new StringBuilder(64);
+            // check conformity with delimiter
+            while (sao.pos < sao.limit && delimiterPos < len) {
+                byte nextByte = sao.bytes[sao.pos ++];
+                if (nextByte == delimiter.charAt(delimiterPos)) {
+                	delimiterPos++;
+                    sb.append((char) nextByte);
+                } else {
+                	// delimiter not found so break here !
+                	undecodedChunk.readerIndex(readerIndex);
+                    throw new NotEnoughDataDecoderException();
+                }
+            }
+            // Now check if either opening delimiter or closing delimiter
+            if (sao.pos < sao.limit) {
+                byte nextByte = sao.bytes[sao.pos ++];
+                if (nextByte == HttpConstants.CR) {
+                    // first check for opening delimiter
+                    if (sao.pos < sao.limit) {
+                        nextByte = sao.bytes[sao.pos ++];
+                        if (nextByte == HttpConstants.LF) {
+                            sao.setReadPosition(0);
+                            return sb.toString();
+                        }
+                    } else {
+                    	// error since CR must be followed by LF
+                    	// delimiter not found so break here !
+                    	undecodedChunk.readerIndex(readerIndex);
+                        throw new NotEnoughDataDecoderException();
+                    }
+                } else if (nextByte == HttpConstants.LF) {
+                    // same first check for opening delimiter where LF used with no CR
+                    sao.setReadPosition(0);
+                    return sb.toString();
+                } else if (nextByte == '-') {
+                    sb.append((char) nextByte);
+                    // second check for closing delimiter
+                	if (sao.pos < sao.limit) {
+                        nextByte = sao.bytes[sao.pos ++];
+                        if (nextByte == '-') {
+                            sb.append((char) nextByte);
+                            // now try to find if CRLF or LF there
+                            if (sao.pos < sao.limit) {
+                                nextByte = sao.bytes[sao.pos ++];
+	                            if (nextByte == HttpConstants.CR) {
+	                                if (sao.pos < sao.limit) {
+	                                    nextByte = sao.bytes[sao.pos ++];
+	                                    if (nextByte == HttpConstants.LF) {
+	                                        sao.setReadPosition(0);
+	                                        return sb.toString();
+	                                    }
+	                                } else {
+	                                	// error CR without LF
+	                                	// delimiter not found so break here !
+	                                	undecodedChunk.readerIndex(readerIndex);
+	                                    throw new NotEnoughDataDecoderException();
+	                                }
+	                            } else if (nextByte == HttpConstants.LF) {
+	                                sao.setReadPosition(0);
+	                                return sb.toString();
+	                            } else {
+	                            	// No CRLF but ok however (Adobe Flash uploader)
+	                            	// minus 1 since we read one char ahead but should not
+	                                sao.setReadPosition(1);
+	                                return sb.toString();
+	                            }
+                            }
+                            // FIXME what do we do here? 
+                            // either considering it is fine, either waiting for more data to come?
+                            // lets try considering it is fine...
+                            sao.setReadPosition(0);
+                            return sb.toString();
+                        }
+                        // whatever now => error since incomplete
+                    	// only one '-' => not enough or whatever not enough element
+                    }
+                }
+            }
+        } catch (IndexOutOfBoundsException e) {
+            undecodedChunk.readerIndex(readerIndex);
+            throw new NotEnoughDataDecoderException(e);
+        }
+        undecodedChunk.readerIndex(readerIndex);
+        throw new NotEnoughDataDecoderException();
+    }
+
+    
     /**
      * Read a FileUpload data as Byte (Binary) and add the bytes directly to the
      * FileUpload. If the delimiter is found, the FileUpload is completed.


Test patch: