From b3fac52b10a588b08a257035fbb7b55c017ac305 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 9 Nov 2024 22:54:37 +0100 Subject: [PATCH 1/3] Avoid accessing uninitialized type variable in Oracle classes oracle_standard_into_type_backend::type_ may not be initialized when clean_up() is called, and comparing its value with x_xmltype and other constants was triggering undefined behaviour in this case, so avoid doing it by checking if ociData_ is non-null before doing it. Also modify oracle_standard_use_type_backend in a similar way, even if it doesn't suffer from this problem, for symmetry. --- include/soci/oracle/soci-oracle.h | 4 ++-- src/backends/oracle/standard-into-type.cpp | 22 ++++++++++++++------- src/backends/oracle/standard-use-type.cpp | 23 +++++++++++++++------- 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/include/soci/oracle/soci-oracle.h b/include/soci/oracle/soci-oracle.h index 43cff3ec4..705751bd1 100644 --- a/include/soci/oracle/soci-oracle.h +++ b/include/soci/oracle/soci-oracle.h @@ -63,7 +63,7 @@ struct oracle_standard_into_type_backend : details::standard_into_type_backend OCIDefine *defnp_; sb2 indOCIHolder_; void *data_; - void *ociData_; + void *ociData_ = NULL; char *buf_; // generic buffer details::exchange_type type_; @@ -143,7 +143,7 @@ struct oracle_standard_use_type_backend : details::standard_use_type_backend OCIBind *bindp_; sb2 indOCIHolder_; void *data_; - void *ociData_; + void *ociData_ = NULL; bool readOnly_; char *buf_; // generic buffer details::exchange_type type_; diff --git a/src/backends/oracle/standard-into-type.cpp b/src/backends/oracle/standard-into-type.cpp index 89eb05936..5c0e78f19 100644 --- a/src/backends/oracle/standard-into-type.cpp +++ b/src/backends/oracle/standard-into-type.cpp @@ -391,15 +391,23 @@ void oracle_standard_into_type_backend::post_fetch( void oracle_standard_into_type_backend::clean_up() { - if (type_ == x_xmltype || type_ == x_longstring) + if (ociData_) { - free_temp_lob(statement_.session_, static_cast(ociData_)); - ociData_ = NULL; - } + switch (type_) + { + case x_xmltype: + case x_longstring: + free_temp_lob(statement_.session_, static_cast(ociData_)); + break; + + case x_blob: + OCIDescriptorFree(ociData_, OCI_DTYPE_LOB); + break; + + default: + throw soci_error("Internal error: OCI data used for unexpected type"); + } - if (type_ == x_blob) - { - OCIDescriptorFree(ociData_, OCI_DTYPE_LOB); ociData_ = NULL; } diff --git a/src/backends/oracle/standard-use-type.cpp b/src/backends/oracle/standard-use-type.cpp index c9adf4422..8a2b64b04 100644 --- a/src/backends/oracle/standard-use-type.cpp +++ b/src/backends/oracle/standard-use-type.cpp @@ -710,14 +710,23 @@ void oracle_standard_use_type_backend::post_use(bool gotData, indicator *ind) void oracle_standard_use_type_backend::clean_up() { - if (type_ == x_xmltype || type_ == x_longstring) - { - free_temp_lob(statement_.session_, static_cast(ociData_)); - ociData_ = NULL; - } - - if (type_ == x_blob) + if (ociData_) { + switch (type_) + { + case x_xmltype: + case x_longstring: + free_temp_lob(statement_.session_, static_cast(ociData_)); + break; + + case x_blob: + // We don't own the LOB locator, oracle_blob_backend does. + break; + + default: + throw soci_error("Internal error: OCI data used for unexpected type"); + } + ociData_ = NULL; } From 7ad6c66486e59c5b53fe4e43529f254fd6a4651d Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 9 Nov 2024 23:34:08 +0100 Subject: [PATCH 2/3] Don't pass null pointers to memcpy() in Firebird blob code This is similar to 96668e9a (Don't pass null pointers to memcpy() in trivial_blob_backend code, 2024-10-16) and done for the same reason: when there is no data to read/write, the buffer pointer may be null and passing it to memcpy() triggers UBSAN error, so avoid doing it. --- src/backends/firebird/blob.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/backends/firebird/blob.cpp b/src/backends/firebird/blob.cpp index b4e401b7b..e0e5f8456 100644 --- a/src/backends/firebird/blob.cpp +++ b/src/backends/firebird/blob.cpp @@ -59,7 +59,8 @@ std::size_t firebird_blob_backend::read_from_start(void * buf, std::size_t toRea // Ensure we don't read more than we have toRead = std::min(toRead, size - offset); - std::memcpy(buf, &data_[offset], toRead); + if (toRead) + std::memcpy(buf, &data_[offset], toRead); return toRead; } @@ -121,7 +122,8 @@ void firebird_blob_backend::trim(std::size_t newLen) void firebird_blob_backend::writeBuffer(std::size_t offset, const void * buf, std::size_t toWrite) { - std::memcpy(data_.data() + offset, buf, toWrite); + if (toWrite) + std::memcpy(data_.data() + offset, buf, toWrite); } void firebird_blob_backend::open() From 115663a2ff352190f9f7ece5c7464a901f008980 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 9 Nov 2024 22:57:40 +0100 Subject: [PATCH 3/3] Ensure that we stop on error when using UBSAN Otherwise the errors reported by it could remain unnoticed, as happened with the UB detected in Oracle backend. --- .github/workflows/ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 90aec85d6..f7f8708d1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -152,6 +152,8 @@ jobs: fi if [ "${{matrix.no_ubsan}}" = true ]; then set_env_var SOCI_NO_UBSAN 1 + else + set_env_var UBSAN_OPTIONS print_stacktrace=1:halt_on_error=1 fi - name: Setup tmate