ginkgo-project / ginkgo

Numerical linear algebra software package
https://ginkgo-project.github.io/
BSD 3-Clause "New" or "Revised" License
415 stars 90 forks source link

Undefined sanitizer triggered about downcasting #1655

Open tpadioleau opened 4 months ago

tpadioleau commented 4 months ago

Hello, the following C++ snippet triggers the undefined sanitizer:

#include <ginkgo/ginkgo.hpp>

int main(int argc, char **argv) {
  std::shared_ptr const gko_executor = gko::ReferenceExecutor::create();

  // Create the solver factory
  std::shared_ptr const residual_criterion =
      gko::stop::ResidualNorm<double>::build().on(gko_executor);

  std::unique_ptr const solver_factory = gko::solver::Bicgstab<double>::build()
                                             .with_criteria(residual_criterion)
                                             .on(gko_executor);

  int const mat_size = 10;

  std::shared_ptr const matrix_sparse = gko::matrix::Csr<double, int>::create(
      gko_executor, gko::dim<2>(mat_size, mat_size));

  std::unique_ptr const solver = solver_factory->generate(matrix_sparse);

  return 0;
}

The sanitizer reports

/local/home/tpadiole/spack-0.22.1/opt/spack/linux-ubuntu20.04-icelake/gcc-10.5.0/ginkgo-1.8.0-shuyfkrpn22wdno3jhruqz4lk5qz43ug/include/ginkgo/core/solver/solver_base.hpp:572:38: runtime error: member call on address 0x5591484b8848 which does not point to an object of type 'EnableSolverBase'
0x5591484b8848: note: object has invalid vptr
 91 55 00 00  00 00 00 00 00 00 00 00  40 8a 4b 48 91 55 00 00  70 8a 4b 48 91 55 00 00  e0 89 4b 48
              ^~~~~~~~~~~~~~~~~~~~~~~
              invalid vptr
/local/home/tpadiole/spack-0.22.1/opt/spack/linux-ubuntu20.04-icelake/gcc-10.5.0/ginkgo-1.8.0-shuyfkrpn22wdno3jhruqz4lk5qz43ug/include/ginkgo/core/solver/solver_base.hpp:650:34: runtime error: downcast of address 0x5591484b8800 which does not point to an object of type 'Bicgstab'
0x5591484b8800: note: object is of type 'gko::EnableLinOp<gko::solver::Bicgstab<double>, gko::LinOp>'
 00 00 00 00  a8 9e 22 48 91 55 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'gko::EnableLinOp<gko::solver::Bicgstab<double>, gko::LinOp>'
/local/home/tpadiole/spack-0.22.1/opt/spack/linux-ubuntu20.04-icelake/gcc-10.5.0/ginkgo-1.8.0-shuyfkrpn22wdno3jhruqz4lk5qz43ug/include/ginkgo/core/solver/solver_base.hpp:765:34: runtime error: downcast of address 0x5591484b8800 which does not point to an object of type 'Bicgstab'
0x5591484b8800: note: object is of type 'gko::EnableLinOp<gko::solver::Bicgstab<double>, gko::LinOp>'
 00 00 00 00  a8 9e 22 48 91 55 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'gko::EnableLinOp<gko::solver::Bicgstab<double>, gko::LinOp>'
/local/home/tpadiole/spack-0.22.1/opt/spack/linux-ubuntu20.04-icelake/gcc-10.5.0/ginkgo-1.8.0-shuyfkrpn22wdno3jhruqz4lk5qz43ug/include/ginkgo/core/solver/solver_base.hpp:343:34: runtime error: downcast of address 0x5591484b8800 which does not point to an object of type 'Bicgstab'
0x5591484b8800: note: object is of type 'gko::EnableLinOp<gko::solver::Bicgstab<double>, gko::LinOp>'
 00 00 00 00  a8 9e 22 48 91 55 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'gko::EnableLinOp<gko::solver::Bicgstab<double>, gko::LinOp>'

It was compiled with the following CMakeLists.txt

cmake_minimum_required(VERSION 3.22)

project(bug_report LANGUAGES CXX)

find_package(Ginkgo 1.8.0 EXACT REQUIRED)

add_executable(main main.cpp)
target_link_libraries(main PUBLIC Ginkgo::ginkgo)
target_compile_features(main PUBLIC cxx_std_17)

using the cmake command cmake -B build -DCMAKE_CXX_COMPILER=g++-10 -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS="-fsanitize=undefined -fno-omit-frame-pointer"

Thanks, Thomas

upsj commented 4 months ago

I can reproduce this behavior, but I am willing to say that this is not UB - this is a static_cast from a derived class to a base class for the CRTP pattern, which is fine as long as the object has started construction (which it has, since its base classes are already partially constructed, namely the PolymorphicObject base class which provides the get_executor() function that is being called via self()->get_executor()). More precisely, this is a downcast to Bicgstab followed by an upcast to PolymorphicObject, we are not accessing any members of Bicgstab, instead only members of the already constructed PolymorphicObject. Likely, this would not give any warnings if EnableSolverBase wasn't polymorphic, UBSAN only considers it problematic because we are using both dynamic and static polymorphism in the same class.

A more in-depth discussion of the same question: https://stackoverflow.com/questions/73172193/can-you-static-cast-this-to-a-derived-class-in-a-base-class-constructor-then-u

tpadioleau commented 4 months ago

Thanks for taking the time to look at it. I will follow the discussion on google/sanitizers.

I agree that it may not be a UB but isn't it fragile ? In your example, EnableB assumes that EnableA, as part of Derived, is already initialized right ? If someone inverts them by mistake, then it becomes UB ?

upsj commented 4 months ago

It definitely depends on the order of base classes, but in our case, it would fail very loudly because it will lead to an uninitialized shared_ptr for the executor, so I feel somewhat okay with it. I don't think there is a way to enforce this statically.