FirebirdSQL / firebird

Firebird server, client and tools
https://firebirdsql.org
1.26k stars 217 forks source link

Wrong error code output in EDS #8257

Open aafemt opened 2 months ago

aafemt commented 2 months ago

If the status vector contains isc_arg_sql_state, EDS code writes SQLSTATE's string address to error message instead of following GDS status like this:

Statement failed, SQLSTATE = 42000 
Execute statement error at attach : 
335544421 : connection rejected by remote interface 
140707296949552 : [unixODBC][SQLite]connect failed 

Here actual error code is isc_random (335544382), not 140707296949552.

hvlad commented 2 months ago

Provide full status-vector or minimal reproducible test case.

EDS uses common way to interprete status-vector, so I doubt it directly related. I see that safe_interpret() considers isc_arg_sql_state as 'always first' item in status-vector, which is obviously violated in your case. At last, I see no usage of isc_arg_sql_state in the current code of master branch and can't verify safe_interpret() behaviour.

aafemt commented 2 months ago

Provide full status-vector or minimal reproducible test case.

Full status vector: 1 335544569 19 140737020651594 1 335544382 2 1400737020651600.

I see that safe_interpret() considers isc_arg_sql_state as 'always first' item in status-vector, which is obviously violated in your case.

Yes it is. I never saw such requirement documented anywhere.

hvlad commented 2 months ago

I failed to find any docs re isc_arg_sql_state. It was introduced in 2.5 and backported from Vulcan, AFAIU. Consider to prepare the patch for safe_interpret(), or change your status-vector as safe_interpret() expects.

aafemt commented 2 months ago

Line const ISC_STATUS code = *p ? p[1] : 0; is not in safe_interpret() which works correctly (it correctly skips isc_arg_sql_state in any position). And changing of my status vector will fix nothing, just move the wrong number into a different line.

hvlad commented 2 months ago

Do you expect me to search all codebase for this line ?

aafemt commented 2 months ago

grep is a useful utility you know... It can show you IscDS.cpp:79 in no time.

hvlad commented 2 months ago

So you now have just one way to go. And use the useful utility you know to find the second instance of the same pattern.

PS all your messages with code, cite of docs, or anything else without references will be ignored by me.

aafemt commented 2 months ago

Unfortunately this utility is completely useless when I'm trying to understand why this code exists at all in the place where everything you need is to pass received status to caller as is.

hvlad commented 2 months ago

IscProvider::getRemoteError() as it name suggests, uses external provider's implementation of fb_interpret() to get both error codes and error string's from status-vector (that was created by external provider).

aafemt commented 2 months ago

Yes, this is exactly how it is described:

    // Interprete status and put error description into passed string
    virtual void getRemoteError(const Jrd::FbStatusVector* status, Firebird::string& err) const = 0;

Unfortunately, still no explanation why the error is thrown as a string instead of usual practice of prepending it with some prefix error code in the only place where this routine is called:

    // Execute statement error at @1 :\n@2Statement : @3\nData source : @4
    ERR_post(Arg::Gds(isc_eds_statement) << Arg::Str(sWhere) <<
                                            Arg::Str(rem_err) <<
                                            Arg::Str(sQuery ? sQuery->substr(0, 255) : m_sql.substr(0, 255)) <<
                                            Arg::Str(m_connection.getDataSourceName()));

Of course I can prepare a patch that will wipe all this pointless mangling completely. Will be here anyone willing to merge it?

hvlad commented 2 months ago

Unfortunately, still no explanation why the error is thrown as a string instead of usual practice of prepending it with some prefix error code in the only place where this routine is called:

I can't decrypt your statement. If you need explanations, make sure you can be understood.

Of course I can prepare a patch that will wipe all this pointless mangling completely. Will be here anyone willing to merge it?

What kind of help you expect after such wording ?

PS I can fix original issue with isc_arg_sql_state but can't verify the fix as this error code never used by Firebird. BTW, it also means that such a fix have near zero value for Firebird itself. Thus I not going to spend a time on it, at least currently.