FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: do not allow socket manipulation · nodejs/node@e6e99eb · GitHub

/ node Public

Commit e6e99eb

Browse files
apapirovski
authored and
gibfahn
committed
http2: do not allow socket manipulation
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, pause, read, resume and write methods of the socket. It also redirects setTimeout to session instead of socket. PR-URL: #16330 Fixes: #16252 Refs: #16211 Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent ac02a0b commit e6e99eb

14 files changed

Lines changed: 200 additions & 73 deletions

‎doc/api/errors.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -725,8 +725,8 @@ reached.
725725
<a id="ERR_HTTP2_NO_SOCKET_MANIPULATION"></a>
726726
### ERR_HTTP2_NO_SOCKET_MANIPULATION
727727

728-
Used when attempting to read, write, pause, and/or resume a socket attached to
729-
an `Http2Session`.
728+
Used when attempting to directly manipulate (e.g read, write, pause, resume,
729+
etc.) a socket attached to an `Http2Session`.
730730

731731
<a id="ERR_HTTP2_OUT_OF_STREAMS"></a>
732732
### ERR_HTTP2_OUT_OF_STREAMS

‎doc/api/http2.md‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -463,12 +463,16 @@ added: v8.4.0
463463

464464
* Value: {net.Socket|tls.TLSSocket}
465465

466-
A reference to the [`net.Socket`][] or [`tls.TLSSocket`][] to which this
467-
`Http2Session` instance is bound.
466+
Returns a Proxy object that acts as a `net.Socket` (or `tls.TLSSocket`) but
467+
limits available methods to ones safe to use with HTTP/2.
468468

469-
*Note*: It is not recommended for user code to interact directly with a
470-
`Socket` bound to an `Http2Session`. See [Http2Session and Sockets][] for
471-
details.
469+
`destroy`, `emit`, `end`, `pause`, `read`, `resume`, and `write` will throw
470+
an error with code `ERR_HTTP2_NO_SOCKET_MANIPULATION`. See
471+
[Http2Session and Sockets][] for more information.
472+
473+
`setTimeout` method will be called on this `Http2Session`.
474+
475+
All other interactions will be routed directly to the socket.
472476

473477
#### http2session.state
474478
<!-- YAML
@@ -2138,10 +2142,10 @@ Returns `request`.
21382142
added: v8.4.0
21392143
-->
21402144

2141-
* {net.Socket}
2145+
* {net.Socket|tls.TLSSocket}
21422146

2143-
Returns a Proxy object that acts as a `net.Socket` but applies getters,
2144-
setters and methods based on HTTP/2 logic.
2147+
Returns a Proxy object that acts as a `net.Socket` (or `tls.TLSSocket`) but
2148+
applies getters, setters and methods based on HTTP/2 logic.
21452149

21462150
`destroyed`, `readable`, and `writable` properties will be retrieved from and
21472151
set on `request.stream`.
@@ -2293,7 +2297,7 @@ will result in a [`TypeError`][] being thrown.
22932297
added: v8.4.0
22942298
-->
22952299

2296-
* {net.Socket}
2300+
* {net.Socket|tls.TLSSocket}
22972301

22982302
See [`response.socket`][].
22992303

@@ -2510,10 +2514,10 @@ Returns `response`.
25102514
added: v8.4.0
25112515
-->
25122516

2513-
* {net.Socket}
2517+
* {net.Socket|tls.TLSSocket}
25142518

2515-
Returns a Proxy object that acts as a `net.Socket` but applies getters,
2516-
setters and methods based on HTTP/2 logic.
2519+
Returns a Proxy object that acts as a `net.Socket` (or `tls.TLSSocket`) but
2520+
applies getters, setters and methods based on HTTP/2 logic.
25172521

25182522
`destroyed`, `readable`, and `writable` properties will be retrieved from and
25192523
set on `response.stream`.

