From 41cab4591339ea48ca95ac6f8447011f894564e3 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Mon, 18 Jul 2016 09:47:33 +1000 Subject: [PATCH] Fix REF CURSOR mem leak in error handling scenario. Also, bail out earlier when querying unsupported column types. --- src/njs/src/njsConnection.cpp | 178 ++++++++++++++++++++-------------- src/njs/src/njsOracle.cpp | 16 ++- test/nestedCursor.js | 60 +++++------- test/resultSet2.js | 41 ++------ test/stream1.js | 31 ++---- 5 files changed, 160 insertions(+), 166 deletions(-) diff --git a/src/njs/src/njsConnection.cpp b/src/njs/src/njsConnection.cpp index c3925cf49..bf07f6225 100644 --- a/src/njs/src/njsConnection.cpp +++ b/src/njs/src/njsConnection.cpp @@ -563,6 +563,13 @@ void Connection::ProcessOptions (Nan::NAN_METHOD_ARGS_TYPE args, unsigned int in options = args[index]->ToObject(); NJS_GET_UINT_FROM_JSON ( executeBaton->maxRows, executeBaton->error, options, "maxRows", 2, exitProcessOptions ); + + if ( executeBaton->maxRows <= 0 ) + { + executeBaton->error = NJSMessages::getErrorMsg ( errInvalidmaxRows ); + goto exitProcessOptions; + } + NJS_GET_UINT_FROM_JSON ( executeBaton->prefetchRows, executeBaton->error, options, "prefetchRows", 2, exitProcessOptions ); NJS_GET_UINT_FROM_JSON ( executeBaton->outFormat, executeBaton->error, @@ -1628,6 +1635,8 @@ void Connection::Async_Execute (uv_work_t *req) Connection::CopyMetaData ( executeBaton->mInfo, executeBaton, mData, executeBaton->numCols ); + if ( !executeBaton->error.empty() ) + goto exitAsyncExecute; if ( executeBaton->getRS ) { @@ -1763,6 +1772,9 @@ void Connection::Async_Execute (uv_work_t *req) } Connection::CopyMetaData ( extBind->mInfo, executeBaton, mData, extBind->numCols ); + if ( !executeBaton->error.empty() ) + goto exitAsyncExecute; + executeBaton->extBinds [ b ] = extBind; } else @@ -1807,6 +1819,21 @@ void Connection::Async_Execute (uv_work_t *req) { executeBaton->dpistmt->release (); } + + // In case of error, release the statement handles allocated (REF CURSOR) + if ( !(executeBaton->error).empty() ) + { + for( unsigned int index = 0 ;index < executeBaton->binds.size(); + index++ ) + { + if( executeBaton->binds[index]->value && + ( executeBaton->binds[index]->type == DpiRSet ) ) + { + ((Stmt*)executeBaton->binds[index]->value)->release (); + executeBaton->binds[index]->value = NULL; + } + } + } ; } @@ -1961,7 +1988,9 @@ void Connection::CopyMetaData ( MetaInfo *mInfo, const MetaData* mData, const unsigned int numCols ) { - for ( unsigned int col = 0; col < numCols; col++ ) + bool error = false; + + for ( unsigned int col = 0; !error && ( col < numCols ); col++ ) { mInfo[col].name = string( (const char*)mData[col].colName, mData[col].colNameLen ); @@ -2045,6 +2074,16 @@ void Connection::CopyMetaData ( MetaInfo *mInfo, mInfo[col].dbType = NJS_DATATYPE_UNKNOWN; break; } + + if ( mInfo[col].njsFetchType == NJS_DATATYPE_UNKNOWN ) + { + error = true; + } + } + + if ( error ) + { + executeBaton->error = NJSMessages::getErrorMsg ( errUnsupportedDatType ); } } @@ -2270,19 +2309,17 @@ unsigned short Connection::GetTargetType ( eBaton *executeBaton, */ void Connection::DoDefines ( eBaton* executeBaton ) { - unsigned int numCols = executeBaton->numCols; - Define *defines = executeBaton->defines = new Define[numCols]; - int csratio = executeBaton->dpiconn->getByteExpansionRatio (); - - // Check for maxRows must be greater than zero in case of non-resultSet - if ( executeBaton->maxRows == 0 ) - { - executeBaton->error = NJSMessages::getErrorMsg ( errInvalidmaxRows ); - return; - } + unsigned int numCols = executeBaton->numCols; + Define *defines = executeBaton->defines = new Define[numCols]; + int csratio = executeBaton->dpiconn->getByteExpansionRatio (); + bool error = false; - for (unsigned int col = 0; col < numCols; col++) + for (unsigned int col = 0; !error && ( col < numCols ); col++) { + /* + * Only supported DB column types handled here and others would have + * reported error while processing meta-data + */ switch( executeBaton->mInfo[col].dbType ) { case dpi::DpiNumber : @@ -2297,7 +2334,7 @@ void Connection::DoDefines ( eBaton* executeBaton ) executeBaton->maxRows ) ) { executeBaton->error = NJSMessages::getErrorMsg( errResultsTooLarge ); - return; + error = true; } else { @@ -2308,7 +2345,7 @@ void Connection::DoDefines ( eBaton* executeBaton ) { executeBaton->error = NJSMessages::getErrorMsg( errInsufficientMemory ); - return; + error = true; } } @@ -2320,7 +2357,7 @@ void Connection::DoDefines ( eBaton* executeBaton ) /* * the buffer size is increased to account for possible character * size expansion when data is converted from the DB character set - * to AL32UTF8 + * to client character set */ if ( executeBaton->mInfo[col].byteSize != 0 ) @@ -2333,7 +2370,7 @@ void Connection::DoDefines ( eBaton* executeBaton ) { executeBaton->error = NJSMessages::getErrorMsg( errResultsTooLarge ); - return; + error = true; } else { @@ -2343,7 +2380,7 @@ void Connection::DoDefines ( eBaton* executeBaton ) { executeBaton->error = NJSMessages::getErrorMsg( errInsufficientMemory ); - return; + error = true; } } } @@ -2351,24 +2388,13 @@ void Connection::DoDefines ( eBaton* executeBaton ) * not required. */ break; + case dpi::DpiTimestampTZ: + // TIMESTAMPTZ WITH TIMEZONE (TZ) supported only as STRING value case dpi::DpiDate : case dpi::DpiTimestamp: - case dpi::DpiTimestampTZ: case dpi::DpiTimestampLTZ: defines[col].fetchType = executeBaton->mInfo[col].dpiFetchType; - if ( ( executeBaton->mInfo[col].dbType == dpi::DpiTimestampTZ ) && - ( defines[col].fetchType != dpi::DpiVarChar )) - { - /* - * TIMESTAMP WITH TIMEZONE (TZ) column type supported only as - * STRING value. - */ - executeBaton->error = NJSMessages::getErrorMsg ( - errUnsupportedDatType ) ; - return; - } - if ( defines[col].fetchType != dpi::DpiVarChar ) { defines[col].dttmarr = executeBaton->dpienv->getDateTimeArray ( @@ -2385,8 +2411,9 @@ void Connection::DoDefines ( eBaton* executeBaton ) if ( NJS_SIZE_T_OVERFLOW ( defines[col].maxSize, executeBaton->maxRows ) ) { - executeBaton->error = NJSMessages::getErrorMsg( errResultsTooLarge ); - return; + executeBaton->error = + NJSMessages::getErrorMsg( errResultsTooLarge ); + error = true; } else { @@ -2397,7 +2424,7 @@ void Connection::DoDefines ( eBaton* executeBaton ) { executeBaton->error = NJSMessages::getErrorMsg( errInsufficientMemory); - return; + error = true; } } @@ -2412,15 +2439,18 @@ void Connection::DoDefines ( eBaton* executeBaton ) { executeBaton->error = NJSMessages::getErrorMsg ( errResultsTooLarge ) ; - return; + error = true; } - defines[col].buf = (char *)malloc(defines[col].maxSize * - (size_t) executeBaton->maxRows) ; - if ( !defines[col].buf ) + else { - executeBaton->error = NJSMessages::getErrorMsg ( - errInsufficientMemory ); - return; + defines[col].buf = (char *)malloc(defines[col].maxSize * + (size_t) executeBaton->maxRows) ; + if ( !defines[col].buf ) + { + executeBaton->error = NJSMessages::getErrorMsg ( + errInsufficientMemory ); + error = true; + } } break; @@ -2434,7 +2464,7 @@ void Connection::DoDefines ( eBaton* executeBaton ) executeBaton->maxRows ) ) { executeBaton->error = NJSMessages::getErrorMsg( errResultsTooLarge ); - return; + error = true; } else { @@ -2443,34 +2473,33 @@ void Connection::DoDefines ( eBaton* executeBaton ) if( !defines[col].buf ) { - executeBaton->error = NJSMessages::getErrorMsg( errInsufficientMemory ); - return; + executeBaton->error = + NJSMessages::getErrorMsg( errInsufficientMemory ); + error = true; } } - for (unsigned int j = 0; j < executeBaton->maxRows; j++) + if ( !error ) { - ((Descriptor **)(defines[col].buf))[j] = - executeBaton->dpienv->allocDescriptor(LobDescriptorType); + for (unsigned int j = 0; j < executeBaton->maxRows; j++) + { + ((Descriptor **)(defines[col].buf))[j] = + executeBaton->dpienv->allocDescriptor(LobDescriptorType); + } } break; case dpi::DpiRowid: defines[col].fetchType = executeBaton->mInfo[col].dpiFetchType; - if ( defines[col].fetchType != dpi::DpiVarChar ) - { - executeBaton->error = NJSMessages::getErrorMsg ( - errUnsupportedDatType); - return; - } + // ROWID supported only as STRING value defines[col].maxSize = NJS_MAX_FETCH_AS_STRING_SIZE; if ( NJS_SIZE_T_OVERFLOW ( defines[col].maxSize, executeBaton->maxRows ) ) { executeBaton->error = NJSMessages::getErrorMsg( errResultsTooLarge ); - return; + error = true; } else { @@ -2481,36 +2510,41 @@ void Connection::DoDefines ( eBaton* executeBaton ) { executeBaton->error = NJSMessages::getErrorMsg( errInsufficientMemory); - return; + error = true; } } break; default : - executeBaton->error = NJSMessages::getErrorMsg(errUnsupportedDatType); - return; + // For unsupported column types, an error is reported earlier itself + executeBaton->error = NJSMessages::getErrorMsg( errInternalError, + "default:", "DoDefines" ); + error = true; break; } - defines[col].ind = (short*)malloc (sizeof(short)*(executeBaton->maxRows)); - if(!defines[col].ind) + if ( !error ) { - executeBaton->error = NJSMessages::getErrorMsg( errInsufficientMemory ); - return; - } - defines[col].len = (DPI_BUFLEN_TYPE *)malloc(sizeof(DPI_BUFLEN_TYPE)* - executeBaton->maxRows); - if(!defines[col].len) - { - executeBaton->error = NJSMessages::getErrorMsg( errInsufficientMemory ); - return; - } + defines[col].ind = (short*)malloc ( sizeof( short ) * + ( executeBaton->maxRows ) ); + if(!defines[col].ind) + { + executeBaton->error = NJSMessages::getErrorMsg( errInsufficientMemory ); + error = true; + } + defines[col].len = (DPI_BUFLEN_TYPE *)malloc(sizeof(DPI_BUFLEN_TYPE)* + executeBaton->maxRows); + if(!defines[col].len) + { + executeBaton->error = NJSMessages::getErrorMsg( errInsufficientMemory ); + error = true; + } - executeBaton->dpistmt->define(col+1, defines[col].fetchType, - (defines[col].buf) ? defines[col].buf : defines[col].extbuf, - defines[col].maxSize, defines[col].ind, defines[col].len); + executeBaton->dpistmt->define(col+1, defines[col].fetchType, + (defines[col].buf) ? defines[col].buf : defines[col].extbuf, + defines[col].maxSize, defines[col].ind, defines[col].len); + } } - executeBaton->numCols = numCols; } /*****************************************************************************/ diff --git a/src/njs/src/njsOracle.cpp b/src/njs/src/njsOracle.cpp index ceef57269..fcf058586 100644 --- a/src/njs/src/njsOracle.cpp +++ b/src/njs/src/njsOracle.cpp @@ -383,9 +383,21 @@ NAN_GETTER(Oracledb::GetMaxRows) */ NAN_SETTER(Oracledb::SetMaxRows) { - Oracledb* oracledb = Nan::ObjectWrap::Unwrap(info.Holder()); + unsigned int tempMaxRows = NJS_MAX_ROWS; + Oracledb* oracledb = Nan::ObjectWrap::Unwrap(info.Holder()); + NJS_CHECK_OBJECT_VALID(oracledb); - NJS_SET_PROP_UINT(oracledb->maxRows_, value, "maxRows"); + NJS_SET_PROP_UINT(tempMaxRows, value, "maxRows"); + + if ( tempMaxRows <= 0 ) + { + string errMsg = NJSMessages::getErrorMsg ( errInvalidmaxRows ); + NJS_SET_EXCEPTION( errMsg.c_str(), errMsg.length() ); + } + else + { + oracledb->maxRows_ = tempMaxRows; + } } /*****************************************************************************/ diff --git a/test/nestedCursor.js b/test/nestedCursor.js index 33349e220..426ec7528 100644 --- a/test/nestedCursor.js +++ b/test/nestedCursor.js @@ -24,6 +24,9 @@ * DESCRIPTION * Testing nested cursor. * + * Note: Nested cursor is still not a supported data type. So NJS-010 + * error is expected. + * * NUMBERING RULE * Test numbers follow this numbering rule: * 1 - 20 are reserved for basic functional tests @@ -40,6 +43,7 @@ var dbConfig = require('./dbconfig.js'); describe('57. nestedCursor.js', function() { + var connection = null; var createParentTable = "BEGIN \ DECLARE \ @@ -55,7 +59,7 @@ describe('57. nestedCursor.js', function() { CREATE TABLE nodb_parent_tab ( \ id NUMBER, \ description VARCHAR2(32), \ - CONSTRAINT parent_tab_pk PRIMARY KEY (id) \ + CONSTRAINT nodb_parent_tab_pk PRIMARY KEY (id) \ ) \ '); \ EXECUTE IMMEDIATE (' \ @@ -91,8 +95,8 @@ describe('57. nestedCursor.js', function() { id NUMBER, \ parent_id NUMBER, \ description VARCHAR2(32), \ - CONSTRAINT child_tab_pk PRIMARY KEY (id), \ - CONSTRAINT child_parent_fk FOREIGN KEY (parent_id) REFERENCES nodb_parent_tab(id) \ + CONSTRAINT nodb_child_tab_pk PRIMARY KEY (id), \ + CONSTRAINT nodb_child_parent_fk FOREIGN KEY (parent_id) REFERENCES nodb_parent_tab(id) \ ) \ '); \ EXECUTE IMMEDIATE (' \ @@ -128,13 +132,12 @@ describe('57. nestedCursor.js', function() { END; "; var cursorExpr = - "CREATE OR REPLACE PROCEDURE cursor_parent_child (p_out OUT SYS_REFCURSOR) \ + "CREATE OR REPLACE PROCEDURE nodb_cursor_parent_child (p_out OUT SYS_REFCURSOR) \ AS \ BEGIN \ OPEN p_out FOR \ SELECT p "; - var connection = false; before(function(done) { async.series([ function(callback) { @@ -199,27 +202,6 @@ describe('57. nestedCursor.js', function() { ], done); }) - function fetchOneRowFromRS(rs, cb) { - rs.getRow(function(err, row) { - if(err) { - // NJS-010: unsupported data type in select list - (err.message).should.startWith('NJS-010:'); - rs.close(function(err) { - should.not.exist(err); - cb(); - }); - } else if(row) { - console.log(row); - fetchOneRowFromRS(rs, cb); - } else { - rs.close(function(err) { - should.not.exist(err); - cb(); - }); - } - }); - } - it('57.1 testing nested cursor support - result set', function(done) { connection.should.be.ok(); @@ -237,17 +219,19 @@ describe('57. nestedCursor.js', function() { [], { resultSet: true }, function(err, result) { - should.not.exist(err); - should.exist(result.resultSet); - fetchOneRowFromRS(result.resultSet, done); + should.exist(err); + (err.message).should.startWith('NJS-010:'); + // NJS-010: unsupported data type in select list + should.not.exist(result); + done(); } ); - }) + }) // 57.1 it('57.2 testing nested cursor support - REF Cursor', function(done) { var testproc = - "CREATE OR REPLACE PROCEDURE get_family_tree(p_out OUT SYS_REFCURSOR) \ + "CREATE OR REPLACE PROCEDURE nodb_get_family_tree(p_out OUT SYS_REFCURSOR) \ AS \ BEGIN \ OPEN p_out FOR \ @@ -272,19 +256,22 @@ describe('57. nestedCursor.js', function() { }, function(callback){ connection.execute( - "BEGIN get_family_tree(:out); END;", + "BEGIN nodb_get_family_tree(:out); END;", { out: { type: oracledb.CURSOR, dir: oracledb.BIND_OUT } }, function(err, result) { - should.not.exist(err); - fetchOneRowFromRS(result.outBinds.out, callback); + should.exist(err); + (err.message).should.startWith('NJS-010:'); + // NJS-010: unsupported data type in select list + should.not.exist(result); + callback(); } ); }, function(callback) { connection.execute( - "DROP PROCEDURE get_family_tree", + "DROP PROCEDURE nodb_get_family_tree", function(err, result) { should.not.exist(err); callback(); @@ -292,5 +279,6 @@ describe('57. nestedCursor.js', function() { ); } ], done); - }) + }) // 57.2 + }) diff --git a/test/resultSet2.js b/test/resultSet2.js index 40259ae40..ee278cdaa 100644 --- a/test/resultSet2.js +++ b/test/resultSet2.js @@ -745,47 +745,22 @@ describe('55. resultSet2.js', function() { describe('55.11 result set with unsupported data types', function() { - var sql2 = "SELECT dummy, rowid FROM dual"; - - function fetchOneRowFromRS(rs, cb) { - rs.getRow(function(err, row) { - /* Currently, even if the driver doesn't support certain data type - * the result set can still be created. - */ - // Error at accessing RS - should.exist(err); - if(err) { - // console.error("Error at accessing RS: " + err.message); - // NJS-010: unsupported data type in select list - (err.message).should.startWith('NJS-010:'); - rs.close( function(err) { - should.not.exist(err); - cb(); - }); - } else if(row) { - console.log(row); - fetchOneRowFromRS(rs, cb); - } else { - rs.close( function(err) { - should.not.exist(err); - cb(); - }); - } - }); - } - it('55.11.1 ROWID date type', function(done) { connection.execute( - sql2, + "SELECT dummy, rowid FROM dual", [], { resultSet: true }, function(err, result) { - should.not.exist(err); - fetchOneRowFromRS(result.resultSet, done); + should.exist(err); + (err.message).should.startWith('NJS-010:'); + // NJS-010: unsupported data type in select list + should.not.exist(result); + done(); } ); }) - }) + + }) // 55.11 describe.skip('55.12 bind a cursor BIND_INOUT', function() { diff --git a/test/stream1.js b/test/stream1.js index 5866368df..1fe632a64 100644 --- a/test/stream1.js +++ b/test/stream1.js @@ -594,7 +594,7 @@ describe('13. stream1.js', function () { }); }); - it('13.3.2 should default to 100 if oracledb.maxRows is false', function (done) { + it('13.3.2 Negative - should fail with NJS-026 if oracledb.maxRows is zero', function (done) { var defaultMaxRows; var testMaxRows = 0; @@ -602,30 +602,15 @@ describe('13. stream1.js', function () { defaultMaxRows = oracledb.maxRows; - oracledb.maxRows = testMaxRows; - - var stream = connection.queryStream('SELECT employee_name FROM nodb_stream1 ORDER BY employee_name'); - - stream.on('data', function () { - stream.pause(); - - // Using the internal/private caches to validate - should.equal(stream._fetchedRows.length, (99 - stream._readableState.buffer.length)); - stream._close(); - }); - - stream.on('close', function() { + try { + oracledb.maxRows = testMaxRows; + } catch (err) { + should.exist(err); + (err.message).should.startWith('NJS-026:'); + // NJS-026: maxRows must be greater than zero oracledb.maxRows = defaultMaxRows; done(); - }); - - stream.on('end', function () { - done(new Error('Reached the end of the stream')); - }); - - stream.on('error', function (err) { - done(err); - }); + } }); }); });