Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

use-of-uninitialized-value in SCIPparamGetChar #68

Closed
lperron opened this issue Nov 14, 2023 · 16 comments
Closed

use-of-uninitialized-value in SCIPparamGetChar #68

lperron opened this issue Nov 14, 2023 · 16 comments
Assignees
@svigerske

Comments

@lperron
Copy link

lperron commented Nov 14, 2023

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
Copy link
Member

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

@svigerske
Copy link
Member

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.

@ju-manns ju-manns assigned svigerske and unassigned ju-manns Nov 14, 2023
@lperron
Copy link
Author

lperron commented Nov 14, 2023 via email

@lperron
Copy link
Author

lperron commented Nov 14, 2023 via email

@svigerske
Copy link
Member

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
Copy link
Author

lperron commented Nov 14, 2023

It does not. Is the init called ?

@svigerske
Copy link
Member

svigerske commented Nov 14, 2023 •
edited

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
Copy link
Member

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
Copy link
Author

lperron commented Nov 14, 2023

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

@svigerske
Copy link
Member

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
Copy link
Author

lperron commented Nov 14, 2023

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
Copy link
Member

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
Copy link
Author

lperron commented Nov 14, 2023 via email

@lperron
Copy link
Author

lperron commented Nov 14, 2023 via email

@svigerske
Copy link
Member

svigerske commented Nov 14, 2023 •
edited

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
Copy link
Author

lperron commented Nov 15, 2023 •
edited

First tests seem ok. Thanks for the speedy patch.

@lperron lperron closed this as completed Nov 15, 2023
scip-ci pushed a commit that referenced this issue Nov 16, 2023
@svigerske
do not read parameter current value when creating parameter
c9948e8 
- the parameter value may not have been initialized, which raises
  uninitialized memory read warnings in memory checkers, e.g., valgrind
- do no longer call the paramchg callback when creating a parameter,
  because, in case of a failure, we would try to reset the value to the
  old value, but now we have no longer read the old value
- it also does not seem logical to claim a parameter change at the
  moment a parameter is created
- fixes #68
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@svigerske svigerske

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@matbesancon @svigerske @lperron @ju-manns