Thread: More strict bind param count checking
Hello, I attach patch for more strict bind parameters count checking. It could avoid some bad behaviour (driver doesn't fail in some situations). Please review and comment Luf
Attachment
> -----Original Message----- > From: pgsql-odbc-owner@postgresql.org > [mailto:pgsql-odbc-owner@postgresql.org] On Behalf Of Ludek Finstrle > Sent: 14 December 2005 00:25 > To: pgsql-odbc@postgresql.org > Subject: [ODBC] More strict bind param count checking > > Hello, > > I attach patch for more strict bind parameters count checking. > It could avoid some bad behaviour (driver doesn't fail in some > situations). OK, just reading the patch, it appears to remove a check to ensure ipdopts isn't null which is perhaps not such a good thing. Other than that nothing stands out as being horrendously wrong. Can you apply it to CVS if you're happy with it please? I'm still pretty busy ATM, and am likely to be for a few days at least :-( Regards, Dave.
> > I attach patch for more strict bind parameters count checking. > > It could avoid some bad behaviour (driver doesn't fail in some > > situations). > > OK, just reading the patch, it appears to remove a check to ensure > ipdopts isn't null which is perhaps not such a good thing. Other than > that nothing stands out as being horrendously wrong. I take a look at it. But as I know ipdopts may exists during whole stmt life. I get the code from convert.c (I don't remember func name). There is no ipdopts check (I don't say now it's well). > Can you apply it to CVS if you're happy with it please? I'm still pretty > busy ATM, and am likely to be for a few days at least :-( I'll try in the evening or maybe at Friday (tomorrow I'm going to official journey). Thanks, Luf
Hi Ludek, thanks to your recent effort to improve the psqlodbc driver.
Please let me comment on your patch a little.
Ludek Finstrle wrote:
> Hello,
>
> I attach patch for more strict bind parameters count checking.
> It could avoid some bad behaviour (driver doesn't fail in some
> situations).
>
> Please review and comment
>
> Luf
>
>
> ------------------------------------------------------------------------
>
> diff -c psqlodbc.orig\convert.c psqlodbc\convert.c
> *** psqlodbc.orig\convert.c Mon Dec 05 20:20:53 2005
> --- psqlodbc\convert.c Wed Dec 14 00:58:24 2005
> ***************
> *** 1858,1866 ****
> QB_Destructor(qb);
> return SQL_ERROR;
> }
> ! if (marker_count > 0)
> {
> ! if (ipdopts && (ipdopts->allocated == marker_count))
> {
> CVT_APPEND_CHAR(qb, '(');
> for (i = 0; i < marker_count; i++)
> --- 1858,1866 ----
> QB_Destructor(qb);
> return SQL_ERROR;
> }
> ! if (ipdopts->allocated == marker_count)
Why are you using the operator '==' not '>=' ?
AFAIK there's no such limitation.
> {
> ! if (marker_count > 0)
> {
> CVT_APPEND_CHAR(qb, '(');
> for (i = 0; i < marker_count; i++)
> ***************
> *** 1871,1883 ****
> }
> CVT_APPEND_CHAR(qb, ')');
> }
> ! else
> ! {
> ! SC_set_error(stmt, STMT_COUNT_FIELD_INCORRECT, "The # of binded parameters < the # of parameter
markers");
> ! SC_set_sqlstate(stmt, "07002");
> ! QB_Destructor(qb);
> ! return SQL_ERROR;
> ! }
> }
> CVT_APPEND_STR(qb, " as ");
> for (qp->opos = 0; qp->opos < qp->stmt_len; qp->opos++)
> --- 1871,1884 ----
> }
> CVT_APPEND_CHAR(qb, ')');
> }
> ! }
> ! else
> ! {
> ! CC_clear_error(stmt->hdbc);
> ! SC_set_error(stmt, STMT_COUNT_FIELD_INCORRECT, "The # of binded parameters < the # of parameter
markers");
> ! SC_set_sqlstate(stmt, "07002");
> ! QB_Destructor(qb);
> ! return SQL_ERROR;
> }
> CVT_APPEND_STR(qb, " as ");
> for (qp->opos = 0; qp->opos < qp->stmt_len; qp->opos++)
> diff -c psqlodbc.orig\execute.c psqlodbc\execute.c
> *** psqlodbc.orig\execute.c Tue Dec 06 21:53:30 2005
> --- psqlodbc\execute.c Wed Dec 14 01:18:40 2005
> ***************
> *** 136,141 ****
> --- 136,143 ----
> StatementClass *stmt = (StatementClass *) hstmt;
> RETCODE result;
> CSTR func = "PGAPI_ExecDirect";
> + IPDFields *ipdopts;
> + SWORD marker_count;
>
> mylog("%s: entering...\n", func);
>
> ***************
> *** 156,161 ****
> --- 158,175 ----
>
> mylog("**** %s: hstmt=%u, statement='%s'\n", func, hstmt, stmt->statement);
>
> + /* Check bind parameters count */
> + if (SQL_SUCCESS != PGAPI_NumParams(stmt, &marker_count))
> + return SQL_ERROR;
> + ipdopts = SC_get_IPDF(stmt);
> + if (ipdopts->allocated != marker_count)
Why are you using '!=' not '<' ?
> + {
> + CC_clear_error(stmt->hdbc);
> + SC_set_error(stmt, STMT_COUNT_FIELD_INCORRECT, "The # of binded parameters < the # of parameter markers");
> + SC_set_sqlstate(stmt, "07002");
> + return SQL_ERROR;
> + }
> +
> /*
> * If an SQLPrepare was performed prior to this, but was left in the
> * premature state because an error occurred prior to SQLExecute then
> ***************
> *** 196,206 ****
> --- 210,234 ----
> APDFields *apdopts;
> IPDFields *ipdopts;
> BOOL prepare_before_exec = FALSE;
> + SWORD marker_count;
> +
>
> *exec_end = FALSE;
> conn = SC_get_conn(stmt);
> mylog("%s: copying statement params: trans_status=%d, len=%d, stmt='%s'\n", func, conn->transact_status,
strlen(stmt->statement),stmt->statement);
>
> + /* Check bind parameters count */
> + if (SQL_SUCCESS != PGAPI_NumParams(stmt, &marker_count))
> + return SQL_ERROR;
> + ipdopts = SC_get_IPDF(stmt);
> + if (ipdopts->allocated != marker_count)
The same comment as the previous one.
In addtion, the check is not appropriate because Exec_with_parameter_resolved
could be called with insufficient parameters.
> + {
> + CC_clear_error(stmt->hdbc);
> + SC_set_error(stmt, STMT_COUNT_FIELD_INCORRECT, "The # of binded parameters < the # of parameter markers");
> + SC_set_sqlstate(stmt, "07002");
> + return SQL_ERROR;
> + }
> +
> /* save the cursor's info before the execution */
> cursor_type = stmt->options.cursor_type;
> scroll_concurrency = stmt->options.scroll_concurrency;
> Hi Ludek, thanks to your recent effort to improve the psqlodbc driver. > Please let me comment on your patch a little. Hello, your comments are welcome. > > ! if (ipdopts->allocated == marker_count) > > Why are you using the operator '==' not '>=' ? > AFAIK there's no such limitation. We have problems with Visual FoxPro. It calls SQLCancel without FreeStmt(SQL_RESET_PARAMS). It leads to failure becouse next ExecDirect with fewer parameters in some cases think that it has data_at_exec (which was parameters from previous). So I think this could be good prevent againist driver failure. > > + if (ipdopts->allocated != marker_count) > > Why are you using '!=' not '<' ? It's the same as previous (only negated). > > + ipdopts = SC_get_IPDF(stmt); > > + if (ipdopts->allocated != marker_count) > > The same comment as the previous one. > In addtion, the check is not appropriate because Exec_with_parameter_resolved > could be called with insufficient parameters. I take a look at it again. Thanks for comments Luf
Ludek Finstrle wrote: >>Hi Ludek, thanks to your recent effort to improve the psqlodbc driver. >>Please let me comment on your patch a little. > > > Hello, > > your comments are welcome. > > >>>! if (ipdopts->allocated == marker_count) >> >>Why are you using the operator '==' not '>=' ? >>AFAIK there's no such limitation. > > > We have problems with Visual FoxPro. It calls SQLCancel without > FreeStmt(SQL_RESET_PARAMS). It leads to failure becouse next > ExecDirect with fewer parameters in some cases think that it > has data_at_exec (which was parameters from previous). Isn't it the right solution for the problem to check data_at_exec appropriately ? regards, Hiroshi Inoue
> >>>! if (ipdopts->allocated == marker_count) > >> > >>Why are you using the operator '==' not '>=' ? > >>AFAIK there's no such limitation. > > > >We have problems with Visual FoxPro. It calls SQLCancel without > >FreeStmt(SQL_RESET_PARAMS). It leads to failure becouse next > >ExecDirect with fewer parameters in some cases think that it > >has data_at_exec (which was parameters from previous). > > Isn't it the right solution for the problem to check data_at_exec > appropriately ? You're absolutely right. Thank you for showing me another view. This is bad patch. I'll try another way. Luf
> >We have problems with Visual FoxPro. It calls SQLCancel without > >FreeStmt(SQL_RESET_PARAMS). It leads to failure becouse next > >ExecDirect with fewer parameters in some cases think that it > >has data_at_exec (which was parameters from previous). > > Isn't it the right solution for the problem to check data_at_exec > appropriately ? Please Hiroshi could you take a look on solution for data_at_exec check? I see no problem but I didn't see it in previous patch also. Thanks a lot Luf
Attachment
Ludek Finstrle wrote:
>>>We have problems with Visual FoxPro. It calls SQLCancel without
>>>FreeStmt(SQL_RESET_PARAMS). It leads to failure becouse next
>>>ExecDirect with fewer parameters in some cases think that it
>>>has data_at_exec (which was parameters from previous).
>>>
>>>
>>Isn't it the right solution for the problem to check data_at_exec
>>appropriately ?
>>
>>
>
>Please Hiroshi could you take a look on solution for data_at_exec check?
>I see no problem but I didn't see it in previous patch also.
>
>Thanks a lot
>
>Luf
>
>
>------------------------------------------------------------------------
>
>diff -c psqlodbc.orig\execute.c psqlodbc\execute.c
>*** psqlodbc.orig\execute.c Tue Dec 06 21:53:30 2005
>--- psqlodbc\execute.c Fri Dec 16 18:11:16 2005
>***************
>*** 542,547 ****
>--- 542,548 ----
> UInt4 offset = apdopts->param_offset_ptr ? *apdopts->param_offset_ptr : 0;
> Int4 bind_size = apdopts->param_bind_type;
> Int4 current_row = stmt->exec_current_row < 0 ? 0 : stmt->exec_current_row;
>+ SWORD param_count;
>
> /*
> * Increment the number of currently processed rows
>***************
>*** 549,555 ****
> if (ipdopts->param_processed_ptr)
> (*ipdopts->param_processed_ptr)++;
> stmt->data_at_exec = -1;
>! for (i = 0; i < apdopts->allocated; i++)
> {
> Int4 *pcVal = apdopts->parameters[i].used;
>
>--- 550,559 ----
> if (ipdopts->param_processed_ptr)
> (*ipdopts->param_processed_ptr)++;
> stmt->data_at_exec = -1;
>! /* Check bind parameters count */
>! if (SQL_SUCCESS != PGAPI_NumParams(stmt, ¶m_count))
>! return SQL_ERROR;
>! for (i = 0; i < param_count; i++)
>
>
Looks pretty good to me.
It's more safe to adjust the param_count in case of apdopts->allocated <
param_count.
Also it's safe to change the following code in PGAPI_ParamData.
/* At least 1 data at execution parameter, so Fill in the token value */
for (; i < apdopts->allocated; i++)
regars, Hiroshi Inoue.
> Looks pretty good to me. Very good. > It's more safe to adjust the param_count in case of apdopts->allocated < > param_count. Thanks fot pointing it. > Also it's safe to change the following code in PGAPI_ParamData. > > /* At least 1 data at execution parameter, so Fill in the token value */ > for (; i < apdopts->allocated; i++) I implement it both. New patch attached. I also change older test for param number from == to >= and comment out some not used code. Thank you very much again Hiroshi Luf P.S. Dave can I commit it if there it no negative answer?
Attachment
-----Original Message----- From: pgsql-odbc-owner@postgresql.org on behalf of Ludek Finstrle Sent: Sat 12/17/2005 8:50 PM To: Hiroshi Inoue Cc: pgsql-odbc@postgresql.org Subject: Re: [ODBC] More strict bind param count checking > S. Dave can I commit it if there it no negative answer? Yep, unless it's a no-brainer (i.e. obviously right), then leave it for 3 or 4 days, and if no-one bleats go ahead and commit. It's CVS of course, so if there are any problems later we can always back it out easily enough. Regards, Dave.