fukamachi / fast-http

A fast HTTP request/response parser for Common Lisp.
343 stars 37 forks source link

INVALID-HEADER-TOKEN: invalid character in header #27

Closed knobo closed 7 years ago

knobo commented 8 years ago

Looks like an error in fast-http.

I have also attached the error message from my repl. lisp-error.txt

And this is how to reproduce the error:

// filename  /tmp/tmp.js

var app = angular.module('upl', ['ngFileUpload']);
  app.controller('myCtrl', function(Upload) {
    var file = new Blob([new Uint8Array(1000000)], {type: 'application/octet-stream'});

    Upload.upload({
      url: '/upload',
      data: {file: file}
    }).then(
      function (resp){
    console.log('Success ' + resp.config.data.file.name + 'uploaded. Response: ' + resp.data);
      },
      function (resp) {
    console.log('Error status: ' + resp.status);
      },
      function (evt) {
    var progressPercentage = parseInt(100.0 * evt.loaded / evt.total);
        console.log('progress: ' + progressPercentage + '% ' + evt.config.data.file.name);
      });
});
;; (ql:quickload "clack")
;; (ql:quickload "ningle")
;; (ql:quickload "cl-who")

(defpackage upload
  (:use :cl :cl-who :clack))

(in-package :upload)
(setf (html-mode) :html5)

(defun page (&optional arg &rest args)
  (declare (ignore arg args))
  (with-html-output-to-string (s nil :indent t :prologue t)
    (:html :ng-app "upl"
     (:head
      (:title "upload-test")
      (:meta :charset "UTF-8")
      (:meta :http-equiv "cache-control" :content "no-cache")
      (:script :src "https://ajax.googleapis.com/ajax/libs/angularjs/1.4.5/angular.min.js")
      (:script :src "https://cdnjs.cloudflare.com/ajax/libs/danialfarid-angular-file-upload/12.0.4/ng-file-upload-all.min.js")
      (:script :src "https://cdnjs.cloudflare.com/ajax/libs/danialfarid-angular-file-upload/12.0.4/FileAPI.js")
      (:script :src "/tmp.js"))
      (:body :ng-controller "myCtrl"
         "Uploading.. Wating for server to crash"))))

