Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
K900 opened this issue Feb 7, 2019 · 18 comments
Closed
Labels

Comments

@K900
Copy link

K900 commented Feb 7, 2019

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
Copy link
Author

K900 commented Feb 7, 2019

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
Copy link

ddevienne commented Feb 7, 2019

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
Copy link
Member

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
Copy link
Author

K900 commented Feb 7, 2019

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

@K900
Copy link
Author

K900 commented Feb 7, 2019

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

@anthony-tuininga
Copy link
Member

You mean 3.1.x?

@K900
Copy link
Author

K900 commented Feb 7, 2019

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

@anthony-tuininga
Copy link
Member

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

@K900
Copy link
Author

K900 commented Feb 7, 2019

The diffs seem identical: mine, yours.

@anthony-tuininga
Copy link
Member

Excellent. Thanks.

@anthony-tuininga
Copy link
Member

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

@K900
Copy link
Author

K900 commented Feb 7, 2019

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

@anthony-tuininga
Copy link
Member

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

@K900
Copy link
Author

K900 commented Feb 7, 2019

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
Copy link
Member

That would be appreciated. Thanks!

@K900
Copy link
Author

K900 commented Feb 11, 2019

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

@cjbj
Copy link
Member

cjbj commented Feb 11, 2019

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

@anthony-tuininga
Copy link
Member

Released in ODPI-C 3.1.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants