use-of-uninitialized-value in SCIPparamGetChar

[lperron]

We have this issue when using scip with memory sanitizer with scip called from OR-Tools MPSolver

==2807165==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7fe597a44cbd in SCIPparamGetChar third_party/scip/src/scip/paramset.c:886:1
    #1 0x7fe597a4a470 in SCIPparamSetChar third_party/scip/src/scip/paramset.c:4774:18
    #2 0x7fe597a46ab2 in paramCreateChar third_party/scip/src/scip/paramset.c:1157:4
    #3 0x7fe597a46ab2 in SCIPparamsetAddChar third_party/scip/src/scip/paramset.c:1635:4
    #4 0x7fe597e5afcc in SCIPsetAddCharParam third_party/scip/src/scip/set.c:3038:4
    #5 0x7fe597e528a2 in SCIPsetCreate third_party/scip/src/scip/set.c:1247:4
    #6 0x7fe597c9f2d0 in doScipCreate third_party/scip/src/scip/scip_general.c:250:4
    #7 0x7fe597c9f2d0 in SCIPcreate third_party/scip/src/scip/scip_general.c:298:4
    #8 0x7fe59d614341 in operations_research::SCIPInterface::CreateSCIP() util/operations_research/linear_solver/scip_interface.cc:294:3

where memory is allocated during:

  Uninitialized value was created by a heap allocation
    #0 0x5632b292ee12 in malloc third_party/llvm/llvm-project/compiler-rt/lib/msan/msan_interceptors.cpp:1007:3
    #1 0x7fe596fefdb7 in BMSallocMemory_call third_party/scip/src/blockmemshell/memory.c:411:10
    #2 0x7fe597e514ff in SCIPsetCreate third_party/scip/src/scip/set.c:1110:4
    #3 0x7fe597c9f2d0 in doScipCreate third_party/scip/src/scip/scip_general.c:250:4
    #4 0x7fe597c9f2d0 in SCIPcreate third_party/scip/src/scip/scip_general.c:298:4
    #5 0x7fe59d614341 in operations_research::SCIPInterface::CreateSCIP() util/operations_research/linear_solver/scip_interface.cc:294:3
    #6 0x7fe59d614093 in operations_research::SCIPInterface::SCIPInterface(operations_research::MPSolver*) util/operations_research/linear_solver/scip_interface.cc:261:13
    #7 0x7fe59d62a271 in operations_research::BuildSCIPInterface(operations_research::MPSolver*) util/operations_research/linear_solver/scip_interface.cc:1187:14

[matbesancon]

cc @pfetsch do you have hints on what is happening here? Since this is about the SCIP memory management

[svigerske]

It could be helpful to know what version of SCIP this is, since the line numbers don't seem to match in the last release (8.0.4).

Compiling with NOBLKBUFMEM=true (or -DBMS_NOBLOCKMEM -DSCIP_NOBUFFERMEM) may also help to get more precise output, though in this case it seems quite clear what this is about. It should be harmless, but I wonder why this didn't happen more often.

[lperron]

this is 8.0.4 scipoptsuite tarz. Laurent Perron | Operations Research | @.*** | (33) 1 42 68 53 00

Le mar. 14 nov. 2023 à 12:05, Stefan Vigerske @.***> a écrit :

It could be helpful to know what version of SCIP this is, since the line numbers don't seem to match in the last release (8.0.4).

Compiling with NOBLKBUFMEM=true (or -DBMS_NOBLOCKMEM -DSCIP_NOBUFFERMEM) may also help to get more precise output, though in this case it seems quite clear what this is about. It should be harmless, but I wonder why this didn't happen more often.

— Reply to this email directly, view it on GitHub https://github.com/scipopt/scip/issues/68#issuecomment-1809996558, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACUPL3NFIJ5WOVHTYF4QI4LYENF67AVCNFSM6AAAAAA7KS25KSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBZHE4TMNJVHA . You are receiving this because you authored the thread.Message ID: @.***>

[lperron]

Is there a workaround ? I cannot turn off msan in our tests.