(defvar *app* (make-instance 'ningle:<app>))

(setf (ningle:route *app* "/")
      (lambda (&rest args)(declare (ignore args)) (page)))

(setf (ningle:route *app* "/tmp.js")
      #p"/tmp/tmp.js")

(setf (ningle:route *app* "/upload" :method :post)
      #'(lambda (&rest args)
      (declare (ignore args))
      "{\"Status\":\"OK\"}"))

(defvar *handler* nil)

(when *handler*
  (clack:stop *handler*))

(setf *handler*
      (clack:clackup 
       *app*
       :server :woo))
rudolph-miller commented 8 years ago

@knobo

I tried code above, but I can not reproduce the error (can upload successfully) . ;( Can you try to upgrade your Quicklisp dist and retry code above?

knobo commented 8 years ago

I already have the latest version of quicklisp and dists. But, which browser are you using? Maybe it is browser specific. I use firefox and chrome on linux.

Here is the dump of what's send over the network. Is it similar to what your client is sending. wireshark.dump.txt

I have tried with ccl and sbcl.

########################################################################## The most up-to-date client, version 2016-02-22, is already installed. T CL-USER> (ql:update-all-dists) 1 dist to check. Downloading http://beta.quicklisp.org/dist/quicklisp.txt ########################################################################## You already have the latest version of "quicklisp": 2016-04-21. NIL

knobo commented 8 years ago

Maybe try to increase the blob size to see if you get the error.

knobo commented 8 years ago

Now I got a different error also. I don't know the conditions that makes this happen though. It has happend several times now, when I have tried a new debugging code. I'll put the debugging code in a new comment.

INVALID-BOUNDARY: invalid boundary
   [Condition of type FAST-HTTP.ERROR:INVALID-BOUNDARY]

Restarts:
 0: [ABORT] abort thread (#<THREAD "clack-handler-woo" RUNNING {10086C1DA3}>)

Backtrace:
  0: (FAST-HTTP.MULTIPART-PARSER:HTTP-MULTIPART-PARSE #S(FAST-HTTP.MULTIPART-PARSER:LL-MULTIPART-PARSER :STATE 2 :HEADER-PARSER #S(FAST-HTTP.HTTP:HTTP :METHOD NIL :MAJOR-VERSION 0 :MINOR-VERSION 9 :STATUS ..
      Locals:
        #:.DEFAULTING-TEMP. = 0
        #:.DEFAULTING-TEMP.#1 = NIL
        CALLBACKS = #S(FAST-HTTP.PARSER:CALLBACKS ..)
        DATA = #(45 45 45 45 45 45 ...)
        PARSER = #S(FAST-HTTP.MULTIPART-PARSER:LL-MULTIPART-PARSER ..)
  1: ((LAMBDA (FAST-HTTP::DATA) :IN FAST-HTTP:MAKE-MULTIPART-PARSER) #(45 45 45 45 45 45 ...))
      Locals:
        SB-DEBUG::ARG-0 = #(45 45 45 45 45 45 ...)
  2: (HTTP-BODY.MULTIPART:MULTIPART-PARSE "multipart/form-data; boundary=---------------------------1799159197184904671830863343" 32816 #<FLEXI-STREAMS::VECTOR-INPUT-STREAM {10094600F3}>)
      Locals:
        SB-DEBUG::ARG-0 = "multipart/form-data; boundary=---------------------------1799159197184904671830863343"
        SB-DEBUG::ARG-1 = 32816
        SB-DEBUG::ARG-2 = #<FLEXI-STREAMS::VECTOR-INPUT-STREAM {10094600F3}>
  3: (HTTP-BODY:PARSE "multipart/form-data; boundary=---------------------------1799159197184904671830863343" 32816 #<FLEXI-STREAMS::VECTOR-INPUT-STREAM {10094600F3}>)
      Locals:
        SB-DEBUG::ARG-0 = "multipart/form-data; boundary=---------------------------1799159197184904671830863343"
        SB-DEBUG::ARG-1 = 32816
        SB-DEBUG::ARG-2 = #<FLEXI-STREAMS::VECTOR-INPUT-STREAM {10094600F3}>
knobo commented 8 years ago

Debug-server-crash-code to find the size where the server crashes.

This is the changes to the page function

(defun page (&optional arg &rest args)
  (declare (ignore arg args))
  (with-html-output-to-string (s nil :indent t :prologue t)
    (:html :ng-app "upl"
     (:head
      (:title "upload-test")
      (:meta :charset "UTF-8")
      (:meta :http-equiv "cache-control" :content "no-cache")
      (:script :src "https://ajax.googleapis.com/ajax/libs/angularjs/1.4.5/angular.min.js")
      (:script :src "https://cdnjs.cloudflare.com/ajax/libs/danialfarid-angular-file-upload/12.0.4/ng-file-upload-all.min.js")
      (:script :src "https://cdnjs.cloudflare.com/ajax/libs/danialfarid-angular-file-upload/12.0.4/FileAPI.js")
      (:script :src "/tmp.js"))
      (:body :ng-controller "myCtrl"
         "Uploading.. Wating for server to crash"
         (:br)(:label "Step 10 more bytes")
         (:input :type "number" :ng-click "run()" :ng-model "size" :step "10")
         (:br)(:label "Step 100 more bytes")         

         (:input :type "number" :ng-click "run()" :ng-model "size" :step "100")
         (:br)(:label "Step 1000 more bytes")

         (:input :type "number" :ng-click "run()" :ng-model "size" :step "1000")))))

Javascript file:

// Filename /tmp/tmp.js
var app = angular.module('upl', ['ngFileUpload']);

app.controller('myCtrl', function($scope, Upload) {
  $scope.size = 32499;

  $scope.run  = function () {
    var file = new Blob([new Uint8Array($scope.size)], {type: 'application/octet-stream'});

    Upload.upload({
      url: '/upload',
      data: {file: file}
    }).then(
      function (resp){
    console.log('Success ' + resp.config.data.file.name + 'uploaded. Response: ' + resp.data);
      },
      function (resp) {
    console.log('Error status: ' + resp.status);
      },
      function (evt) {
    var progressPercentage = parseInt(100.0 * evt.loaded / evt.total);
        console.log('progress: ' + progressPercentage + '% ' + evt.config.data.file.name);
      });
  };
});
knobo commented 8 years ago

I don't get the error when the blob is small enough.

rudolph-miller commented 8 years ago

I retried to reproduce the error with the code above, but I could not. ;(

Seeing the stacktrace above, INVALID-BOUNDARY is raised when the state of parser is +parsing-delimiter+ (https://github.com/fukamachi/fast-http/blob/master/src/multipart-parser.lisp#L128) and there are 2 possibilities (https://github.com/fukamachi/fast-http/blob/master/src/multipart-parser.lisp#L150, https://github.com/fukamachi/fast-http/blob/master/src/multipart-parser.lisp#L166).

knobo commented 8 years ago

Now I have tried this on some other computers and a virtual machine. I also compiled my own version of libev4 just to check if it was relevant. On debian stable I had to increase the upload size to 66000 bytes before the error occurred. How big file did you try?

knobo commented 8 years ago

At some point FAST-HTTP.PARSER::READ-BODY-DATA returns wrong END. I think it should be (48000+212). But my trace shows
2: FAST-HTTP.PARSER::READ-BODY-DATA returned 26916 T

I have a slow laptop from long time ago. Maybe there is a racecondition that triggers in this old laptop.

knobo commented 8 years ago

Now, I upload 33000 bytes, and body-callback is called with

(#S(FAST-HTTP.HTTP:HTTP-REQUEST
                                :METHOD POST
                                :MAJOR-VERSION 1
                                :MINOR-VERSION 1
                                :STATUS 0
                                :CONTENT-LENGTH 10739
                                :CHUNKED-P NIL
                                :UPGRADE-P NIL
                                :HEADERS #<HASH-TABLE
                                           :TEST EQUAL :COUNT 10
                                           {1009E55F83}>
                                :HEADER-READ 0
                                :MARK 489
                                :STATE 3
                                :RESOURCE /upload)

Shouldn't content-length be 33212 + headers? fast-http.parser::read-body-data is called 2 times. Shouldn't it be only once?

Looks like make-parser is called with a to small end value. So the bug is maybe in woo:read-cb?

(this is from a different upload, and it looks like the end number is different every time.) (LAMBDA (FAST-HTTP::DATA &KEY :START :END) :IN FAST-HTTP:MAKE-PARSER) #(80 79 83 84 32 47 ...) :START 0 :END 32768)

hmmm.. Yes it looks like woo does not read the whole request, and runs the parser to times.

So the question is then: is the bug in woo or libev? And is the bug reproducible any on other system then ubuntu/debian/archlinux

knobo commented 8 years ago

Yes. Because when I use :server fcgi I don't get this problem. And it still uses fast-http. So is it possible to move an issue, or do I have to close it and open a new one on woo?

rudolph-miller commented 8 years ago

No, it's not. ;( You should attach th link to this issue to the new one, please. :)

knobo commented 8 years ago

Or should the parser see that see that the buffer is shorter then content-length, and return a continuation that should be used on the rest. And woo uses that it if a function is returned.