duckdb / duckdb-wasm

WebAssembly version of DuckDB
https://shell.duckdb.org
MIT License
1.02k stars 110 forks source link

Apply additional checks on dynamic_casts to references #1697

Closed carlopi closed 2 months ago

carlopi commented 2 months ago

There has been a bug report where a duckdb-wasm user manage to relyably have a std::bad_cast thrown by duckdb-wasm.

This is likely due to a combinations of problems: 1) duckdb using dynamic_cast<Derived&> that might lead to a throw std::bad_cast if the RTTI do not match 2) some yet to be identified problem that leads to a mismatch (possibly uninitialised memory or some other logical error) 3) duckdb-wasm not addressing throws properly in all possible callsites (work in progres)

This PR adds an experimental checked_dynamic_cast that do check whether a cast will throw, and in that case log this to console AND throw a FatalError.

This will hopefully make failures due to 2 more visible and have them fail better and more clearly.

Added patch is:

diff --git a/src/include/duckdb/common/exception.hpp b/src/include/duckdb/common/exception.hpp
index 2e45cb92af..46967277a7 100644
--- a/src/include/duckdb/common/exception.hpp
+++ b/src/include/duckdb/common/exception.hpp
@@ -16,6 +16,7 @@

 #include <vector>
 #include <stdexcept>
+#include <iostream>

 namespace duckdb {
 enum class PhysicalType : uint8_t;
@@ -363,4 +364,13 @@ public:
        DUCKDB_API explicit ParameterNotResolvedException();
 };

+       template <typename A, typename B>
+       A&& checked_dynamic_cast (B&& target) {
+               if (dynamic_cast<typename std::remove_reference<A>::type*>(&target)) {
+                       return dynamic_cast<A&>(target);
+               }
+               std::cout <<"\n"<< "checked_dynamic_cast between " << typeid(A).name() << "\tand " << typeid(B).name() << " ERRORRED\n";
+               throw FatalException("Failed checked_dynamic_cast");
+       }
+
 } // namespace duckdb
 diff --git a/src/planner/operator/logical_update.cpp b/src/planner/operator/logical_update.cpp
index e66dd36d1a..7fe2e907e1 100644
--- a/src/planner/operator/logical_update.cpp
+++ b/src/planner/operator/logical_update.cpp
@@ -12,7 +12,7 @@ LogicalUpdate::LogicalUpdate(TableCatalogEntry &table)
 LogicalUpdate::LogicalUpdate(ClientContext &context, const unique_ptr<CreateInfo> &table_info)
     : LogicalOperator(LogicalOperatorType::LOGICAL_UPDATE),
       table(Catalog::GetEntry<TableCatalogEntry>(context, table_info->catalog, table_info->schema,
-                                                 dynamic_cast<CreateTableInfo &>(*table_info).table)) {
+                                                 checked_dynamic_cast<CreateTableInfo &>(*table_info).table)) {
 }

 idx_t LogicalUpdate::EstimateCardinality(ClientContext &context) {

(+ other few places where dynamic_cast on a reference has been changed to checked_dynamic_cast) that is a bit rough (std::cout ...) but should work for now to track this down. This should likely be considered in a similar form also duckdb-side.

Mytherin commented 2 months ago

Thanks!