Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Apply additional checks on dynamic_casts to references #1697

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

carlopi
Copy link
Collaborator

@carlopi carlopi commented Apr 11, 2024

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 Mytherin merged commit 26245e6 into duckdb:main Apr 11, 2024
15 checks passed
@Mytherin
Copy link
Contributor

Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants