Avoid hardly debugable PHP.exe crashes

[KoudelkaB]

The current code causes hardly debugable PHP.exe crashes in case when PHP developer does not release query or results in proper order. This is just minimal fix of just one such scenario. Architectural fix would be better of course i.e. defining supported scenarios and throwing regular PHP exception instead of crashing PHP.exe in unsuported cases.

THE FIX: Avoids memory overwrites of released query or result (the memory already allocated by somebody else) or in case of more cursors opened for one query at the time of query destruction.

[MartinKoeditz]

Looks good. Do you have an example for such crashes? In that case I can implement a test case for the qa.

[KoudelkaB]

The crashes are hard to simulate because it depend on what exact memory you overwrite. But you can easily simulate the problem when you execute from PHP the same query more times and never release the query results. Then after the query is released the results are still hanging there with invalid reference to the same query (except the last lucky one that NULLed the query as expected) In my case the memory used previously by the query was in the moment of other result destructor occupied by unrelated zend_object and the ib_query->result->query = NULL actualy deleted it's member ce which caused the php.exe crash on ACCESS_VIOLATION when ending the process in zend_objects_store_call_destructors on the expression "obj->ce->destructor".

As you see the code is absolutely not designed to handle more opened query results at the same time and could basicaly fail anywhere when the situation occures. That is why I have added the second commint to this PR where I explicitely forbid this situation to happen.

[KoudelkaB]

I know also about another php.exe crash that I did not investigate precisely. I only know that the first commit here did not help in that case but the proper closing of the query result in PHP code did - so it was most probably a bug of the same origin.

[KoudelkaB]

I am working on better fix by making the query statement reference counted to avoid the problematic week pointers completely.

[KoudelkaB]

I have updated PR with better fix. I have created resource for reference counting of statement to properly share it between query and result and call destructor after nobody use it.

[MartinKoeditz]

At first glance it looks good. We have to check whether existing codes can still be run. In particular, when dealing with transactions, there must be no breaks. I don't see this as a problem at the moment.

Do you already have experience regarding code breaks?

[KoudelkaB]

Concerning "code breaks" I am not sure if I understand, what do you mean. The only thing I changed was exchanging the week result/query pointers by reference counts - As you see I did not touch transactions. So if the code worked before with transatrions now it should not be worse... So far I have tested two scenarios in our PHP code:

1. queury is destructed before last result - then the statement is destructed during the last result destruction.
2. query is destructed after all results - then it also destructs the statement pointing to.

[KoudelkaB]

The only thing I am not sure if I handeled it well is the code ib_query.stmt = 0; /* keep stmt when free query */ on the line 1244 and result->stmt = 0; on the line 1884. I did not touch them because if I nulled "stms" in the shared resource as well then the statement would never be destructed! But I am not sure why do you null it here and thus what appropriate action should be taken on corresponding ib_query.stmt_res resp. result->stmt_res? I hope that these two lines only somehow workarouded the original wrong design and now can be deleted?

[MartinKoeditz]

Hm, we will have to check, if we can delete these lines.

I didn't create the extension. Since nobody wanted to keep on the development I became the maintainer. So I'm still investigating the code.

I will check for errors in our ERP system. But I think it will turn out.