‎lib/internal/errors.js‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -199,8 +199,7 @@ E('ERR_HTTP2_INVALID_STREAM', 'The stream has been destroyed');
199199
E('ERR_HTTP2_MAX_PENDING_SETTINGS_ACK',
200200
(max) => `Maximum number of pending settings acknowledgements (${max})`);
201201
E('ERR_HTTP2_NO_SOCKET_MANIPULATION',
202-
'HTTP/2 sockets should not be directly read from, written to, ' +
203-
'paused and/or resumed.');
202+
'HTTP/2 sockets should not be directly manipulated (e.g. read and written)');
204203
E('ERR_HTTP2_OUT_OF_STREAMS',
205204
'No stream ID is available because maximum stream ID has been reached');
206205
E('ERR_HTTP2_PAYLOAD_FORBIDDEN',

‎lib/internal/http2/compat.js‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ const Readable = Stream.Readable;
55
const binding = process.binding('http2');
66
const constants = binding.constants;
77
const errors = require('internal/errors');
8+
const { kSocket } = require('internal/http2/util');
89

910
const kFinish = Symbol('finish');
1011
const kBeginSend = Symbol('begin-send');
@@ -176,15 +177,15 @@ const proxySocketHandler = {
176177
throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION');
177178
default:
178179
const ref = stream.session !== undefined ?
179-
stream.session.socket : stream;
180+
stream.session[kSocket] : stream;
180181
const value = ref[prop];
181182
return typeof value === 'function' ? value.bind(ref) : value;
182183
}
183184
},
184185
getPrototypeOf(stream) {
185186
if (stream.session !== undefined)
186-
return stream.session.socket.constructor.prototype;
187-
return stream.prototype;
187+
return Reflect.getPrototypeOf(stream.session[kSocket]);
188+
return Reflect.getPrototypeOf(stream);
188189
},
189190
set(stream, prop, value) {
190191
switch (prop) {
@@ -201,9 +202,9 @@ const proxySocketHandler = {
201202
case 'setTimeout':
202203
const session = stream.session;
203204
if (session !== undefined)
204-
session[prop] = value;
205+
session.setTimeout = value;
205206
else
206-
stream[prop] = value;
207+
stream.setTimeout = value;
207208
return true;
208209
case 'write':
209210
case 'read':
@@ -212,7 +213,7 @@ const proxySocketHandler = {
212213
throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION');
213214
default:
214215
const ref = stream.session !== undefined ?
215-
stream.session.socket : stream;
216+
stream.session[kSocket] : stream;
216217
ref[prop] = value;
217218
return true;
218219
}

‎lib/internal/http2/core.js‎

Lines changed: 50 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ const {
3838
getSettings,
3939
getStreamState,
4040
isPayloadMeaningless,
41+
kSocket,
4142
mapToHeaders,
4243
NghttpError,
4344
sessionName,
@@ -70,10 +71,10 @@ const kOptions = Symbol('options');
7071
const kOwner = Symbol('owner');
7172
const kProceed = Symbol('proceed');
7273
const kProtocol = Symbol('protocol');
74+
const kProxySocket = Symbol('proxy-socket');
7375
const kRemoteSettings = Symbol('remote-settings');
7476
const kServer = Symbol('server');
7577
const kSession = Symbol('session');
76-
const kSocket = Symbol('socket');
7778
const kState = Symbol('state');
7879
const kType = Symbol('type');
7980

@@ -672,6 +673,48 @@ function finishSessionDestroy(self, socket) {
672673
debug(`[${sessionName(self[kType])}] nghttp2session destroyed`);
673674
}
674675

676+
const proxySocketHandler = {
677+
get(session, prop) {
678+
switch (prop) {
679+
case 'setTimeout':
680+
return session.setTimeout.bind(session);
681+
case 'destroy':
682+
case 'emit':
683+
case 'end':
684+
case 'pause':
685+
case 'read':
686+
case 'resume':
687+
case 'write':
688+
throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION');
689+
default:
690+
const socket = session[kSocket];
691+
const value = socket[prop];
692+
return typeof value === 'function' ? value.bind(socket) : value;
693+
}
694+
},
695+
getPrototypeOf(session) {
696+
return Reflect.getPrototypeOf(session[kSocket]);
697+
},
698+
set(session, prop, value) {
699+
switch (prop) {
700+
case 'setTimeout':
701+
session.setTimeout = value;
702+
return true;
703+
case 'destroy':
704+
case 'emit':
705+
case 'end':
706+
case 'pause':
707+
case 'read':
708+
case 'resume':
709+
case 'write':
710+
throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION');
711+
default:
712+
session[kSocket][prop] = value;
713+
return true;
714+
}
715+
}
716+
};
717+
675718
// Upon creation, the Http2Session takes ownership of the socket. The session
676719
// may not be ready to use immediately if the socket is not yet fully connected.
677720
class Http2Session extends EventEmitter {
@@ -707,6 +750,7 @@ class Http2Session extends EventEmitter {
707750
};
708751

709752
this[kType] = type;
753+
this[kProxySocket] = null;
710754
this[kSocket] = socket;
711755

712756
// Do not use nagle's algorithm
@@ -756,7 +800,10 @@ class Http2Session extends EventEmitter {
756800

757801
// The socket owned by this session
758802
get socket() {
759-
return this[kSocket];
803+
const proxySocket = this[kProxySocket];
804+
if (proxySocket === null)
805+
return this[kProxySocket] = new Proxy(this, proxySocketHandler);
806+
return proxySocket;
760807
}
761808

762809
// The session type
@@ -957,6 +1004,7 @@ class Http2Session extends EventEmitter {
9571004
// Disassociate from the socket and server
9581005
const socket = this[kSocket];
9591006
// socket.pause();
1007+
delete this[kProxySocket];
9601008
delete this[kSocket];
9611009
delete this[kServer];
9621010

@@ -2155,30 +2203,6 @@ function socketDestroy(error) {
21552203
this.destroy(error);
21562204
}
21572205

2158-
function socketOnResume() {
2159-
if (this._paused)
2160-
return this.pause();
2161-
if (this._handle && !this._handle.reading) {
2162-
this._handle.reading = true;
2163-
this._handle.readStart();
2164-
}
2165-
}
2166-
2167-
function socketOnPause() {
2168-
if (this._handle && this._handle.reading) {
2169-
this._handle.reading = false;
2170-
this._handle.readStop();
2171-
}
2172-
}
2173-
2174-
function socketOnDrain() {
2175-
const needPause = 0 > this._writableState.highWaterMark;
2176-
if (this._paused && !needPause) {
2177-
this._paused = false;
2178-
this.resume();
2179-
}
2180-
}
2181-
21822206
// When an Http2Session emits an error, first try to forward it to the
21832207
// server as a sessionError; failing that, forward it to the socket as
21842208
// a sessionError; failing that, destroy, remove the error listener, and
@@ -2267,9 +2291,6 @@ function connectionListener(socket) {
22672291
}
22682292

22692293
socket.on('error', socketOnError);
2270-
socket.on('resume', socketOnResume);
2271-
socket.on('pause', socketOnPause);
2272-
socket.on('drain', socketOnDrain);
22732294
socket.on('close', socketOnClose);
22742295

22752296
// Set up the Session
@@ -2426,9 +2447,6 @@ function connect(authority, options, listener) {
24262447
}
24272448

24282449
socket.on('error', socketOnError);
2429-
socket.on('resume', socketOnResume);
2430-
socket.on('pause', socketOnPause);
2431-
socket.on('drain', socketOnDrain);
24322450
socket.on('close', socketOnClose);
24332451

24342452
const session = new ClientHttp2Session(options, socket);

‎lib/internal/http2/util.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
const binding = process.binding('http2');
44
const errors = require('internal/errors');
55

6+
const kSocket = Symbol('socket');
7+
68
const {
79
NGHTTP2_SESSION_CLIENT,
810
NGHTTP2_SESSION_SERVER,
@@ -551,6 +553,7 @@ module.exports = {
551553
getSettings,
552554
getStreamState,
553555
isPayloadMeaningless,
556+
kSocket,
554557
mapToHeaders,
555558
NghttpError,
556559
sessionName,

‎test/parallel/test-http2-client-destroy.js‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
1+
// Flags: --expose-internals
2+
13
'use strict';
24

35
const common = require('../common');
46
if (!common.hasCrypto)
57
common.skip('missing crypto');
68
const assert = require('assert');
79
const h2 = require('http2');
10+
const { kSocket } = require('internal/http2/util');
811

912
{
1013
const server = h2.createServer();
@@ -13,7 +16,7 @@ const h2 = require('http2');
1316
common.mustCall(() => {
1417
const destroyCallbacks = [
1518
(client) => client.destroy(),
16-
(client) => client.socket.destroy()
19+
(client) => client[kSocket].destroy()
1720
];
1821

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

28-
assert(client.socket, 'client session has associated socket');
31+
assert(socket, 'client session has associated socket');
2932
assert(
3033
!client.destroyed,
3134
'client has not been destroyed before destroy is called'
@@ -41,7 +44,7 @@ const h2 = require('http2');
4144
destroyCallback(client);
4245

4346
assert(
44-
!client.socket,
47+
!client[kSocket],
4548
'client.socket undefined after destroy is called'
4649
);
4750

‎test/parallel/test-http2-client-socket-destroy.js‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,13 @@
1+
// Flags: --expose-internals
2+
13
'use strict';
24

35
const common = require('../common');
46
if (!common.hasCrypto)
57
common.skip('missing crypto');
68
const h2 = require('http2');
9+
const { kSocket } = require('internal/http2/util');
10+
711
const body =
812
'<html><head></head><body><h1>this is some data</h2></body></html>';
913

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

3337
req.on('response', common.mustCall(() => {
3438
// send a premature socket close
35-
client.socket.destroy();
39+
client[kSocket].destroy();
3640
}));
3741
req.on('data', common.mustNotCall());
3842

‎test/parallel/test-http2-compat-socket-set.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ const h2 = require('http2');
1313
const errMsg = {
1414
code: 'ERR_HTTP2_NO_SOCKET_MANIPULATION',
1515
type: Error,
16-
message: 'HTTP/2 sockets should not be directly read from, written to, ' +
17-
'paused and/or resumed.'
16+
message: 'HTTP/2 sockets should not be directly manipulated ' +
17+
'(e.g. read and written)'
1818
};
1919

2020
const server = h2.createServer();

‎test/parallel/test-http2-compat-socket.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@ const net = require('net');
1515
const errMsg = {
1616
code: 'ERR_HTTP2_NO_SOCKET_MANIPULATION',
1717
type: Error,
18-
message: 'HTTP/2 sockets should not be directly read from, written to, ' +
19-
'paused and/or resumed.'
18+
message: 'HTTP/2 sockets should not be directly manipulated ' +
19+
'(e.g. read and written)'
2020
};
2121

2222
const server = h2.createServer();

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL