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

Fatal error when attempting to (wrongly) set incoming packet encoding #18118

Closed
iSkore opened this issue Jan 12, 2018 · 2 comments
Closed

Fatal error when attempting to (wrongly) set incoming packet encoding #18118

iSkore opened this issue Jan 12, 2018 · 2 comments
Labels
http Issues or PRs related to the http subsystem.

Comments

@iSkore
Copy link
Contributor

iSkore commented Jan 12, 2018

Error occurs in both versions of Node (likely because http_parser is the same version)

  • Version: v8.9.4
    Output from process.versions (v8.9.4)
  • http_parser: '2.7.0'
  • node: '8.9.4'
  • v8: '6.1.534.50'
  • nghttp2: '1.25.0'

  • Version: v9.4.0
    Output from process.versions (v9.4.0)
  • http_parser: '2.7.0'
  • node: '9.4.0'
  • v8: '6.2.414.46-node.17'
  • nghttp2: '1.29.0'

  • Platform: Darwin MacBook-Pro 17.3.0 Darwin Kernel Version 17.3.0: Thu Nov 9 18:09:22 PST 2017; root:xnu-4570.31.3~1/RELEASE_X86_64 x86_64
  • Subsystem: node_http_parser
  • Repeatability: constant

  • Error:

node[2066]: ../src/node_http_parser.cc:423:static void node::(anonymous namespace)::Parser::Execute(const FunctionCallbackInfo<v8::Value> &): Assertion `(Buffer::HasInstance(args[0])) == (true)' failed.
 1: node::Abort() [/usr/local/bin/node]
 2: node::(anonymous namespace)::DomainEnter(node::Environment*, v8::Local<v8::Object>) [/usr/local/bin/node]
 3: node::(anonymous namespace)::Parser::Execute(v8::FunctionCallbackInfo<v8::Value> const&) [/usr/local/bin/node]
 4: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [/usr/local/bin/node]
 5: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/usr/local/bin/node]
 6: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/usr/local/bin/node]
 7: 0x32860940463d
Abort trap: 6
  • Code:
'use strict';

const
	http   = require( 'http' ),
	port   = 3000,
	server = http.createServer( ( req, res ) => res.end( 'OK' ) ),
	inst   = server.listen( port,
		e => e ? console.error( e ) : console.log( `server is listening on ${port}` )
	);

process.on( 'beforeExit', console.log );
process.on( 'disconnect', console.log );
process.on( 'rejectionHandled', console.log );
process.on( 'uncaughtException', console.log );
process.on( 'unhandledRejection', console.log );
process.on( 'warning', console.log );

inst.on( 'connection', socket => {
	socket.setEncoding( 'utf8' );
	socket.on( 'data', d => {
		console.log( d );
	} );
} );
  • Steps to reproduce:
  • Started with the basic node index.js

Packet submitted to process:

GET / HTTP/1.1
Host: localhost:3000

Tested request with

  • postman
  • basic cli:
    printf "GET / HTTP/1.1\r\nHost: localhost:3000\r\n\r\n" > /dev/tcp/localhost/3000
  • browser
  • Behavior:

With the example code (above), the process immediately terminates when any request is made and prints out the fatal V8 error (above)
Since HTTP uses US-ASCII, I completely understand why an incoming packet (encoded differently) would break node_http_parser.
This fatal error bypasses all my attempts to catch errors on the process and crashes.

  • Summary:

I typically expect a clean error report, stackTrace, error code and all.
But this appears to be a fatal crash due to user error without being caught in the process level error handling.
I understand that setEncoding was meant for the response packet, but I believe this error report could lead to confusion.

I recommend blocking (and throwing and error) for any attempts to set encoding on an incoming packet/connection.

Ref: #18178
Ref: #19344

@bnoordhuis bnoordhuis added the http Issues or PRs related to the http subsystem. label Jan 13, 2018
iSkore added a commit to iSkore/node that referenced this issue Apr 3, 2018
added override to socket.setEncoding to not allow encoding changes for
incoming HTTP requests
added tests to ensure method throws JavaScript error
because an HTTP buffer must be in US-ASCII, this function should not
be allowed and should throw an Error
currently, the process encounters a fatal v8 error and crashes

error report detailed in
[issue nodejs#18118](nodejs#18118)

Fixes: nodejs#18118
Ref: nodejs#18178
iSkore added a commit to iSkore/node that referenced this issue Apr 3, 2018
added test to ensure setEncoding inside socket connection
would throw an error

Fixes: nodejs#18118
Ref: nodejs#18178
iSkore added a commit to iSkore/node that referenced this issue Apr 3, 2018
added ERR_HTTP_INCOMING_SOCKET_ENCODING error and error docs
throw ERR_HTTP_INCOMING_SOCKET_ENCODING error when incoming request
socket encoding is manipulated

error report detailed in nodejs#18118

Fixes: nodejs#18118
Ref: nodejs#18178
iSkore added a commit to iSkore/node that referenced this issue Apr 3, 2018
added note and link to RFC7230

>A recipient MUST parse an HTTP message as a sequence of octets in an
encoding that is a superset of US-ASCII [USASCII].

Ref: https://tools.ietf.org/html/rfc7230#section-3

Ref: nodejs#18118
Ref: nodejs#18178
Ref: nodejs#19344
iSkore added a commit to iSkore/node that referenced this issue Apr 3, 2018
@Trott
Copy link
Member

Trott commented Nov 4, 2018

@nodejs/http-parser

iSkore added a commit to iSkore/node that referenced this issue May 14, 2020
applied updates from previous pull-requests to disallow
socket.setEncoding before a http connection is parsed.
wrapped socket.setEncoding to throw an error.
previously resulted in a fatal error

Fixes: nodejs#18118
Ref: nodejs#18178
Ref: nodejs#19344
@iSkore
Copy link
Contributor Author

iSkore commented May 14, 2020

ping @nodejs/http-parser and teammates
I made a new PR #33405 with respect to upstream changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
3 participants