Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid HttpRequest kills HttpServer #3222

Closed
DartBot opened this issue May 25, 2012 · 11 comments
Closed

Invalid HttpRequest kills HttpServer #3222

DartBot opened this issue May 25, 2012 · 11 comments
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-as-intended Closed as the reported issue is expected behavior library-io
Milestone

Comments

@DartBot
Copy link

DartBot commented May 25, 2012

This issue was originally filed by [email protected]


I was doing 'telnet localhost 8000' and the Dart HttpServer crashes on some simple mal-formed HTTP:

GET /

and got:

Unhandled exception:
HttpParserException: Invalid request URI
 0. Function: '[email protected]' url: 'dart:io' line:510 col:388
 1. Function: '_HttpConnection@19ffb8b9._onConnectionClosed@19ffb8b9' url: 'dart:io' line:497 col:133
 2. Function: '_HttpConnectionBase@19ffb8b9._onError@19ffb8b9' url: 'dart:io' line:487 col:237
 3. Function: '[email protected]' url: 'dart:io' line:497 col:1
 4. Function: '[email protected]' url: 'dart:io' line:673 col:22
 5. Function: '_HttpConnectionBase@19ffb8b9._onData@19ffb8b9' url: 'dart:io' line:486 col:71
 6. Function: '_SocketBase@19ffb8b9._multiplex@19ffb8b9' url: 'dart:io' line:926 col:1
 7. Function: '[email protected]' url: 'dart:io' line:942 col:142
 8. Function: '_ReceivePortImpl@38422f21._handleMessage@38422f21' url: 'dart:isolate' line:19 col:46

GET / h

listening on 8000...
Unhandled exception:
HttpParserException: Failed to parse HTTP
 0. Function: '[email protected]' url: 'dart:io' line:510 col:388
 1. Function: '_HttpConnection@19ffb8b9._onConnectionClosed@19ffb8b9' url: 'dart:io' line:497 col:133
 2. Function: '_HttpConnectionBase@19ffb8b9._onError@19ffb8b9' url: 'dart:io' line:487 col:237
 3. Function: '[email protected]' url: 'dart:io' line:497 col:1
 4. Function: '[email protected]' url: 'dart:io' line:673 col:22
 5. Function: '_HttpConnectionBase@19ffb8b9._onData@19ffb8b9' url: 'dart:io' line:486 col:71
 6. Function: '_SocketBase@19ffb8b9._multiplex@19ffb8b9' url: 'dart:io' line:926 col:1
 7. Function: '[email protected]' url: 'dart:io' line:942 col:142
 8. Function: '_ReceivePortImpl@38422f21._handleMessage@38422f21' url: 'dart:isolate' line:19 col:46

The Http Server should be resilient enough not to crash when it receives invalid HTTP.
If throwing these exceptions is expected behaviour how do we catch it and prevent it from taking the server down?

@DartBot
Copy link
Author

DartBot commented May 25, 2012

This comment was originally written by [email protected]


Also Dart can't handle reverse proxy requests from nginx (a very popular story we should be supporting), this is the StackTrace I get:

Unhandled exception:
HttpException: Content length required for HTTP 1.0
 0. Function: '_HttpResponse@19ffb8b9._writeHeader@19ffb8b9' url: 'dart:io' line:464 col:34
 1. Function: '_HttpRequestResponseBase@19ffb8b9._ensureHeadersSent@19ffb8b9' url: 'dart:io' line:417 col:37
 2. Function: '_HttpResponse@19ffb8b9._responseEnd@19ffb8b9' url: 'dart:io' line:445 col:68
 3. Function: '_HttpResponse@19ffb8b9._streamClose@19ffb8b9' url: 'dart:io' line:449 col:19
 4. Function: '[email protected]' url: 'dart:io' line:476 col:60
 5. Function: '[email protected]' url: 'file:///home/mythz/src/DartRedisTodos/vendor/Express/Express.dart' line:278 col:27
 6. Function: '[email protected]' url: 'file:///home/mythz/src/DartRedisTodos/vendor/Express/Express.dart' line:300 col:11
 7. Function: 'StaticFileHandler.function' url: 'file:///home/mythz/src/DartRedisTodos/vendor/Express/Express.dart' line:385 col:21
 8. Function: 'FutureImpl._complete@367218ca' url: 'bootstrap_impl' line:557 col:40
 9. Function: 'FutureImpl._setValue@367218ca' url: 'bootstrap_impl' line:561 col:1
 10. Function: 'CompleterImpl.complete' url: 'bootstrap_impl' line:573 col:62
 11. Function: 'FutureImpl.function' url: 'bootstrap_impl' line:565 col:39
 12. Function: 'FutureImpl._complete@367218ca' url: 'bootstrap_impl' line:557 col:40
 13. Function: 'FutureImpl._setValue@367218ca' url: 'bootstrap_impl' line:561 col:1
 14. Function: 'CompleterImpl.complete' url: 'bootstrap_impl' line:573 col:62
 15. Function: '[email protected]' url: 'dart:isolate' line:22 col:356
 16. Function: '_ReceivePortImpl@38422f21._handleMessage@38422f21' url: 'dart:isolate' line:19 col:46

