cyberdstar / muparserx

Automatically exported from code.google.com/p/muparserx
Other
0 stars 0 forks source link

Segmentation fault in mup::Variable::GetRows #40

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
From a program I'm writing which uses muparserx to assign values to simulation 
variables from a configuration file:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000000003 in ?? ()
(gdb) up
#1  0x000000010003d56a in mup::Variable::GetRows (this=0x10050b8e0) at 
parser/mpVariable.cpp:248
248     return m_pVal->GetRows();
(gdb) 
#2  0x0000000100066d61 in mup::OprtIndex::Eval (this=0x10050c7b0, ret=..., 
a_pArg=0x10050ca38, a_iArgc=1)
    at parser/mpOprtIndex.cpp:61
61              int rows = a_pArg[-1]->GetRows();
(gdb) 
#3  0x000000010002b30e in mup::ParserXBase::ParseFromRPN (this=0x100506d48) at 
parser/mpParserBase.cpp:1090
1090              pIdxOprt->Eval(val, &idx, nArgs);
(gdb) 
#4  0x000000010002ae6d in mup::ParserXBase::ParseFromString (this=0x100506d48) 
at parser/mpParserBase.cpp:1004
1004      return (this->*m_pParserEngine)();
(gdb) 
#5  0x0000000100028521 in mup::ParserXBase::Eval (this=0x100506d48) at 
parser/mpParserBase.cpp:243
243   return (this->*m_pParserEngine)();
(gdb) 
#6  0x000000010000b9ab in 
pamhd::boundaries::Variable_To_Option_Vector<Mass_Density>::get_parsed_value<dou
ble> (
    this=0x100506d30, given_position=..., given_time=0) at source/boundaries/variable_to_option_vector.hpp:722
722             const auto& evaluated = this->parser.Eval();

Some values for variables above:
print rows
$3 = 1
print m_pVal
warning: RTTI symbol not found for class 'mup::Variable'
$4 = (IValue *) 0x7fff5fbff0e8

When calling parser.Eval() the expression is r[0] + r[1] + r[2] + t where r and 
t are
this->parser.DefineVar("r", this->r_var);
this->parser.DefineVar("t", this->t_var);
and set to
this->r_val = mup::Value(3, 0); r_val[0] = 1; r_val[1] = 2; r_val[2] = 3;
this->t_val = doube(0);
before calling Eval().

What steps will reproduce the problem?

The program I received this from is complicated, I might be able to make a 
simplified test. If not I'll probably upload the whole thing to github.

What version of the product are you using?

3.0.1 compiled with -g instead of -O3. I get the same result with -O3 but 
without any line numbers in muparserx.

Original issue reported on code.google.com by ilja.j.h...@nasa.gov on 23 Jul 2014 at 4:43

GoogleCodeExporter commented 9 years ago
Never mind, I forgot to make a copy constructor that always uses the current 
class instance's mup::Value for its mup::Variable instead of the other class' 
mup::Value. When pushing the class to a vector with v.push_back(C()) the final 
mup::Variable ended up referring to a temporary mup::Value (created in C()) 
which was destroyed after the push_back.

Is there a use case for having multiple mup::Variables referring to the same 
mup::Value? And even if there is I guess there'd be even less use for 
intentionally copying a mup::Variable which would still refer to the same 
mup::Value. Deleting the copy constructor for mup::Variable would've saved a 
few hours of debugging...

Original comment by ilja.j.h...@nasa.gov on 23 Jul 2014 at 6:45

GoogleCodeExporter commented 9 years ago
Thanks for the clearification. There is no immediate use i can think of for 
letting to variables point to the same value in regular use. There may be a 
need to do it temporarily when storing Variable objects in STL containes. There 
may also be the need to have a copy constructor when returning it in a tuple. 
(I did not check this, just a guess...) Sorry for your debugging hours but you 
know this is open source i get nothing out of it and you can rest assured that 
i spent hundrets of hours coding and maintaining it.

Original comment by ib...@gmx.info on 26 Jul 2014 at 2:12

GoogleCodeExporter commented 9 years ago
Don't worry about the hours and muparserx is a nice library. There do appear to 
be some rough corner cases though, like this issue and 39. I'm not sure whether 
the rule or three or five is relevant here but since there is no default 
constructor there probably shouldn't be a copy constructor either. A variable 
and value seem to always go together and copying a variable alone probably 
doesn't make sense.

In the long run variables and values could be merged together or is either one 
needed separately? Actually couldn't both be part of parser? It already has 
DefineVar("r", var), why couldn't that create an internal pair of variable and 
value which are exposed from parser by e.g. parser.Get("r") = 3? Another 
example: if parser.Type("r") == 'm' then parser.Get("r").At(i) = 3.

Original comment by ilja.j.h...@nasa.gov on 26 Jul 2014 at 5:13

GoogleCodeExporter commented 9 years ago
At some point in the future variables and values will probably merge due to 
implementation reasons. Lookup with a Get("...") interface will not happen 
though. 

Original comment by ib...@gmx.info on 31 Jul 2014 at 8:04

GoogleCodeExporter commented 9 years ago
I decided to close the issue since it appears to be related to the way 
muparserx is used by the client code. If you still have problems with this 
please create a small sample application illustrating the isssue and open a new 
ticket.

Original comment by ib...@gmx.info on 23 Mar 2015 at 5:02

GoogleCodeExporter commented 9 years ago
Well the underlying issue isn't fixed if there indeed is one, this program 
shows the problem I had and worked around when realized what it was:

#include "vector"
#include "mpParser.h"
using namespace std;
int main()
{
    mup::ParserX parser(mup::pckCOMMON | mup::pckNON_COMPLEX | mup::pckMATRIX | mup::pckUNIT);
    std::vector<mup::Variable> variables;
    { // comment out this line to fix crash
        mup::Value value1{0};
        variables.push_back(mup::Variable(&value1));
    } // comment out this line to fix crash

    parser.DefineVar("v1", variables[0]);
    parser.SetExpr("v1 + 1");
    parser.Eval().GetInteger();
}

And the error when I run it on OSX: /bin/sh: line 1: 46277 Bus error: 10

To me it seems that every variable can only reference one value and a variable 
always has to reference a value so having them separately only serves to 
introduce bugs like this, i.e. when copying a variable it continues to 
reference the only value which might be deallocated with the first variable.

Original comment by ilja.j.h...@nasa.gov on 23 Mar 2015 at 5:24

GoogleCodeExporter commented 9 years ago
I created https://code.google.com/p/muparserx/issues/detail?id=48

Original comment by ilja.j.h...@nasa.gov on 23 Mar 2015 at 5:34