Skip to content

Commit

Permalink
http2: do not allow socket manipulation
Browse files Browse the repository at this point in the history
Because of the specific serialization and processing
requirements of HTTP/2, sockets should not be
directly manipulated. This forbids any interactions
with destroy, emit, end, once, on, pause, read,
resume and write methods of the socket. It also
redirects setTimeout to session instead of socket.

Fixes: nodejs#16252
Refs: nodejs#16211
  • Loading branch information
apapirovski committed Oct 24, 2017
1 parent 4108072 commit eaa71a1
Show file tree
Hide file tree
Showing 9 changed files with 182 additions and 51 deletions.
13 changes: 7 additions & 6 deletions lib/internal/http2/compat.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const Readable = Stream.Readable;
const binding = process.binding('http2');
const constants = binding.constants;
const errors = require('internal/errors');
const { kSocket } = require('internal/http2/util');

const kFinish = Symbol('finish');
const kBeginSend = Symbol('begin-send');
Expand Down Expand Up @@ -176,15 +177,15 @@ const proxySocketHandler = {
throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION');
default:
const ref = stream.session !== undefined ?
stream.session.socket : stream;
stream.session[kSocket] : stream;
const value = ref[prop];
return typeof value === 'function' ? value.bind(ref) : value;
}
},
getPrototypeOf(stream) {
if (stream.session !== undefined)
return stream.session.socket.constructor.prototype;
return stream.prototype;
return Reflect.getPrototypeOf(stream.session[kSocket]);
return Reflect.getPrototypeOf(stream);
},
set(stream, prop, value) {
switch (prop) {
Expand All @@ -201,9 +202,9 @@ const proxySocketHandler = {
case 'setTimeout':
const session = stream.session;
if (session !== undefined)
session[prop] = value;
session.setTimeout = value;
else
stream[prop] = value;
stream.setTimeout = value;
return true;
case 'write':
case 'read':
Expand All @@ -212,7 +213,7 @@ const proxySocketHandler = {
throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION');
default:
const ref = stream.session !== undefined ?
stream.session.socket : stream;
stream.session[kSocket] : stream;
ref[prop] = value;
return true;
}
Expand Down
86 changes: 54 additions & 32 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ const {
getSettings,
getStreamState,
isPayloadMeaningless,
kSocket,
mapToHeaders,
NghttpError,
sessionName,
Expand Down Expand Up @@ -70,10 +71,10 @@ const kOptions = Symbol('options');
const kOwner = Symbol('owner');
const kProceed = Symbol('proceed');
const kProtocol = Symbol('protocol');
const kProxySocket = Symbol('proxy-socket');
const kRemoteSettings = Symbol('remote-settings');
const kServer = Symbol('server');
const kSession = Symbol('session');
const kSocket = Symbol('socket');
const kState = Symbol('state');
const kType = Symbol('type');

Expand Down Expand Up @@ -672,6 +673,52 @@ function finishSessionDestroy(self, socket) {
debug(`[${sessionName(self[kType])}] nghttp2session destroyed`);
}

const proxySocketHandler = {
get(session, prop) {
switch (prop) {
case 'setTimeout':
return session.setTimeout.bind(session);
case 'destroy':
case 'emit':
case 'end':
case 'once':
case 'on':
case 'pause':
case 'read':
case 'resume':
case 'write':
throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION');
default:
const socket = session[kSocket];
const value = socket[prop];
return typeof value === 'function' ? value.bind(socket) : value;
}
},
getPrototypeOf(session) {
return Reflect.getPrototypeOf(session[kSocket]);
},
set(session, prop, value) {
switch (prop) {
case 'setTimeout':
session.setTimeout = value;
return true;
case 'destroy':
case 'emit':
case 'end':
case 'once':
case 'on':
case 'pause':
case 'read':
case 'resume':
case 'write':
throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION');
default:
session[kSocket][prop] = value;
return true;
}
}
};

// Upon creation, the Http2Session takes ownership of the socket. The session
// may not be ready to use immediately if the socket is not yet fully connected.
class Http2Session extends EventEmitter {
Expand Down Expand Up @@ -707,6 +754,7 @@ class Http2Session extends EventEmitter {
};

this[kType] = type;
this[kProxySocket] = null;
this[kSocket] = socket;

// Do not use nagle's algorithm
Expand Down Expand Up @@ -756,7 +804,10 @@ class Http2Session extends EventEmitter {

// The socket owned by this session
get socket() {
return this[kSocket];
const proxySocket = this[kProxySocket];
if (proxySocket === null)
return this[kProxySocket] = new Proxy(this, proxySocketHandler);
return proxySocket;
}

// The session type
Expand Down Expand Up @@ -957,6 +1008,7 @@ class Http2Session extends EventEmitter {
// Disassociate from the socket and server
const socket = this[kSocket];
// socket.pause();
delete this[kProxySocket];
delete this[kSocket];
delete this[kServer];

Expand Down Expand Up @@ -2155,30 +2207,6 @@ function socketDestroy(error) {
this.destroy(error);
}

function socketOnResume() {
if (this._paused)
return this.pause();
if (this._handle && !this._handle.reading) {
this._handle.reading = true;
this._handle.readStart();
}
}

function socketOnPause() {
if (this._handle && this._handle.reading) {
this._handle.reading = false;
this._handle.readStop();
}
}

function socketOnDrain() {
const needPause = 0 > this._writableState.highWaterMark;
if (this._paused && !needPause) {
this._paused = false;
this.resume();
}
}

// When an Http2Session emits an error, first try to forward it to the
// server as a sessionError; failing that, forward it to the socket as
// a sessionError; failing that, destroy, remove the error listener, and
Expand Down Expand Up @@ -2267,9 +2295,6 @@ function connectionListener(socket) {
}

socket.on('error', socketOnError);
socket.on('resume', socketOnResume);
socket.on('pause', socketOnPause);
socket.on('drain', socketOnDrain);
socket.on('close', socketOnClose);

// Set up the Session
Expand Down Expand Up @@ -2426,9 +2451,6 @@ function connect(authority, options, listener) {
}

socket.on('error', socketOnError);
socket.on('resume', socketOnResume);
socket.on('pause', socketOnPause);
socket.on('drain', socketOnDrain);
socket.on('close', socketOnClose);

const session = new ClientHttp2Session(options, socket);
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/http2/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
const binding = process.binding('http2');
const errors = require('internal/errors');

const kSocket = Symbol('socket');

const {
NGHTTP2_SESSION_CLIENT,
NGHTTP2_SESSION_SERVER,
Expand Down Expand Up @@ -551,6 +553,7 @@ module.exports = {
getSettings,
getStreamState,
isPayloadMeaningless,
kSocket,
mapToHeaders,
NghttpError,
sessionName,
Expand Down
11 changes: 7 additions & 4 deletions test/parallel/test-http2-client-destroy.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
// Flags: --expose-internals

'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const assert = require('assert');
const h2 = require('http2');
const { kSocket } = require('internal/http2/util');

{
const server = h2.createServer();
Expand All @@ -13,7 +16,7 @@ const h2 = require('http2');
common.mustCall(() => {
const destroyCallbacks = [
(client) => client.destroy(),
(client) => client.socket.destroy()
(client) => client[kSocket].destroy()
];

let remaining = destroyCallbacks.length;
Expand All @@ -23,9 +26,9 @@ const h2 = require('http2');
client.on(
'connect',
common.mustCall(() => {
const socket = client.socket;
const socket = client[kSocket];

assert(client.socket, 'client session has associated socket');
assert(socket, 'client session has associated socket');
assert(
!client.destroyed,
'client has not been destroyed before destroy is called'
Expand All @@ -41,7 +44,7 @@ const h2 = require('http2');
destroyCallback(client);

assert(
!client.socket,
!client[kSocket],
'client.socket undefined after destroy is called'
);

Expand Down
6 changes: 5 additions & 1 deletion test/parallel/test-http2-client-socket-destroy.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
// Flags: --expose-internals

'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const h2 = require('http2');
const { kSocket } = require('internal/http2/util');

const body =
'<html><head></head><body><h1>this is some data</h2></body></html>';

Expand Down Expand Up @@ -32,7 +36,7 @@ server.on('listening', common.mustCall(function() {

req.on('response', common.mustCall(() => {
// send a premature socket close
client.socket.destroy();
client[kSocket].destroy();
}));
req.on('data', common.mustNotCall());

Expand Down
7 changes: 5 additions & 2 deletions test/parallel/test-http2-create-client-secure-session.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// Flags: --expose-internals

'use strict';

const common = require('../common');
Expand All @@ -8,14 +10,15 @@ if (!common.hasCrypto)
const assert = require('assert');
const fixtures = require('../common/fixtures');
const h2 = require('http2');
const { kSocket } = require('internal/http2/util');
const tls = require('tls');

function loadKey(keyname) {
return fixtures.readKey(keyname, 'binary');
}

function onStream(stream, headers) {
const socket = stream.session.socket;
const socket = stream.session[kSocket];
assert(headers[':authority'].startsWith(socket.servername));
stream.respond({
'content-type': 'text/html',
Expand Down Expand Up @@ -55,7 +58,7 @@ function verifySecureSession(key, cert, ca, opts) {
assert.strictEqual(jsonData.servername, opts.servername || 'localhost');
assert.strictEqual(jsonData.alpnProtocol, 'h2');
server.close();
client.socket.destroy();
client[kSocket].destroy();
}));
req.end();
});
Expand Down
7 changes: 5 additions & 2 deletions test/parallel/test-http2-server-socket-destroy.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
// Flags: --expose-internals

'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const h2 = require('http2');
const assert = require('assert');
const h2 = require('http2');
const { kSocket } = require('internal/http2/util');

const {
HTTP2_HEADER_METHOD,
Expand All @@ -24,7 +27,7 @@ function onStream(stream) {
});
stream.write('test');

const socket = stream.session.socket;
const socket = stream.session[kSocket];

// When the socket is destroyed, the close events must be triggered
// on the socket, server and session.
Expand Down
11 changes: 7 additions & 4 deletions test/parallel/test-http2-server-socketerror.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
// Flags: --expose-internals

'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const assert = require('assert');
const http2 = require('http2');
const { kSocket } = require('internal/http2/util');

const server = http2.createServer();
server.on('stream', common.mustCall((stream) => {
Expand All @@ -19,12 +22,12 @@ server.on('session', common.mustCall((session) => {
type: Error,
message: 'test'
})(error);
assert.strictEqual(socket, session.socket);
assert.strictEqual(socket, session[kSocket]);
});
const isNotCalled = common.mustNotCall();
session.on('socketError', handler);
server.on('socketError', isNotCalled);
session.socket.emit('error', new Error('test'));
session[kSocket].emit('error', new Error('test'));
session.removeListener('socketError', handler);
server.removeListener('socketError', isNotCalled);

Expand All @@ -35,10 +38,10 @@ server.on('session', common.mustCall((session) => {
type: Error,
message: 'test'
})(error);
assert.strictEqual(socket, session.socket);
assert.strictEqual(socket, session[kSocket]);
assert.strictEqual(session, session);
}));
session.socket.emit('error', new Error('test'));
session[kSocket].emit('error', new Error('test'));
}));

server.listen(0, common.mustCall(() => {
Expand Down
Loading

0 comments on commit eaa71a1

Please sign in to comment.