From 90f116f6ba3562201e4bd272edf37c882cabd548 Mon Sep 17 00:00:00 2001 From: Golam Samdani Sayam Date: Fri, 23 Oct 2026 11:10:51 +0600 Subject: [PATCH] [ntuple] Fail cleanly if a field value cannot be constructed RClassField::ConstructValue() ignored TClass::New() failures, so MakeField() or MakeField() for a class without a default constructor returned a pointer to unconstructed memory. For interpreted classes, TClass::New() also JITs a constructor wrapper; the failed compilation is what triggered the Cling state corruption from #13910. Check kIsAbstract and HasDefaultConstructor() before calling TClass::New(), throw an RException if construction fails, and free the allocated buffer. Also make REntry::AddValue() construct the value before recording the field name/type so a failed MakeField() leaves the entry unchanged. Add a roottest regression covering abstract classes, classes without a default constructor, and continued model/interpreter use afterwards. --- roottest/root/ntuple/CMakeLists.txt | 6 ++ roottest/root/ntuple/makefield_invalid.C | 105 +++++++++++++++++++++++ tree/ntuple/inc/ROOT/REntry.hxx | 4 +- tree/ntuple/src/RFieldBase.cxx | 10 ++- tree/ntuple/src/RFieldMeta.cxx | 24 +++++- 5 files changed, 144 insertions(+), 5 deletions(-) create mode 100644 roottest/root/ntuple/makefield_invalid.C diff --git a/roottest/root/ntuple/CMakeLists.txt b/roottest/root/ntuple/CMakeLists.txt index 7eb85bc3194fd..fc9fb3e558b3e 100644 --- a/roottest/root/ntuple/CMakeLists.txt +++ b/roottest/root/ntuple/CMakeLists.txt @@ -2,4 +2,10 @@ ROOTTEST_ADD_TEST(test_Basics MACRO basics.C OUTREF test_basics.ref) +# issue 13910: MakeField() must fail cleanly for invalid types and keep the +# model/interpreter usable. +ROOTTEST_ADD_TEST(test_MakeFieldInvalid + MACRO makefield_invalid.C + PASSREGEX "all MakeField failure checks passed") + ROOTTEST_ADD_TESTDIRS() diff --git a/roottest/root/ntuple/makefield_invalid.C b/roottest/root/ntuple/makefield_invalid.C new file mode 100644 index 0000000000000..f07a2b4191a41 --- /dev/null +++ b/roottest/root/ntuple/makefield_invalid.C @@ -0,0 +1,105 @@ +#include + +#include + +#include +#include +#include +#include + +// Regression test for https://github.com/root-project/root/issues/13910: +// MakeField() must report a diagnosable error for types that RNTuple cannot +// construct, and it must leave the model and the interpreter usable. + +class RNTupleTestAbstract { +public: + virtual ~RNTupleTestAbstract() = default; + virtual void Virt() = 0; +}; + +class RNTupleTestDerived : public RNTupleTestAbstract { +public: + int fValue = 42; + void Virt() final {} +}; + +class RNTupleTestNoDefaultCtor { +public: + explicit RNTupleTestNoDefaultCtor(int value) : fValue(value) {} + int fValue = 0; +}; + +namespace { + +int gFailures = 0; + +template +void ExpectFieldFailure(ROOT::RNTupleModel &model, const char *label, const char *fieldName, const char *expected) +{ + try { + // Keep the returned value alive until after the failure is reported: it + // may point to unconstructed memory, so letting it go out of scope would + // crash instead of reporting a clean test failure. + auto value = model.MakeField(fieldName); + (void)value; + std::cerr << "FAIL: MakeField<" << label << "> did not throw" << std::endl; + gSystem->Exit(1); + } catch (const std::exception &ex) { + const std::string msg = ex.what(); + std::cout << "MakeField<" << label << "> threw: " << msg << std::endl; + if (msg.find(expected) == std::string::npos) { + std::cerr << "FAIL: expected the error message to contain \"" << expected << "\"" << std::endl; + ++gFailures; + } + } catch (...) { + std::cerr << "FAIL: MakeField<" << label << "> threw a non-standard exception" << std::endl; + ++gFailures; + } +} + +} // namespace + +void makefield_invalid() +{ + { + auto model = ROOT::RNTupleModel::Create(); + ExpectFieldFailure(*model, "Abstract", "abstract", "abstract"); + if (model->GetDefaultEntry().HasField("abstract")) { + std::cerr << "FAIL: failed field \"abstract\" was added to the default entry" << std::endl; + ++gFailures; + } + + // A failed MakeField() must not leave a partial field in the default + // entry: further fields on the same model have to keep working. + try { + model->MakeField>("vec"); + std::cout << "OK: model usable after abstract failure" << std::endl; + } catch (const std::exception &ex) { + std::cerr << "FAIL: model unusable after abstract failure: " << ex.what() << std::endl; + ++gFailures; + } + } + + { + auto model = ROOT::RNTupleModel::Create(); + ExpectFieldFailure(*model, "NoDefaultCtor", "nodefault", "default constructor"); + if (model->GetDefaultEntry().HasField("nodefault")) { + std::cerr << "FAIL: failed field \"nodefault\" was added to the default entry" << std::endl; + ++gFailures; + } + + try { + model->MakeField("derived"); + std::cout << "OK: model usable after no-default-constructor failure" << std::endl; + } catch (const std::exception &ex) { + std::cerr << "FAIL: model unusable after no-default-constructor failure: " << ex.what() << std::endl; + ++gFailures; + } + } + + if (gFailures != 0) { + std::cerr << gFailures << " check(s) failed" << std::endl; + gSystem->Exit(1); + } + std::cout << "all MakeField failure checks passed" << std::endl; +} diff --git a/tree/ntuple/inc/ROOT/REntry.hxx b/tree/ntuple/inc/ROOT/REntry.hxx index 590e39853eff2..81eba06ba5ccd 100644 --- a/tree/ntuple/inc/ROOT/REntry.hxx +++ b/tree/ntuple/inc/ROOT/REntry.hxx @@ -85,9 +85,11 @@ private: template std::shared_ptr AddValue(ROOT::RField &field) { + // Create the value first: if construction fails, the entry must not be + // left with a field name/type that has no corresponding value. + auto value = field.CreateValue(); fFieldName2Token[field.GetQualifiedFieldName()] = fValues.size(); fFieldTypes.push_back(field.GetTypeName()); - auto value = field.CreateValue(); fValues.emplace_back(value); // We know that the created RValue has the right type, skip the unnecessary check. return std::static_pointer_cast(value.template GetPtr()); diff --git a/tree/ntuple/src/RFieldBase.cxx b/tree/ntuple/src/RFieldBase.cxx index 1de0bb530051f..65d40aed5251a 100644 --- a/tree/ntuple/src/RFieldBase.cxx +++ b/tree/ntuple/src/RFieldBase.cxx @@ -658,7 +658,15 @@ void *ROOT::RFieldBase::CreateObjectRawPtr() const where = operator new(GetValueSize(), std::align_val_t(GetAlignment())); } R__ASSERT(where != nullptr); - ConstructValue(where); + try { + ConstructValue(where); + } catch (...) { + if (align <= sizeof(std::max_align_t)) + operator delete(where); + else + operator delete(where, std::align_val_t(align)); + throw; + } return where; } diff --git a/tree/ntuple/src/RFieldMeta.cxx b/tree/ntuple/src/RFieldMeta.cxx index 8358f1274f1be..5017781211808 100644 --- a/tree/ntuple/src/RFieldMeta.cxx +++ b/tree/ntuple/src/RFieldMeta.cxx @@ -109,6 +109,24 @@ void EnsureValidUserClass(TClass *cl, const ROOT::RFieldBase &field, std::string } } +/// Construct a class value without going through TClass::New() if the class +/// cannot be instantiated. TClass::New() may JIT a constructor wrapper, and a +/// failed compilation can leave the interpreter in an inconsistent state. +void ConstructClassValue(TClass *cl, const ROOT::RFieldBase &field, void *where) +{ + const std::string typeName = field.GetTypeName(); + if (cl->Property() & kIsAbstract) { + throw ROOT::RException(R__FAIL("cannot construct an object of type " + typeName + ": the class is abstract")); + } + if (!cl->HasDefaultConstructor(kTRUE)) { + throw ROOT::RException( + R__FAIL("cannot construct an object of type " + typeName + ": the class has no default constructor")); + } + if (!cl->New(where)) { + throw ROOT::RException(R__FAIL("cannot construct an object of type " + typeName)); + } +} + TEnum *EnsureValidEnum(std::string_view enumName) { auto e = TEnum::GetEnum(std::string(enumName).c_str()); @@ -627,7 +645,7 @@ void ROOT::RClassField::ReconcileOnDiskField(const RNTupleDescriptor &desc) void ROOT::RClassField::ConstructValue(void *where) const { - fClass->New(where); + ConstructClassValue(fClass, *this, where); } ROOT::RClassField::RClassDeleter::RClassDeleter(TClass *cl) : RDeleter(cl->GetClassAlignment()), fClass(cl) {} @@ -1070,7 +1088,7 @@ void ROOT::RSoAField::ReconcileOnDiskField(const RNTupleDescriptor &desc) void ROOT::RSoAField::ConstructValue(void *where) const { - fClass->New(where); + ConstructClassValue(fClass, *this, where); } ROOT::RSoAField::RSoADeleter::RSoADeleter(TClass *cl) : RDeleter(cl->GetClassAlignment()), fSoAClass(cl) {} @@ -1714,7 +1732,7 @@ void ROOT::RStreamerField::ReconcileOnDiskField(const RNTupleDescriptor &desc) void ROOT::RStreamerField::ConstructValue(void *where) const { - fClass->New(where); + ConstructClassValue(fClass, *this, where); } ROOT::RStreamerField::RStreamerFieldDeleter::RStreamerFieldDeleter(TClass *cl)