Preserve problem arrays and ownership after allocation failure

CHANGELOG

@@ -6,6 +6,8 @@
 Fixed bugs
 ----------
 
+- preserve problem arrays and ownership when allocation fails during variable or constraint insertion, avoiding crashes in cleanup
+
 - corrected memory allocation condition for storing column swaps in symmetry_orbitopal
 
 

src/scip/prob.c

@@ -80,10 +80,14 @@ SCIP_RETCODE probEnsureVarsMem(
 
    if( num > prob->varssize )
    {
+      SCIP_VAR** resized;
       int newsize;
 
       newsize = SCIPsetCalcMemGrowSize(set, num);
-      SCIP_ALLOC( BMSreallocMemoryArray(&prob->vars, newsize) );
+      resized = prob->vars;
+      /* Preserve the owned array if allocation fails: cleanup still needs it. */
+      SCIP_ALLOC( BMSreallocMemoryArray(&resized, newsize) );
+      prob->vars = resized;
       prob->varssize = newsize;
    }
    assert(num <= prob->varssize);
@@ -104,10 +108,14 @@ SCIP_RETCODE probEnsureFixedvarsMem(
 
    if( num > prob->fixedvarssize )
    {
+      SCIP_VAR** resized;
       int newsize;
 
       newsize = SCIPsetCalcMemGrowSize(set, num);
-      SCIP_ALLOC( BMSreallocMemoryArray(&prob->fixedvars, newsize) );
+      resized = prob->fixedvars;
+      /* Preserve the owned array if allocation fails: cleanup still needs it. */
+      SCIP_ALLOC( BMSreallocMemoryArray(&resized, newsize) );
+      prob->fixedvars = resized;
       prob->fixedvarssize = newsize;
    }
    assert(num <= prob->fixedvarssize);
@@ -128,10 +136,14 @@ SCIP_RETCODE probEnsureDeletedvarsMem(
 
    if( num > prob->deletedvarssize )
    {
+      SCIP_VAR** resized;
       int newsize;
 
       newsize = SCIPsetCalcMemGrowSize(set, num);
-      SCIP_ALLOC( BMSreallocMemoryArray(&prob->deletedvars, newsize) );
+      resized = prob->deletedvars;
+      /* Preserve the owned array if allocation fails: cleanup still needs it. */
+      SCIP_ALLOC( BMSreallocMemoryArray(&resized, newsize) );
+      prob->deletedvars = resized;
       prob->deletedvarssize = newsize;
    }
    assert(num <= prob->deletedvarssize);
@@ -152,14 +164,21 @@ SCIP_RETCODE probEnsureConssMem(
 
    if( num > prob->consssize )
    {
+      SCIP_CONS** resized;
       int newsize;
 
       newsize = SCIPsetCalcMemGrowSize(set, num);
-      SCIP_ALLOC( BMSreallocMemoryArray(&prob->conss, newsize) );
+      resized = prob->conss;
+      /* Preserve the owned array if allocation fails: cleanup still needs it. */
+      SCIP_ALLOC( BMSreallocMemoryArray(&resized, newsize) );
+      prob->conss = resized;
       /* resize sorted original constraints if they exist */
       if( prob->origcheckconss != NULL )
       {
-         SCIP_ALLOC( BMSreallocMemoryArray(&prob->origcheckconss, newsize) );
+         SCIP_CONS** resized = prob->origcheckconss;
+         /* Preserve the owned array if allocation fails: cleanup still needs it. */
+         SCIP_ALLOC( BMSreallocMemoryArray(&resized, newsize) );
+         prob->origcheckconss = resized;
       }
       prob->consssize = newsize;
    }
@@ -1123,12 +1142,12 @@ SCIP_RETCODE SCIPprobAddVar(
    }
 #endif
 
+   /* Allocate before acquiring ownership: a failed resize must not leak a capture. */
+   SCIP_CALL( probEnsureVarsMem(prob, set, prob->nvars+1) );
+
    /* capture variable */
    SCIPvarCapture(var);
 
-   /* allocate additional memory */
-   SCIP_CALL( probEnsureVarsMem(prob, set, prob->nvars+1) );
-
    /* insert variable in vars array and mark it to be in problem */
    probInsertVar(prob, var);
 
@@ -1524,12 +1543,14 @@ SCIP_RETCODE SCIPprobAddCons(
    SCIPsetDebugMsg(set, "adding constraint <%s> to global problem -> %d constraints\n",
       SCIPconsGetName(cons), prob->nconss+1);
 
+   /* A failed resize must leave the constraint outside the problem. */
+   SCIP_CALL( probEnsureConssMem(prob, set, prob->nconss+1) );
+
    /* mark the constraint as problem constraint, and remember the constraint's position */
    cons->addconssetchg = NULL;
    cons->addarraypos = prob->nconss;
 
    /* add the constraint to the problem's constraint array */
-   SCIP_CALL( probEnsureConssMem(prob, set, prob->nconss+1) );
    prob->conss[prob->nconss] = cons;
    if( prob->origcheckconss != NULL )
       prob->origcheckconss[prob->nconss] = cons;

tests/CMakeLists.txt

@@ -154,3 +154,13 @@ if( CRITERION_FOUND )
         endif()  # LP Error: Xpress returned 120, #3724 (LPS=xprs, unittest-cons-nonlinear-vertexpolyhedral)
     endforeach(testSrc)
 endif()
+
+# Fault injection uses ELF symbol interposition and does not require Criterion.
+if(CMAKE_SYSTEM_NAME STREQUAL "Linux" AND SHARED)
+    add_executable(test_problem_array_allocation_failure problem_array_allocation_failure.cpp)
+    target_link_libraries(test_problem_array_allocation_failure PRIVATE libscip ${CMAKE_DL_LIBS})
+    target_compile_features(test_problem_array_allocation_failure PRIVATE cxx_std_11)
+    target_compile_options(test_problem_array_allocation_failure PRIVATE -UNDEBUG)
+    add_test(NAME problem_array_allocation_failure COMMAND test_problem_array_allocation_failure)
+    set_tests_properties(problem_array_allocation_failure PROPERTIES TIMEOUT 20)
+endif()

tests/problem_array_allocation_failure.cpp

@@ -0,0 +1,75 @@
+// Fail problem-array growth, then release the rejected object and the partial model.
+#include <scip/scip.h>
+#include <scip/scipdefplugins.h>
+#include <scip/cons_linear.h>
+#include <dlfcn.h>
+#include <cassert>
+#include <cstring>
+#include <string>
+#include <sys/resource.h>
+
+static bool fail_growth = false;
+static bool injected = false;
+
+// Interpose only this executable's calls, including libscip's; no production fault injection.
+extern "C" void* BMSreallocMemoryArray_call(void* ptr, size_t count, size_t size,
+                                          const char* file, int line) {
+    using Function = void* (*)(void*, size_t, size_t, const char*, int);
+    static auto original = reinterpret_cast<Function>(dlsym(RTLD_NEXT, "BMSreallocMemoryArray_call"));
+    assert(original);
+    if (fail_growth && std::strstr(file, "/scip/prob.c")) {
+        fail_growth = false;
+        injected = true;
+        return nullptr;
+    }
+    return original(ptr, count, size, file, line);
+}
+
+static void check(bool constraints) {
+    SCIP* scip = nullptr;
+    assert(SCIPcreate(&scip) == SCIP_OKAY);
+    assert(SCIPincludeDefaultPlugins(scip) == SCIP_OKAY);
+    assert(SCIPcreateProbBasic(scip, "allocation_failure") == SCIP_OKAY);
+    SCIP_VAR* x = nullptr;
+    if (constraints) {
+        assert(SCIPcreateVarBasic(scip, &x, "x", 0, 1, 0, SCIP_VARTYPE_CONTINUOUS) == SCIP_OKAY);
+        assert(SCIPaddVar(scip, x) == SCIP_OKAY);
+    }
+    injected = false;
+    for (int i = 0; i < 256 && !injected; ++i) {
+        fail_growth = i >= 8; // Ensure the array already owns entries when resizing fails.
+        const auto name = std::to_string(i);
+        SCIP_RETCODE code;
+        if (constraints) {
+            SCIP_CONS* cons = nullptr;
+            double coefficient = 1;
+            assert(SCIPcreateConsBasicLinear(scip, &cons, name.c_str(), 1, &x, &coefficient, 0, 1) == SCIP_OKAY);
+            code = SCIPaddCons(scip, cons);
+            if (injected) {
+                assert(SCIPconsGetNUses(cons) == 1);
+                assert(!SCIPconsIsAdded(cons));
+            }
+            assert(SCIPreleaseCons(scip, &cons) == SCIP_OKAY);
+        } else {
+            SCIP_VAR* var = nullptr;
+            assert(SCIPcreateVarBasic(scip, &var, name.c_str(), 0, 1, 0, SCIP_VARTYPE_CONTINUOUS) == SCIP_OKAY);
+            code = SCIPaddVar(scip, var);
+            if (injected) assert(SCIPvarGetNUses(var) == 1);
+            assert(SCIPreleaseVar(scip, &var) == SCIP_OKAY);
+        }
+        assert(code == (injected ? SCIP_NOMEMORY : SCIP_OKAY));
+    }
+    assert(injected);
+    fail_growth = false;
+    if (x) assert(SCIPreleaseVar(scip, &x) == SCIP_OKAY);
+    assert(SCIPfree(&scip) == SCIP_OKAY);
+    assert(!scip);
+}
+
+int main(int argc, char** argv) {
+    const rlimit core{0, 0};
+    assert(setrlimit(RLIMIT_CORE, &core) == 0);
+    // Optional selector permits demonstrating each crash against the unpatched library.
+    if (argc == 1 || std::string(argv[1]) == "variables") check(false);
+    if (argc == 1 || std::string(argv[1]) == "constraints") check(true);
+}