@kasperl
Copy link

kasperl commented May 25, 2012

Added Area-IO, Triaged labels.

@madsager
Copy link
Contributor

Maybe instead of throwing exceptions we should propagate the exception object to the HttpServer onError handler. Then the HttpServer user is in control and can ignore the error, log it, or shut down the server as he sees fit. I agree that the server should not stop because someone fires an invalid request at it.

What do you think Søren?


Set owner to @sgjesse.

@sgjesse
Copy link
Contributor

sgjesse commented May 25, 2012

We need a simple way of handling errors during request processing that goes for both exceptions during the HTTP parsing and exceptions while actually running the request handler.

I think there should be some default handling that will return a response with status code 400 (Bad Request) if HTTP parsing fails and status code 500 (Internal Server Error) if an exception is thrown in the request handler. In both these cases the underlying socket connection should be closed after the response has been sent.


Added Started label.

@sgjesse
Copy link
Contributor

sgjesse commented May 25, 2012

Regarding comment #­1:

The reason for this failure is that the nginx reverse proxy talks HTTP 1.0 with the Dart server. For HTTP 1.0 the content length header needs to be set before sending any content. At the moment an exception is thrown if content length is not set before sending data for HTTP 1.0. With the following nginx configuration:

        location /dart {
                proxy_pass http://127.0.0.1:1234/;
                proxy_set_header Host $host;
        }

And this Dart code

void main() {
  HttpServer server = new HttpServer();
  server.defaultRequestHandler = (req, resp) {
    print(req.headers);
    req.inputStream.onData = () {
      print(new String.fromCharCodes(req.inputStream.read()));
    };
    req.inputStream.onClosed = () {
      resp.contentLength = 3;
      resp.outputStream.writeString("XXX");
      resp.outputStream.close();
    };
  };
  server.listen('0.0.0.0', 1234);
}

the nginx reverse proxy configuration works when navigating to http://localhost/dart/xxx.

Nginx have the configuration proxy_http_version which can be set to 1.1 which might also help. However it requires nginx 1.1.4, which I don't have at hand.

We could consider to buffer all output for HTTP 1.0 if the content length is not set and send it all with content length when available.

@DartBot
Copy link
Author

DartBot commented May 26, 2012

This comment was originally written by [email protected]


I have nginx 1.1.9 and can confirm that this works:

location / {
   ...
   proxy_http_version 1.1;
}

Nginx + Dart at: http://dart-todos.ubixar.com :)

@sgjesse
Copy link
Contributor

sgjesse commented May 29, 2012

The main reason for the Dart HttpServer throwing an uncaught exception and terminating in these error cases is that there is no error handler set on the HttpServer instance. Adding that

  HttpServer server;
  ...
  server.onError = (e) => print(e);

should print the error and keep the server running. It will not return an error response to the client just close the underlying socket connection.

@madsager
Copy link
Contributor

madsager commented Jun 7, 2012

Added this to the M1 milestone.

@DartBot
Copy link
Author

DartBot commented Aug 27, 2012

This comment was originally written by [email protected]


Regarding comment #­7:

Setting the "proxy_http_version 1.1;" does not work for me.

