oracle / odpi

ODPI-C: Oracle Database Programming Interface for Drivers and Applications
https://oracle.github.io/odpi/
Other
269 stars 78 forks source link

CQN notification objects are not freed correctly, causing segmentation faults #96

Closed K900 closed 5 years ago

K900 commented 5 years ago

The code responsible for freeing CQN message objects contains a typo that causes occasional segfaults, use-after-frees, memory leaks and other related weirdness:

dpiSubscr.cpp, line 279:

for (j = 0; j < query->numTables; j++) {
    if (query->tables[i].numRows > 0)
        dpiUtils__freeMemory(query->tables[i].rows);
}

The code erroneously uses i as a counter for freeing tables, where it should be j.

K900 commented 5 years ago

I've also pushed a fixed branch to my fork of the repo, but the policy here prevents me from submitting a pull request.

ddevienne commented 5 years ago

Such a bug would not happen with C++11 and range-for :) At one point, ODPI should take the plunge on modern C++, for a future version. Any plans in that direction, to use the 8 years old standard now?

anthony-tuininga commented 5 years ago

Good catch! It isn't commonly discovered due to the simple fact that most of the time only a single table is being reported. :-)

K900 commented 5 years ago

The way we encountered the issue was with multiple queries and a single table, making this an out-of-bounds free.

K900 commented 5 years ago

Also, any chance of backporting this to the stable branch?

anthony-tuininga commented 5 years ago

You mean 3.1.x?

K900 commented 5 years ago

Yes. I've built a fixed version locally, but it would be nice to have an upstream release.

anthony-tuininga commented 5 years ago

Can you verify that the changes I just made match the ones you made yourself?

K900 commented 5 years ago

The diffs seem identical: mine, yours.

anthony-tuininga commented 5 years ago

Excellent. Thanks.

anthony-tuininga commented 5 years ago

Pushed to the v3.1.x branch as well.

K900 commented 5 years ago

Thanks! Waiting for the next release for this and cx_Oracle then :)

anthony-tuininga commented 5 years ago

Sure. I'll give it a few days before making the patch releases for ODPI-C and cx_Oracle, though!

K900 commented 5 years ago

I've got a long running process over the weekend that's using CQN heavily (and that was crashing every few hours before the fix). I can report back on Monday if it survives :)

anthony-tuininga commented 5 years ago

That would be appreciated. Thanks!

K900 commented 5 years ago

Good news: the process has survived for this whole time - about 4 days total - with no issues.

cjbj commented 5 years ago

@K900 that's great. We'll schedule new releases soon.

anthony-tuininga commented 5 years ago

Released in ODPI-C 3.1.2.