oracledb 2 core dumps

[sagiegurari]

I have few tests that I run using simple-oracledb to check that nothing is broken for every new version of oracledb.
I tried it with oracledb 2 and it core dumps.
You can see it at:
https://travis-ci.org/sagiegurari/misc-test/builds/244591174#L463
Test Code at (it tests many different capabilities, insert/delete/update/select/LOBs and so on..):
https://github.com/sagiegurari/simple-oracledb/blob/master/test/spec/integration-spec.js

The build does the following:

1. run docker image with oracle 11 (wnameless/oracle-xe-11g)
2. run docker image with oci and node (collinestes/docker-node-oracle) and in the dockerfile it looks like it is node 6 and oci client 12 (https://hub.docker.com/r/collinestes/docker-node-oracle/~/dockerfile/)
3. run tests inside the second docker after installing simple-oracledb, oracledb (dev2), mocha and so on...

You can see the above test definition for travis at (still playing with it to try it with oracle 12 docker image):
https://github.com/sagiegurari/misc-test/tree/oracle-docker

I have not investigated why it core dumps and which test exactly core dumps.

which latest oracledb version 1.13.1 the tests pass correctly:
https://travis-ci.org/sagiegurari/misc-test/builds/184018541

[anthony-tuininga]

Looking at the two builds, it looks like it is segfaulting within the "Update - LOB data" test. Do you have the SQL required to build the test schema separately? Or do I have to use the docker file provided?

[sagiegurari]

The tables, test data and test sqls are in the test itself at:
https://github.com/sagiegurari/simple-oracledb/blob/master/test/spec/integration-spec.js

[sagiegurari]

The test uses the following to create the tables and data
https://github.com/sagiegurari/simple-oracledb/blob/master/test/helpers/integration-helper.js
it is invoked at the start of each test and creates a new table for every test

[sagiegurari]

I see few tests are using same table by mistake... maybe thats the reason.
i'll fix that, but

1. not sure if it is the reason for the core dump (checking that now)
2. it should not core dump

[anthony-tuininga]

I'll give that a whirl. And yes, even bad code should not result in a segfault!

[sagiegurari]

finished to retest. it wasn't it. still core dumps.
you don't have to use those docker images. I use them because I don't have oracle installed.

[anthony-tuininga]

Ok. Thanks. Will let you know what I discover.

[sagiegurari]

@anthony-tuininga thanks.
Did more tests and indeed only "update - LOB data" test is failing.
rest seem to work correctly.

[sagiegurari]

Reran with the updated version of the dpi and got the following (sometimes core dump but now i got some error message that might help more):

Error: DPI-1002: invalid dpiLob handle

  Integration Tests
    connection
      update
        1) update - LOB data
  0 passing (162ms)
  1 failing
  1) Integration Tests connection update update - LOB data:
     Uncaught AssertionError: expected [Error: DPI-1002: invalid dpiLob handle] to equal null
      at onUpdate (test/spec/test.js:1020:56)
      at onWrapperCallback (lib/connection.js:1613:17)
      at fnMaxTimesWrapper (node_modules/funcs-js/funcs.js:221:28)
      at onExecute (lib/connection.js:500:9)
      at custExecuteCb (node_modules/oracledb/lib/connection.js:99:7)

[anthony-tuininga]

Yes, I get the same results -- sometimes core dumps and sometimes the error DPI-1002: invalid dpiLob handle. Investigations ongoing....

[anthony-tuininga]

Ok. I tracked it down. The problem doesn't actually occur in the test that is failing! It is actually due to a corruption that is taking place earlier in a DML returning statement that returns LOBs. There is a possibility that additional buffer space will need to be allocated and the old space will have been freed. ODPI-C has the ability to manage that so here is the patch that I have developed. It will need to be reviewed but in my tests the problem goes away and you can continue testing your own code with v2. Let me know how it goes.

--- a/src/njs/src/njsConnection.cpp
+++ b/src/njs/src/njsConnection.cpp
@@ -300,11 +300,16 @@ bool njsConnection::ProcessLOBs(njsBaton *baton, njsVariable *vars,
             default:
                 continue;
         }
-        if (baton->isReturning && var->bindDir == NJS_BIND_OUT)
+        if (baton->isReturning && var->bindDir == NJS_BIND_OUT) {
+            if (dpiVar_getData(var->dpiVarHandle, &numElements,
+                    &var->dpiVarData) < 0) {
+                baton->GetDPIError();
+                return false;
+            }
             numElements = (uint32_t) baton->rowsAffected;
-        else if (!var->isArray)
+        } else if (!var->isArray) {
             numElements = baseNumElements;
-        else {
+        } else {
             if (dpiVar_getNumElementsInArray(var->dpiVarHandle,
                     &numElements) < 0) {
                 baton->GetDPIError();

[sagiegurari]

thanks a lot. will rerunning npm install now on dev2 bring this new patch?

[anthony-tuininga]

Not yet. @cjbj will do that after it is reviewed, I believe. You can patch it yourself, however, if you don't want to wait. :-)

[sagiegurari]

that would be a bit hard since at the moment I only have travis and docker solution for testing.

[anthony-tuininga]

Ok. I guess you'll have to wait a bit, then. :-(

[anthony-tuininga]

Patch applied. Please confirm that this issue has been fixed, @sagiegurari . Thanks!

[sagiegurari]

looks like it is working now.

https://travis-ci.org/sagiegurari/misc-test/builds/244789515

Thanks a lot 👍