Re: More strict bind param count checking - Mailing list pgsql-odbc

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;