Fix garbled diagnostic record on failed SQLDriverConnect#298
Conversation
Two regression tests verify that when SQLDriverConnect fails the returned diagnostic record is well-formed: a 5-character SQLSTATE, a message length that matches strlen of the message, and a message body that is more than a single byte. The wrong-password test skips itself gracefully if the underlying Firebird server uses embedded or trusted authentication (the configuration used by CI), because in that mode the bad password is not rejected and the failure path can't be exercised.
|
@irodushka back from vacation? 😉 Another one similar to #296. As with that one, it would be great if this could make it into -rc2 as well. This would allow us to run the HammerDB benchmark on Linux. P.S. I’ve published a new unofficial build based on the latest |
|
Hi @fdcastel Yeah I'm online! What about this. AI - am I right about that?.. - is trying to solve a trivial bug (that's really very bad, inaccurate, ugly, and the dev hands should be teared off!) with so comlpex & muddy means. A helper foo, and hell, dynamic_cast... My Geniune Intelligence (I will abbreviate it to GI) tells me, that the much better approach is to separate exception handlers, in a standard way, like this: If you're anxious about copypasting - you can use lambda or macrodef. What about postError() - 1) I cannot understand what's the need for calling the extractExceptionInfo() helper in the overloaded version if you definitely know the exception type here 2) IMHO It's better to make a templated foo, and gate the logic with the |
|
Welcome back, man! Sorry about this one. It was a quick fix I put together in a rush while trying to solve an issue on Linux. But ultimately I had to drop it (Unicode on Linux strikes again). I’ll submit an update with your suggestions, and you can tell me what you think. But in the end, we can discard this one if you still don't like it (since I ended up not using it, after all). |
fdcastel
force-pushed
the
fix/sqldriverconnect-diag-record
branch
from
c7922bb to
ec2e608
Compare
Every catch block that handled std::exception followed the same broken
pattern:
catch ( std::exception &ex ) {
SQLException &exception = (SQLException&)ex;
// virtual calls on `exception`...
}
The C-style cast is a reinterpret_cast. When the caught object was not
in fact a SQLException -- e.g. a std::bad_alloc, a raw FbException
that bypassed the IscDbc rewrap, or anything else that propagated out
of the IscDbc layer -- the subsequent virtual dispatch through
`exception` read through whatever bytes lay where SQLException's
vtable pointer would have been. On Linux that produced a diagnostic
record with an empty SQLSTATE, a non-deterministic native code, and a
one-byte message ('[') whose reported length disagreed with its
strlen.
This commit replaces each single `catch (std::exception &ex)` with the
idiomatic two-handler form:
catch (SQLException &ex) { ... }
catch (std::exception &ex) { ... }
C++ exception-matching picks the most-derived handler at runtime, so
the SQLException catch sees only true SQLException objects and the
std::exception catch sees only non-SQLException objects -- no
dynamic_cast or helper required.
A companion postError(state, std::exception&) overload pairs with the
existing postError(state, SQLException&); each catch dispatches the
right one statically based on the type of `ex`.
All 64 occurrences of the broken pattern across OdbcConnection /
OdbcStatement / OdbcDesc / OdbcEnv and the Windows OdbcJdbcSetup tree
are converted to this form.
OdbcConnection::connect gets a small rollbackPartialConnect lambda so
its two catch bodies stay short, plus a NULL guard around
connection->close() -- if createConnection() throws before assignment,
the previous code dereferenced a NULL pointer during cleanup.
|
Thanks for the review @irodushka — fully agreed. Pushed ec2e608 which scraps the catch (SQLException &ex) { ... ex.getText() / ex.getSqlcode() ... }
catch (std::exception &ex) { ... ex.what() ... }C++ catch matching picks the right handler statically; no runtime type check. For
Note on Full matrix is green on the new HEAD: Windows x64 / x86 / ARM64, Linux x64 / ARM64, Linux + Valgrind, against Firebird 5.0.3 and master. |
|
+++ 86c2d02 |
Follow-up to df437ea / 86c2d02, which made SQLException's getters const and updated the postError overloads + DsnDialog.cpp catch site. This commit applies the same `const` to the remaining 63 catch pairs across OdbcConnection, OdbcStatement, OdbcDesc, OdbcEnv, and the rest of the OdbcJdbcSetup tree, so the codebase is uniformly: catch (const SQLException &ex) { ... } catch (const std::exception &ex) { ... } Pure mechanical change; no behavioral effect.
|
Done! Pulled both commits and ran a sweep over the remaining 63 catch pairs in fd06bf2. All |
2 participants
Summary
Every catch block that handled
std::exceptionfollowed the same broken pattern:The C-style cast is a
reinterpret_cast. When the caught object was not in fact aSQLException(e.g. astd::bad_alloc, a rawFbExceptionthat bypassed the IscDbc rewrap, or anything else that propagated out of the IscDbc layer), the subsequent virtual dispatch throughexceptionread through whatever bytes lay whereSQLException's vtable pointer would have been. On Linux that produced a diagnostic record with:SQLSTATE,[) whose reported length disagreed with itsstrlen.ODBC clients (
unixODBC'sisql,pyodbc, Tcl'stdbc::odbc, …) cannot recover useful information from such a record, so application-level retry/branch logic onSQLSTATEor native code is impossible.What this PR does
ExceptionInfohelper plus a newpostError(state, std::exception&)overload inIscDbc/SQLException.h/OdbcObject. Both route throughdynamic_cast<SQLException*>, falling back tostd::exception::what()when the caught object isn't aSQLException.OdbcConnection,OdbcStatement,OdbcDesc,OdbcEnv, and the WindowsOdbcJdbcSetuptree to the safe path.connection->close()inOdbcConnection::connectso a throw insidecreateConnection()(beforeconnectionis assigned) no longer dereferences NULL during cleanup.tests/test_connect_options.cpp:BadPasswordReturnsValidDiagRec— verifies the diag record after a wrong-password failure (skips itself in embedded/trusted-auth setups that don't reject the password, including CI).NonexistentDatabaseReturnsValidDiagRec— verifies the diag record after a connect to a nonexistent database path.Both tests assert that after
SQLDriverConnectfails the diag record has a 5-characterSQLSTATE, a reported message length matchingstrlenof the message, and a message longer than a single byte.Verification
CI matrix (all green on the head of this PR):
Tested against Firebird 5.0.3 and the Firebird master snapshot.
Test plan
NonexistentDatabaseReturnsValidDiagRectest passes on all platformsBadPasswordReturnsValidDiagRectest runs (skips gracefully when the server doesn't reject the bad password)grep -rn '(SQLException&)' *.cpp *.h IscDbc/ OdbcJdbcSetup/returns no remaining unchecked casts