From 778c61b6fc083a951cfa6a939fe04ce8baa656d9 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 18 May 2015 16:52:47 -0700 Subject: [PATCH 1/2] Added failing tests for server bad argument handling --- src/node/test/surface_test.js | 221 +++++++++++++++++++++++----------- 1 file changed, 151 insertions(+), 70 deletions(-) diff --git a/src/node/test/surface_test.js b/src/node/test/surface_test.js index 9c72c29fab3..ccced741abd 100644 --- a/src/node/test/surface_test.js +++ b/src/node/test/surface_test.js @@ -47,6 +47,8 @@ var mathService = math_proto.lookup('math.Math'); var capitalize = require('underscore.string/capitalize'); +var _ = require('underscore'); + describe('File loader', function() { it('Should load a proto file by default', function() { assert.doesNotThrow(function() { @@ -178,9 +180,10 @@ describe('Generic client and server', function() { }); }); }); -describe('Trailing metadata', function() { +describe('Other conditions', function() { var client; var server; + var port; before(function() { var test_proto = ProtoBuf.loadProtoFile(__dirname + '/test_service.proto'); var test_service = test_proto.lookup('TestService'); @@ -189,6 +192,7 @@ describe('Trailing metadata', function() { TestService: { unary: function(call, cb) { var req = call.request; + debugger; if (req.error) { cb(new Error('Requested error'), null, {metadata: ['yes']}); } else { @@ -246,7 +250,7 @@ describe('Trailing metadata', function() { } } }); - var port = server.bind('localhost:0'); + port = server.bind('localhost:0'); var Client = surface_client.makeProtobufClientConstructor(test_service); client = new Client('localhost:' + port); server.listen(); @@ -254,86 +258,163 @@ describe('Trailing metadata', function() { after(function() { server.shutdown(); }); - it('should be present when a unary call succeeds', function(done) { - var call = client.unary({error: false}, function(err, data) { - assert.ifError(err); + describe('Server recieving bad input', function() { + var misbehavingClient; + var badArg = new Buffer([0xFF]); + before(function() { + var test_service_attrs = { + unary: { + path: '/TestService/Unary', + requestStream: false, + responseStream: false, + requestSerialize: _.identity, + responseDeserialize: _.identity + }, + clientStream: { + path: '/TestService/ClientStream', + requestStream: true, + responseStream: false, + requestSerialize: _.identity, + responseDeserialize: _.identity + }, + serverStream: { + path: '/TestService/ServerStream', + requestStream: false, + responseStream: true, + requestSerialize: _.identity, + responseDeserialize: _.identity + }, + bidiStream: { + path: '/TestService/BidiStream', + requestStream: true, + responseStream: true, + requestSerialize: _.identity, + responseDeserialize: _.identity + } + }; + var Client = surface_client.makeClientConstructor(test_service_attrs, + 'TestService'); + misbehavingClient = new Client('localhost:' + port); }); - call.on('status', function(status) { - assert.deepEqual(status.metadata.metadata, ['yes']); - done(); + it('should respond correctly to a unary call', function(done) { + var call = misbehavingClient.unary(badArg, function(err, data) { + assert(err); + assert.strictEqual(err.code, grpc.status.INVALID_ARGUMENT); + done(); + }); }); - }); - it('should be present when a unary call fails', function(done) { - var call = client.unary({error: true}, function(err, data) { - assert(err); + it('should respond correctly to a client stream', function(done) { + var call = misbehavingClient.clientStream(function(err, data) { + assert(err); + assert.strictEqual(err.code, grpc.status.INVALID_ARGUMENT); + done(); + }); + call.write(badArg); }); - call.on('status', function(status) { - assert.deepEqual(status.metadata.metadata, ['yes']); - done(); + it('should respond correctly to a server stream', function(done) { + var call = misbehavingClient.serverStream(badArg); + call.on('data', function(data) { + assert.fail(data, null, 'Unexpected data', '!='); + }); + call.on('error', function(err) { + assert.strictEqual(err.code, grpc.status.INVALID_ARGUMENT); + done(); + }); + }); + it('should respond correctly to a bidi stream', function(done) { + var call = misbehavingClient.bidiStream(); + call.on('data', function(data) { + assert.fail(data, null, 'Unexpected data', '!='); + }); + call.on('error', function(err) { + assert.strictEqual(err.code, grpc.status.INVALID_ARGUMENT); + done(); + }); + call.write(badArg); }); }); - it('should be present when a client stream call succeeds', function(done) { - var call = client.clientStream(function(err, data) { - assert.ifError(err); + describe('Trailing metadata', function() { + it('should be present when a unary call succeeds', function(done) { + var call = client.unary({error: false}, function(err, data) { + assert.ifError(err); + }); + call.on('status', function(status) { + assert.deepEqual(status.metadata.metadata, ['yes']); + done(); + }); }); - call.write({error: false}); - call.write({error: false}); - call.end(); - call.on('status', function(status) { - assert.deepEqual(status.metadata.metadata, ['yes']); - done(); + it('should be present when a unary call fails', function(done) { + var call = client.unary({error: true}, function(err, data) { + assert(err); + }); + call.on('status', function(status) { + assert.deepEqual(status.metadata.metadata, ['yes']); + done(); + }); }); - }); - it('should be present when a client stream call fails', function(done) { - var call = client.clientStream(function(err, data) { - assert(err); + it('should be present when a client stream call succeeds', function(done) { + var call = client.clientStream(function(err, data) { + assert.ifError(err); + }); + call.write({error: false}); + call.write({error: false}); + call.end(); + call.on('status', function(status) { + assert.deepEqual(status.metadata.metadata, ['yes']); + done(); + }); }); - call.write({error: false}); - call.write({error: true}); - call.end(); - call.on('status', function(status) { - assert.deepEqual(status.metadata.metadata, ['yes']); - done(); + it('should be present when a client stream call fails', function(done) { + var call = client.clientStream(function(err, data) { + assert(err); + }); + call.write({error: false}); + call.write({error: true}); + call.end(); + call.on('status', function(status) { + assert.deepEqual(status.metadata.metadata, ['yes']); + done(); + }); }); - }); - it('should be present when a server stream call succeeds', function(done) { - var call = client.serverStream({error: false}); - call.on('data', function(){}); - call.on('status', function(status) { - assert.strictEqual(status.code, grpc.status.OK); - assert.deepEqual(status.metadata.metadata, ['yes']); - done(); + it('should be present when a server stream call succeeds', function(done) { + var call = client.serverStream({error: false}); + call.on('data', function(){}); + call.on('status', function(status) { + assert.strictEqual(status.code, grpc.status.OK); + assert.deepEqual(status.metadata.metadata, ['yes']); + done(); + }); }); - }); - it('should be present when a server stream call fails', function(done) { - var call = client.serverStream({error: true}); - call.on('data', function(){}); - call.on('error', function(error) { - assert.deepEqual(error.metadata.metadata, ['yes']); - done(); + it('should be present when a server stream call fails', function(done) { + var call = client.serverStream({error: true}); + call.on('data', function(){}); + call.on('error', function(error) { + assert.deepEqual(error.metadata.metadata, ['yes']); + done(); + }); }); - }); - it('should be present when a bidi stream succeeds', function(done) { - var call = client.bidiStream(); - call.write({error: false}); - call.write({error: false}); - call.end(); - call.on('data', function(){}); - call.on('status', function(status) { - assert.strictEqual(status.code, grpc.status.OK); - assert.deepEqual(status.metadata.metadata, ['yes']); - done(); + it('should be present when a bidi stream succeeds', function(done) { + var call = client.bidiStream(); + call.write({error: false}); + call.write({error: false}); + call.end(); + call.on('data', function(){}); + call.on('status', function(status) { + assert.strictEqual(status.code, grpc.status.OK); + assert.deepEqual(status.metadata.metadata, ['yes']); + done(); + }); }); - }); - it('should be present when a bidi stream fails', function(done) { - var call = client.bidiStream(); - call.write({error: false}); - call.write({error: true}); - call.end(); - call.on('data', function(){}); - call.on('error', function(error) { - assert.deepEqual(error.metadata.metadata, ['yes']); - done(); + it('should be present when a bidi stream fails', function(done) { + var call = client.bidiStream(); + call.write({error: false}); + call.write({error: true}); + call.end(); + call.on('data', function(){}); + call.on('error', function(error) { + assert.deepEqual(error.metadata.metadata, ['yes']); + done(); + }); }); }); }); From 04589a7e0cbdb22c3fe10d2a1e2b7e698091f10d Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Tue, 19 May 2015 09:51:26 -0700 Subject: [PATCH 2/2] Fixed server to handle invalid arguments without breaking --- src/node/src/server.js | 29 ++++++++++++++++++++++++++--- src/node/test/surface_test.js | 11 +++++++---- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/src/node/src/server.js b/src/node/src/server.js index eef705c44c6..079495afd4c 100644 --- a/src/node/src/server.js +++ b/src/node/src/server.js @@ -291,7 +291,15 @@ function _read(size) { return; } var data = event.read; - if (self.push(self.deserialize(data)) && data !== null) { + var deserialized; + try { + deserialized = self.deserialize(data); + } catch (e) { + e.code = grpc.status.INVALID_ARGUMENT; + self.emit('error', e); + return; + } + if (self.push(deserialized) && data !== null) { var read_batch = {}; read_batch[grpc.opType.RECV_MESSAGE] = true; self.call.startBatch(read_batch, readCallback); @@ -354,7 +362,13 @@ function handleUnary(call, handler, metadata) { handleError(call, err); return; } - emitter.request = handler.deserialize(result.read); + try { + emitter.request = handler.deserialize(result.read); + } catch (e) { + e.code = grpc.status.INVALID_ARGUMENT; + handleError(call, e); + return; + } if (emitter.cancelled) { return; } @@ -388,7 +402,13 @@ function handleServerStreaming(call, handler, metadata) { stream.emit('error', err); return; } - stream.request = handler.deserialize(result.read); + try { + stream.request = handler.deserialize(result.read); + } catch (e) { + e.code = grpc.status.INVALID_ARGUMENT; + stream.emit('error', e); + return; + } handler.func(stream); }); } @@ -401,6 +421,9 @@ function handleServerStreaming(call, handler, metadata) { */ function handleClientStreaming(call, handler, metadata) { var stream = new ServerReadableStream(call, handler.deserialize); + stream.on('error', function(error) { + handleError(call, error); + }); waitForCancel(call, stream); var metadata_batch = {}; metadata_batch[grpc.opType.SEND_INITIAL_METADATA] = metadata; diff --git a/src/node/test/surface_test.js b/src/node/test/surface_test.js index ccced741abd..b390f8b2a55 100644 --- a/src/node/test/surface_test.js +++ b/src/node/test/surface_test.js @@ -192,7 +192,6 @@ describe('Other conditions', function() { TestService: { unary: function(call, cb) { var req = call.request; - debugger; if (req.error) { cb(new Error('Requested error'), null, {metadata: ['yes']}); } else { @@ -297,7 +296,7 @@ describe('Other conditions', function() { misbehavingClient = new Client('localhost:' + port); }); it('should respond correctly to a unary call', function(done) { - var call = misbehavingClient.unary(badArg, function(err, data) { + misbehavingClient.unary(badArg, function(err, data) { assert(err); assert.strictEqual(err.code, grpc.status.INVALID_ARGUMENT); done(); @@ -310,11 +309,13 @@ describe('Other conditions', function() { done(); }); call.write(badArg); + // TODO(mlumish): Remove call.end() + call.end(); }); it('should respond correctly to a server stream', function(done) { var call = misbehavingClient.serverStream(badArg); call.on('data', function(data) { - assert.fail(data, null, 'Unexpected data', '!='); + assert.fail(data, null, 'Unexpected data', '==='); }); call.on('error', function(err) { assert.strictEqual(err.code, grpc.status.INVALID_ARGUMENT); @@ -324,13 +325,15 @@ describe('Other conditions', function() { it('should respond correctly to a bidi stream', function(done) { var call = misbehavingClient.bidiStream(); call.on('data', function(data) { - assert.fail(data, null, 'Unexpected data', '!='); + assert.fail(data, null, 'Unexpected data', '==='); }); call.on('error', function(err) { assert.strictEqual(err.code, grpc.status.INVALID_ARGUMENT); done(); }); call.write(badArg); + // TODO(mlumish): Remove call.end() + call.end(); }); }); describe('Trailing metadata', function() {