Laurent Perron | Operations Research | @.*** | (33) 1 42 68 53 00

Le mar. 14 nov. 2023 à 12:36, Laurent Perron @.***> a écrit :

this is 8.0.4 scipoptsuite tarz. Laurent Perron | Operations Research | @.*** | (33) 1 42 68 53 00

Le mar. 14 nov. 2023 à 12:05, Stefan Vigerske @.***> a écrit :

It could be helpful to know what version of SCIP this is, since the line numbers don't seem to match in the last release (8.0.4).

Compiling with NOBLKBUFMEM=true (or -DBMS_NOBLOCKMEM -DSCIP_NOBUFFERMEM) may also help to get more precise output, though in this case it seems quite clear what this is about. It should be harmless, but I wonder why this didn't happen more often.

— Reply to this email directly, view it on GitHub https://github.com/scipopt/scip/issues/68#issuecomment-1809996558, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACUPL3NFIJ5WOVHTYF4QI4LYENF67AVCNFSM6AAAAAA7KS25KSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBZHE4TMNJVHA . You are receiving this because you authored the thread.Message ID: @.***>

[svigerske]

OK, I seem to have had mixed up line numbers.

Does this change fixes it for you?

--- a/src/scip/paramset.c
+++ b/src/scip/paramset.c
@@ -1147,6 +1147,7 @@ SCIP_RETCODE paramCreateChar(
    (*param)->paramtype = SCIP_PARAMTYPE_CHAR;
    (*param)->data.charparam.valueptr = valueptr;
    (*param)->data.charparam.defaultvalue = defaultvalue;
+   (*param)->data.charparam.curvalue = defaultvalue;
    if( allowedvalues != NULL )
    {
       SCIP_ALLOC( BMSduplicateMemoryArray(&(*param)->data.charparam.allowedvalues, allowedvalues, strlen(allowedvalues)+1) );

[lperron]

It does not. Is the init called ?

[svigerske]

Not sure which "init" you mean.

The use-of-uninitialized-value is in SCIPparamGetChar in third_party/scip/src/scip/paramset.c:886:1, this is

   return param->data.charparam.curvalue;

It is called from SCIPparamSetChar(), which is called from paramCreateChar() (paramset.c:1157), so I hoped that adding an initialization of param->data.charparam.curvalue a few lines before (paramset.c:1150) should fix this.

My only other guess now is that a char is small and for some reason, it is reading more than the one curvalue byte, which then gives a read of uninitialized data. But this seems a bit far fetched.

[svigerske]

I now see that this is about the first parameter created. So, do you actually get more than this one warning? Sure that after the proposed fix, the remaining warning is still about paramCreateChar() and not another paramCreate*() ?

[lperron]

This is the only one I get (surprisingly as all params share the same structure).

[svigerske]

OK. third_party/scip/src/scip/paramset.c:886:1 is actually '}', and it should actually return the value in valueptr in the case. So try this change instead:

--- a/src/scip/paramset.c
+++ b/src/scip/paramset.c
@@ -1147,6 +1147,8 @@ SCIP_RETCODE paramCreateChar(
    (*param)->paramtype = SCIP_PARAMTYPE_CHAR;
    (*param)->data.charparam.valueptr = valueptr;
    (*param)->data.charparam.defaultvalue = defaultvalue;
+   if( valueptr != NULL )
+      *valueptr = defaultvalue;
    if( allowedvalues != NULL )
    {
       SCIP_ALLOC( BMSduplicateMemoryArray(&(*param)->data.charparam.allowedvalues, allowedvalues, strlen(allowedvalues)+1) );

[lperron]

Good news, it works for char. Bad news, it crashes later:

==3776419==WARNING: MemorySanitizer: use-of-uninitialized-value

0 0x561ed56ae273 in SCIPparamGetReal third_party/scip/src/scip/paramset.c:839:1

#1 0x561ed56b2602 in SCIPparamSetReal third_party/scip/src/scip/paramset.c:4720:18
#2 0x561ed56afb21 in paramCreateReal third_party/scip/src/scip/paramset.c:1121:4
#3 0x561ed56afb21 in SCIPparamsetAddReal third_party/scip/src/scip/paramset.c:1611:4
#4 0x561ed5cab7bf in SCIPsetAddRealParam third_party/scip/src/scip/set.c:3015:4
#5 0x561ed5ca38aa in SCIPsetCreate third_party/scip/src/scip/set.c:1252:4

[svigerske]

I don't think it crashes. It reads an uninitialized value. But it hardly does anything with it, that's why this is a harmless warning.

The same fix as for paramCreateChar() would then need to go into the other paramCreate*() functions.

[lperron]

I know. It is just crashing our tests. Laurent Perron | Operations Research | @.*** | (33) 1 42 68 53 00

Le mar. 14 nov. 2023 à 18:21, Stefan Vigerske @.***> a écrit :

I don't think it crashes. It reads an uninitialized value. But it hardly does anything with it, that's why this is a harmless warning.

The same fix as for paramCreateChar() would then need to go into the other paramCreate*() functions.

— Reply to this email directly, view it on GitHub https://github.com/scipopt/scip/issues/68#issuecomment-1810742652, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACUPL3KB77DA45W4UCKAPJ3YEOSCJAVCNFSM6AAAAAA7KS25KSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJQG42DENRVGI . You are receiving this because you authored the thread.Message ID: @.***>

[lperron]

If I fix all code paths, It still crashes in SCIPParamGetBool. Laurent Perron | Operations Research | @.*** | (33) 1 42 68 53 00

Le mar. 14 nov. 2023 à 18:22, Laurent Perron @.***> a écrit :

I know. It is just crashing our tests. Laurent Perron | Operations Research | @.*** | (33) 1 42 68 53 00

Le mar. 14 nov. 2023 à 18:21, Stefan Vigerske @.***> a écrit :

I don't think it crashes. It reads an uninitialized value. But it hardly does anything with it, that's why this is a harmless warning.

The same fix as for paramCreateChar() would then need to go into the other paramCreate*() functions.

— Reply to this email directly, view it on GitHub https://github.com/scipopt/scip/issues/68#issuecomment-1810742652, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACUPL3KB77DA45W4UCKAPJ3YEOSCJAVCNFSM6AAAAAA7KS25KSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJQG42DENRVGI . You are receiving this because you authored the thread.Message ID: @.***>

[svigerske]

Here is a more complete patch:

--- a/src/scip/paramset.c
+++ b/src/scip/paramset.c
@@ -640,7 +640,7 @@ SCIP_RETCODE paramCopyString(

    /* get value of source parameter and copy it to target parameter */
    value = SCIPparamGetString(sourceparam);
-   SCIP_CALL( SCIPparamSetString(targetparam, set, messagehdlr, value, TRUE) );
+   SCIP_CALL( SCIPparamSetString(targetparam, set, messagehdlr, value, FALSE, TRUE) );

    return SCIP_OKAY;
 }
@@ -1186,7 +1186,7 @@ SCIP_RETCODE paramCreateString(
    SCIP_ALLOC( BMSduplicateMemoryArray(&(*param)->data.stringparam.defaultvalue, defaultvalue, strlen(defaultvalue)+1) );
    (*param)->data.stringparam.curvalue = NULL;

-   SCIP_CALL( SCIPparamSetString(*param, NULL, messagehdlr, defaultvalue, TRUE) );
+   SCIP_CALL( SCIPparamSetString(*param, NULL, messagehdlr, defaultvalue, TRUE, TRUE) );

    return SCIP_OKAY;
 }
@@ -1412,7 +1412,7 @@ SCIP_RETCODE paramParseString(
    /* remove the quotes */
    valuestr[len-1] = '\0';
    valuestr++;
-   SCIP_CALL( SCIPparamSetString(param, set, messagehdlr, valuestr, TRUE) );
+   SCIP_CALL( SCIPparamSetString(param, set, messagehdlr, valuestr, FALSE, TRUE) );

    return SCIP_OKAY;
 }
@@ -2135,7 +2135,7 @@ SCIP_RETCODE SCIPparamsetSetString(
    }

    /* set the parameter's current value */
-   SCIP_CALL( SCIPparamSetString(param, set, messagehdlr, value, TRUE) );
+   SCIP_CALL( SCIPparamSetString(param, set, messagehdlr, value, FALSE, TRUE) );

    return SCIP_OKAY;
 }
@@ -4529,15 +4529,17 @@ SCIP_RETCODE SCIPparamSetBool(
       /* check if the parameter is not fixed */
       SCIP_CALL_QUIET( paramTestFixed(param, messagehdlr) );

+      if( !initialize )
+         oldvalue = SCIPparamGetBool(param);
+
       /* set the parameter's current value */
-      oldvalue = SCIPparamGetBool(param);
       if( param->data.boolparam.valueptr != NULL )
          *param->data.boolparam.valueptr = value;
       else
          param->data.boolparam.curvalue = value;

-      /* call the parameter's change information method */
-      if( param->paramchgd != NULL && set != NULL )
+      /* call the parameter's change information method, unless initializing */
+      if( !initialize && param->paramchgd != NULL && set != NULL )
       {
          SCIP_RETCODE retcode;

@@ -4589,15 +4591,17 @@ SCIP_RETCODE SCIPparamSetInt(
       /* check if the parameter is not fixed */
       SCIP_CALL_QUIET( paramTestFixed(param, messagehdlr) );

+      if( !initialize )
+         oldvalue = SCIPparamGetInt(param);
+
       /* set the parameter's current value */
-      oldvalue = SCIPparamGetInt(param);
       if( param->data.intparam.valueptr != NULL )
          *param->data.intparam.valueptr = value;
       else
          param->data.intparam.curvalue = value;

-      /* call the parameter's change information method */
-      if( param->paramchgd != NULL && set != NULL )
+      /* call the parameter's change information method, unless initialization */
+      if( !initialize && param->paramchgd != NULL && set != NULL )
       {
          SCIP_RETCODE retcode;

@@ -4649,15 +4653,17 @@ SCIP_RETCODE SCIPparamSetLongint(
       /* check if the parameter is not fixed */
       SCIP_CALL_QUIET( paramTestFixed(param, messagehdlr) );

+      if( !initialize )
+         oldvalue = SCIPparamGetLongint(param);
+
       /* set the parameter's current value */
-      oldvalue = SCIPparamGetLongint(param);
       if( param->data.longintparam.valueptr != NULL )
          *param->data.longintparam.valueptr = value;
       else
          param->data.longintparam.curvalue = value;

-      /* call the parameter's change information method */
-      if( param->paramchgd != NULL && set != NULL )
+      /* call the parameter's change information method, unless initialization */
+      if( !initialize && param->paramchgd != NULL && set != NULL )
       {
          SCIP_RETCODE retcode;

@@ -4711,15 +4717,17 @@ SCIP_RETCODE SCIPparamSetReal(
       /* check if the parameter is not fixed */
       SCIP_CALL_QUIET( paramTestFixed(param, messagehdlr) );

+      if( !initialize )
+         oldvalue = SCIPparamGetReal(param);
+
       /* set the parameter's current value */
-      oldvalue = SCIPparamGetReal(param);
       if( param->data.realparam.valueptr != NULL )
          *param->data.realparam.valueptr = value;
       else
          param->data.realparam.curvalue = value;

-      /* call the parameter's change information method */
-      if( param->paramchgd != NULL && set != NULL )
+      /* call the parameter's change information method, unless initializing */
+      if( !initialize && param->paramchgd != NULL && set != NULL )
       {
          SCIP_RETCODE retcode;

@@ -4770,15 +4778,17 @@ SCIP_RETCODE SCIPparamSetChar(

       SCIP_CALL_QUIET( paramTestFixed(param, messagehdlr) );

+      if( !initialize )
+         oldvalue = SCIPparamGetChar(param);
+
       /* set the parameter's current value */
-      oldvalue = SCIPparamGetChar(param);
       if( param->data.charparam.valueptr != NULL )
          *param->data.charparam.valueptr = value;
       else
          param->data.charparam.curvalue = value;

-      /* call the parameter's change information method */
-      if( param->paramchgd != NULL && set != NULL )
+      /* call the parameter's change information method, unless initializing */
+      if( !initialize && param->paramchgd != NULL && set != NULL )
       {
          SCIP_RETCODE retcode;

@@ -4812,6 +4822,7 @@ SCIP_RETCODE SCIPparamSetString(
    SCIP_SET*             set,                /**< global SCIP settings, or NULL if param change method should not be called */
    SCIP_MESSAGEHDLR*     messagehdlr,        /**< message handler */
    const char*           value,              /**< new value of the parameter */
+   SCIP_Bool             initialize,         /**< is this the initialization of the parameter? */
    SCIP_Bool             quiet               /**< should the parameter be set quiet (no output) */
    )
 {
@@ -4826,17 +4837,19 @@ SCIP_RETCODE SCIPparamSetString(
    /* set the parameter's current value */
    if( param->data.stringparam.valueptr != NULL )
    {
-      oldvalue = *param->data.stringparam.valueptr;
+      if( !initialize )
+         oldvalue = *param->data.stringparam.valueptr;
       SCIP_ALLOC( BMSduplicateMemoryArray(param->data.stringparam.valueptr, value, strlen(value)+1) );
    }
    else
    {
-      oldvalue = param->data.stringparam.curvalue;
+      if( !initialize )
+         oldvalue = param->data.stringparam.curvalue;
       SCIP_ALLOC( BMSduplicateMemoryArray(&param->data.stringparam.curvalue, value, strlen(value)+1) );
    }

-   /* call the parameter's change information method */
-   if( param->paramchgd != NULL && set != NULL )
+   /* call the parameter's change information method, unless initializing */
+   if( !initialize && param->paramchgd != NULL && set != NULL )
    {
       SCIP_RETCODE retcode;

@@ -4993,7 +5006,7 @@ SCIP_RETCODE SCIPparamSetToDefault(
       break;

    case SCIP_PARAMTYPE_STRING:
-      SCIP_CALL( SCIPparamSetString(param, set, messagehdlr, SCIPparamGetStringDefault(param), TRUE) );
+      SCIP_CALL( SCIPparamSetString(param, set, messagehdlr, SCIPparamGetStringDefault(param), FALSE, TRUE) );
       break;

    default:
diff --git a/src/scip/paramset.h b/src/scip/paramset.h
index 9674b46c76..d744e805fc 100644
--- a/src/scip/paramset.h
+++ b/src/scip/paramset.h
@@ -523,6 +523,7 @@ SCIP_RETCODE SCIPparamSetString(
    SCIP_SET*             set,                /**< global SCIP settings, or NULL if param change method should not be called */
    SCIP_MESSAGEHDLR*     messagehdlr,        /**< message handler */
    const char*           value,              /**< new value of the parameter */
+   SCIP_Bool             initialize,         /**< is this the initialization of the parameter? */
    SCIP_Bool             quiet               /**< should the parameter be set quiet (no output) */
    );

diff --git a/src/scip/set.c b/src/scip/set.c
index 33dcdb06e5..caf3485cb0 100644
--- a/src/scip/set.c
+++ b/src/scip/set.c
@@ -3414,7 +3414,7 @@ SCIP_RETCODE SCIPsetChgStringParam(
    assert(set != NULL);
    assert(param != NULL);

-   retcode = SCIPparamSetString(param, set, messagehdlr, value, TRUE);
+   retcode = SCIPparamSetString(param, set, messagehdlr, value, FALSE, TRUE);

    if( retcode != SCIP_PARAMETERWRONGVAL )
    {

(paramset.patch.txt)

If something still comes up, then we need the backtrace again.

[lperron]

First tests seem ok. Thanks for the speedy patch.