dart code:

var server = new HttpServer();
server.defaultRequestHandler = defaultHandler(HttpRequest req, HttpResponse res) {
print("start processing request");
res.outputStream..writeString("<html><head><title>The title</title></head><body><div>The body message</div></body></html>")
                    ..close();
    print("stop processing request");
};
  server.listen("127.0.0.1", 8080);
  server.onError = handleHttpError(Exception ex) {
    print(ex.toString());
  };

nginx configuration:
location /about {
    proxy_pass http://localhost:8080;
    proxy_set_header X-Real-IP $remote_addr;
    proxy_set_header Host $http_host;
    proxy_http_version 1.1;
}

1 in 10 page reloads it works but in most cases the nginx returns the 502 http exception.

tested both on windows7-64 and ubunty

@madsager
Copy link
Contributor

madsager commented Sep 3, 2012

The original bug report here was that the HttpServer would die with an unhandled exception on invalid requests if no error handler was registered. This is working as designed. You should always register an error handler on your HttpServer.

Please file new bugs for other issues so we can track them properly. Thanks.


Added AsDesigned label.

@kevmoo
Copy link
Member

kevmoo commented May 14, 2014

Removed Area-IO label.
Added Area-Library, Library-IO labels.

@DartBot DartBot added Type-Defect area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io closed-as-intended Closed as the reported issue is expected behavior labels May 14, 2014
@DartBot DartBot added this to the M1 milestone May 14, 2014
copybara-service bot pushed a commit that referenced this issue Jun 13, 2022
Changes:
```
> git log --format="%C(auto) %h %s" 51435ef..c4e9ddc
 https://dart.googlesource.com/pub.git/+/c4e9ddc8 Extend retries for file-ops on windows (#3451)
 https://dart.googlesource.com/pub.git/+/6aeb1795 Upgrade package:lints to 2.0.0 (#3445)
 https://dart.googlesource.com/pub.git/+/73ea9a98 Roll tar to 0.5.5+1 (#3447)
 https://dart.googlesource.com/pub.git/+/2dc887fe Add env var for writing golden files (#3222)
 https://dart.googlesource.com/pub.git/+/764500b8 List all files in pub publish (#3440)
 https://dart.googlesource.com/pub.git/+/0b52e6a8 Remove debug print (#3441)
 https://dart.googlesource.com/pub.git/+/ea070238 Merge pull request #3443 from jonasfj/master
 https://dart.googlesource.com/pub.git/+/bffd1267 Merge branch 'cherry-pick-for-2.17.2'
 https://dart.googlesource.com/pub.git/+/c66381c5 Make `name` field of `_UserInfo` nullable. Fix #3424 (#3442)
 https://dart.googlesource.com/pub.git/+/cecc8e3c Handle broken response from userinfo_endpoint (#3427)
 https://dart.googlesource.com/pub.git/+/6c635040 Make `name` field of `_UserInfo` nullable. Fix #3424 (#3442)
 https://dart.googlesource.com/pub.git/+/6f20a94b Handle missing pubspec.lock in dependency_services list (#3439)
 https://dart.googlesource.com/pub.git/+/0ad17e84 Handle broken response from userinfo_endpoint (#3427)
 https://dart.googlesource.com/pub.git/+/153ef061 `pub add` create top-level attribute in block-style if possible. (#3423)
 https://dart.googlesource.com/pub.git/+/3b96f910 Handle error code 183 on windows (#3426)
 https://dart.googlesource.com/pub.git/+/9eb6428c Ignore `pubspec_overrides.yaml` for `publish` command (#3419)

```

Diff: https://dart.googlesource.com/pub.git/+/51435efcd574b7bc18d47a5dd620cb9759dea8f8~..c4e9ddc888c3aa89ef4462f0c4298929191e32b9/
Change-Id: I6dacb3e95c6399d3fb5cf340b5d0e5cded270684
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/247962
Commit-Queue: Sigurd Meldgaard <[email protected]>
Reviewed-by: Jonas Jensen <[email protected]>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-as-intended Closed as the reported issue is expected behavior library-io
Projects
None yet
Development

No branches or pull requests

5 participants