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

Array to string conversion on exception with query parameters #601

Closed
hschletz opened this issue Jan 17, 2020 · 6 comments
Closed

Array to string conversion on exception with query parameters #601

hschletz opened this issue Jan 17, 2020 · 6 comments
Assignees
Labels
bug core ADOdb core (library and base classes)
Milestone

Comments

@hschletz
Copy link
Contributor

The ADODB_Exception constructor does not handle queries with parameter arrays. Patch attached.

adodb-fix-exception-param.patch.txt

@mnewnham mnewnham added the core ADOdb core (library and base classes) label Jan 18, 2020
@mnewnham
Copy link
Contributor

Could you provide a simple code snippet to simplify the testing process please.

@hschletz
Copy link
Contributor Author

I can't tell the exact circumstances that create the error condition. In my case, the problem occurs when a query raises an exception, but only when the code is called by PHPUnit. PHPUnit's own error handling may contribute to the problem. I could not reproduce the issue with a simple test script, other than invoking the ADODB_Exception constructor directly.

Here's the backtrace that leads to the warning (last call first, i.e. the warning is raised in adodb-exceptions.inc.php:45):

adodb-exceptions.inc.php:45
adodb-exceptions.inc.php:80
adodb.inc.php:306
adodb.inc.php:1277
adodb.inc.php:1237
adodb.inc.php:1808
adodb.inc.php:1641

The $fn parameter has the value "adodb_throw", leading to the code path in line 45. The problematic $p2 parameter contains the query parameters as an array, leading to the "array to string conversion" in that line.
The "CONNECT" and "PCONNECT" cases would be affected by the same issue. My patch converts the parameter manually, possibly breaking the "EXECUTE" case. Maybe $fn should be checked for "EXECUTE" before the conversion.

@hschletz
Copy link
Contributor Author

adodb-fix-exception-param-v2.patch.txt
Updated patch: don't mangle $p2 in case of EXECUTE.

@dregad
Copy link
Member

dregad commented Jan 24, 2020

@hschletz thanks for your contribution. It would be nice if you could submit patches via pull requests instead of attaching diff files, it would make it easier for us to review and merge.

@dregad
Copy link
Member

dregad commented Jan 29, 2020

@hschletz Thanks for the PR. Before merging it, I'd like to have a better understanding of the error's root cause. This is a bit difficult as you did not provide full stack trace details including function names and parameters.

Nevertheless, the adodb.inc.php:306 call (ADODB_TransMonitor function) seems to be the key*.

Can you please confirm that you are using transactions (specifically, StartTrans and CompleteTrans) in your code ? I believe the explanation for the error could be that the $fn parameter in ADODB_TransMonitor() (the value of which I assume is EXECUTE, based on the call at line 1277) is overwritten (line 305) by the original exception thrower (ie. adodb_throw).

If so, then maybe the correct fix for the problem is to rename the $fn variable in ADODB_TransMonitor(), like this:

diff --git a/adodb.inc.php b/adodb.inc.php
index 74c52e00..2a7c1ca9 100644
--- a/adodb.inc.php
+++ b/adodb.inc.php
@@ -302,8 +302,8 @@ if (!defined('_ADODB_LAYER')) {
        //print "Errorno ($fn errno=$errno m=$errmsg) ";
        $thisConnection->_transOK = false;
        if ($thisConnection->_oldRaiseFn) {
-           $fn = $thisConnection->_oldRaiseFn;
-           $fn($dbms, $fn, $errno, $errmsg, $p1, $p2,$thisConnection);
+           $errfn = $thisConnection->_oldRaiseFn;
+           $errfn($dbms, $fn, $errno, $errmsg, $p1, $p2,$thisConnection);
        }
    }

Can you please test and let me know ?

  • Note: you did not specify which version you're using, so I'm assuming you're running 5.20.16 to interpret the line numbers.

@hschletz
Copy link
Contributor Author

test-issue-601.php.txt
You are right, the problem occurs when exceptions get thrown within a transaction. So I was finally able to reproduce it with the attached test script.

Your patch works for me (applied against 5.20.16, as you guessed correctly).

@dregad dregad added the bug label Jan 30, 2020
@dregad dregad added this to the v5.20.17 milestone Jan 30, 2020
@dregad dregad self-assigned this Jan 30, 2020
@dregad dregad closed this as completed in 0e813bc Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug core ADOdb core (library and base classes)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants