Hello again and let me continue my monologue here: On Mon, 21 May 2012 01:06:09 +0200 I wrote:
VZ> Anyhow, after this long introduction here is the problem description: when VZ> type_conversion is specialized for some type, the value to be written to VZ> the database is actually stored in 2 places: conversion_use_type::value_ VZ> and in its base details::base_value_holder::val_. It is transformed from VZ> the former to the latter by convert_to_base() method which is called by VZ> standard_use_type::pre_use() itself called from statement_impl::execute(). VZ> And the problem is that this is too late for ODBC because we call VZ> odbc_standard_use_type_backend::prepare_for_bind() before execute() (from VZ> statement::define_and_bind()) and by this time base_value_holder::val_ is VZ> still empty. So ODBC code calls SQLBindParameter() with size of 1 for it VZ> (it uses for empty string, probably because of trailing NUL concerns). And VZ> while the buffer is resized and filled with the correct value later (patch VZ> http://www.mail-archive.com/[email protected]/msg00957.html VZ> helps with another bug there but this isn't enough), we don't rebind the VZ> parameter at ODBC level again so it's never used correctly. VZ> VZ> Now I have no idea why do we actually write some junk to the database VZ> instead of writing a single byte Turns out that this is actually due to my own patch from the message linked above. As the patch there reallocates the buffer, SQLBindParameter() reads from the old, deallocated, buffer with expectedly bad results. But the behaviour is still wrong without the patch: for one, the buffer overflow it fixed is perfectly real and secondly the custom types still don't work. VZ> A simple solution would seem to call pre_use() from define_and_bind() VZ> instead of doing it from execute() -- then we'd be able to pass the proper VZ> buffer length to SQLBindParameter(). The message from Mateusz above hints VZ> that there are problems with doing it but doesn't say clearly what they are VZ> so I'm a bit lost here. I still don't know why this can't be done. Calling define_and_bind() before pre_use(), or at least before convert_to_base() called from pre_use(), looks like a big design problem to me. I looked at the code of the other backends and they either don't do anything in bind_by_{pos,name} methods at all (Firebird, PostgreSQL, MySQL) or use horrible hacks to make it work (Oracle always allocates a 32769 byte buffer for all strings -- if this doesn't indicate a problem with the SOCI design, I don't know what does). But I could well be missing something so I'm not going to change this, even if looks strange to me. Instead I modified ODBC backend to do the same thing as e.g. PostgreSQL and really bind the parameters in pre_use() itself instead of doing it in the dedicated bind_by_xxx() methods. This seems to work well and fixes both the buffer overflow problem (without allocating fixed size buffers!) and the problem with custom types as pre_use() is called after their values had been converted. The patch is attached and I'll also push it to my Github SOCI repository[1] soon. If anybody sees any problems with this patch, please let me know. Thanks! VZ [1] https://github.com/vadz/soci-experiments
>From 0b587997f349f625507f552c3583c5f73dc77ddd Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin <[email protected]> Date: Thu, 24 May 2012 17:05:51 +0200 Subject: [PATCH] Fix binding of parameters of custom types in ODBC backend. Call SQLBindParameter() from pre_use() which is guaranteed to be called when the input parameters have their correct, final values, notably after they were converted from custom type by conversion_use_type::convert_to_base(). This also fixes another serious problem for string types in the ODBC backend: setting a "use" parameter after define_and_bind() but before execute() doesn't result in buffer overflow any more. Signed-off-by: Vadim Zeitlin <[email protected]> --- src/backends/odbc/soci-odbc.h | 16 ++-- src/backends/odbc/standard-use-type.cpp | 131 +++++++++++++------------------ 2 files changed, 66 insertions(+), 81 deletions(-) diff --git a/src/backends/odbc/soci-odbc.h b/src/backends/odbc/soci-odbc.h index 9c5ecbc..dc3b642 100644 --- a/src/backends/odbc/soci-odbc.h +++ b/src/backends/odbc/soci-odbc.h @@ -99,12 +99,7 @@ struct odbc_vector_into_type_backend : details::vector_into_type_backend struct odbc_standard_use_type_backend : details::standard_use_type_backend { odbc_standard_use_type_backend(odbc_statement_backend &st) - : statement_(st), data_(0), buf_(0), indHolder_(0) {} - - void prepare_for_bind(void *&data, SQLLEN &size, - SQLSMALLINT &sqlType, SQLSMALLINT &cType); - void bind_helper(int &position, - void *data, details::exchange_type type); + : statement_(st), position_(-1), data_(0), buf_(0), indHolder_(0) {} virtual void bind_by_pos(int &position, void *data, details::exchange_type type, bool readOnly); @@ -116,7 +111,16 @@ struct odbc_standard_use_type_backend : details::standard_use_type_backend virtual void clean_up(); + // Return the pointer to the buffer containing data to be used by ODBC. + // This can be either data_ itself or buf_, that is allocated by this + // function if necessary. + // + // Also fill in the size of the data and SQL and C types of it. + void* prepare_for_bind(SQLLEN &size, + SQLSMALLINT &sqlType, SQLSMALLINT &cType); + odbc_statement_backend &statement_; + int position_; void *data_; details::exchange_type type_; char *buf_; diff --git a/src/backends/odbc/standard-use-type.cpp b/src/backends/odbc/standard-use-type.cpp index 8c572ce..2006a3a 100644 --- a/src/backends/odbc/standard-use-type.cpp +++ b/src/backends/odbc/standard-use-type.cpp @@ -14,8 +14,8 @@ using namespace soci; using namespace soci::details; -void odbc_standard_use_type_backend::prepare_for_bind( - void *&data, SQLLEN &size, SQLSMALLINT &sqlType, SQLSMALLINT &cType) +void* odbc_standard_use_type_backend::prepare_for_bind( + SQLLEN &size, SQLSMALLINT &sqlType, SQLSMALLINT &cType) { switch (type_) { @@ -41,20 +41,18 @@ void odbc_standard_use_type_backend::prepare_for_bind( size = sizeof(double); break; - // cases that require adjustments and buffer management case x_char: sqlType = SQL_CHAR; cType = SQL_C_CHAR; - size = sizeof(char) + 1; + size = 2; buf_ = new char[size]; - data = buf_; + buf_[0] = *static_cast<char*>(data_); + buf_[1] = '\0'; indHolder_ = SQL_NTS; break; case x_stdstring: { - // TODO: No textual value is assigned here! - - std::string* s = static_cast<std::string*>(data); + std::string* s = static_cast<std::string*>(data_); #ifdef SOCI_ODBC_VERSION_3_IS_TO_BE_CHECKED sqlType = SQL_VARCHAR; #else @@ -63,21 +61,35 @@ void odbc_standard_use_type_backend::prepare_for_bind( sqlType = SQL_LONGVARCHAR; #endif cType = SQL_C_CHAR; - size = s->size() + 1; - buf_ = new char[size]; - data = buf_; + size = s->size(); + buf_ = new char[size+1]; + memcpy(buf_, s->c_str(), size); + buf_[size++] = '\0'; indHolder_ = SQL_NTS; } break; case x_stdtm: + { + std::tm *t = static_cast<std::tm *>(data_); + sqlType = SQL_TIMESTAMP; cType = SQL_C_TIMESTAMP; buf_ = new char[sizeof(TIMESTAMP_STRUCT)]; - data = buf_; size = 19; // This number is not the size in bytes, but the number // of characters in the date if it was written out // yyyy-mm-dd hh:mm:ss - break; + + TIMESTAMP_STRUCT * ts = reinterpret_cast<TIMESTAMP_STRUCT*>(buf_); + + ts->year = static_cast<SQLSMALLINT>(t->tm_year + 1900); + ts->month = static_cast<SQLUSMALLINT>(t->tm_mon + 1); + ts->day = static_cast<SQLUSMALLINT>(t->tm_mday); + ts->hour = static_cast<SQLUSMALLINT>(t->tm_hour); + ts->minute = static_cast<SQLUSMALLINT>(t->tm_min); + ts->second = static_cast<SQLUSMALLINT>(t->tm_sec); + ts->fraction = 0; + } + break; case x_blob: { @@ -96,33 +108,15 @@ void odbc_standard_use_type_backend::prepare_for_bind( break; case x_statement: case x_rowid: - break; - case x_long_long: break; // TODO: verify if can be supported - case x_unsigned_long_long: break; // TODO: verify if can be supported - } -} - -void odbc_standard_use_type_backend::bind_helper(int &position, void *data, exchange_type type) -{ - data_ = data; // for future reference - type_ = type; // for future reference - - SQLSMALLINT sqlType; - SQLSMALLINT cType; - SQLLEN size; - - prepare_for_bind(data, size, sqlType, cType); - - SQLRETURN rc = SQLBindParameter(statement_.hstmt_, - static_cast<SQLUSMALLINT>(position++), - SQL_PARAM_INPUT, - cType, sqlType, size, 0, data, 0, &indHolder_); - - if (is_odbc_error(rc)) - { - throw odbc_soci_error(SQL_HANDLE_STMT, statement_.hstmt_, - "Binding"); + case x_long_long: + case x_unsigned_long_long: + // Unsupported data types. + return NULL; } + + // Return either the pointer to C++ data itself or the buffer that we + // allocated, if any. + return buf_ ? buf_ : data_; } void odbc_standard_use_type_backend::bind_by_pos( @@ -134,7 +128,9 @@ void odbc_standard_use_type_backend::bind_by_pos( "Binding for use elements must be either by position or by name."); } - bind_helper(position, data, type); + position_ = position++; + data_ = data; + type_ = type; statement_.boundByPos_ = true; } @@ -162,54 +158,39 @@ void odbc_standard_use_type_backend::bind_by_name( count++; } - if (position != -1) - { - bind_helper(position, data, type); - } - else + if (position == -1) { std::ostringstream ss; ss << "Unable to find name '" << name << "' to bind to"; throw soci_error(ss.str().c_str()); } + position_ = position; + data_ = data; + type_ = type; + statement_.boundByName_ = true; } void odbc_standard_use_type_backend::pre_use(indicator const *ind) { // first deal with data - if (type_ == x_char) - { - char *c = static_cast<char*>(data_); - buf_[0] = *c; - buf_[1] = '\0'; - } - else if (type_ == x_stdstring) - { - std::string *s = static_cast<std::string *>(data_); - std::size_t const bufSize = s->size() + 1; - // TODO: this is a hack (for buffer re-size? --mloskot) - //delete [] buf_; - //buf_ = new char[bufSize]; - - std::size_t const sSize = s->size(); - std::size_t const toCopy = sSize < bufSize -1 ? sSize + 1 : bufSize - 1; - strncpy(buf_, s->c_str(), toCopy); - buf_[toCopy] = '\0'; - } - else if (type_ == x_stdtm) - { - std::tm *t = static_cast<std::tm *>(data_); - TIMESTAMP_STRUCT * ts = reinterpret_cast<TIMESTAMP_STRUCT*>(buf_); + SQLSMALLINT sqlType; + SQLSMALLINT cType; + SQLLEN size; - ts->year = static_cast<SQLSMALLINT>(t->tm_year + 1900); - ts->month = static_cast<SQLUSMALLINT>(t->tm_mon + 1); - ts->day = static_cast<SQLUSMALLINT>(t->tm_mday); - ts->hour = static_cast<SQLUSMALLINT>(t->tm_hour); - ts->minute = static_cast<SQLUSMALLINT>(t->tm_min); - ts->second = static_cast<SQLUSMALLINT>(t->tm_sec); - ts->fraction = 0; + void* const sqlData = prepare_for_bind(size, sqlType, cType); + + SQLRETURN rc = SQLBindParameter(statement_.hstmt_, + static_cast<SQLUSMALLINT>(position_), + SQL_PARAM_INPUT, + cType, sqlType, size, 0, + sqlData, 0, &indHolder_); + + if (is_odbc_error(rc)) + { + throw odbc_soci_error(SQL_HANDLE_STMT, statement_.hstmt_, + "Binding"); } // then handle indicators -- 1.7.10
pgpfmEvQ1e1gD.pgp
Description: PGP signature
------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________ Soci-users mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/soci-users
