Bug 2586 - Recursive Object Deserialization
Recursive Object Deserialization
Status: PATCH PENDING
Product: ns-3
Classification: Unclassified
Component: core
unspecified
All All
: P3 normal
Assigned To: Peter Barnes
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-14 17:14 EST by Tom Henderson
Modified: 2018-09-28 13:52 EDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Henderson 2016-12-14 17:14:25 EST
Summarized in this post (with patch provided):

https://groups.google.com/forum/#!topic/ns-3-users/37BJICZiAgk
Comment 1 Peter Barnes 2016-12-14 17:38:33 EST
To ease future the way for future readers, here's the ns-3-users list posting:

I am trying to place nodes uniformly in a circle around a fixed point, but getting a parse error trying to create the position allocator.

The code I wrote to do it looks something like this:

    NodeContainer myNodes;
    // ... code to configure myNodes ..
    std::ostringstream myAngles;
    myAngles
           << "ns3::SequentialRandomVariable[Min=0.0|Max=6.28|Consecutive=1|"
           << "Increment=ns3::ConstantRandomVariable[Constant="
           << 6.28 / myNodes.GetN()
           << "]]";
    mobility.SetPositionAllocator ("ns3::RandomDiscPositionAllocator",
           "X", DoubleValue (10.0),
           "Y", DoubleValue (10.0),
           "Rho" , StringValue ("ns3::ConstantRandomVariable[Constant=8.0]"),
           "Theta", StringValue (myAngles.str()));
    mobility.SetMobilityModel ("ns3::ConstantPositionMobilityModel");
    mobility.Install (myNodes);

I get a SIGSEGV running the code above.  When I debug into it, ns3::PointerValue::DeserializeFromString is trying to create an object of type "[ns3::ConstantRandomVariable[Constant=1.256", which ultimately causes ns3::IidManager::GetConstructor to crash.

The StringValue that gets passed to SetPositionAllocator is "ns3::SequentialRandomVariable[Min=0.0|Max=6.28|Consecutive=1|Increment=ns3::ConstantRandomVariable[Constant=1.256]]".  The problem is that string parameter for the `Rho` attribute has nested `[` and ']' characters in it.  When ns3::PointerValue::DeserialzeFromString tries to deserialize the "Rho" attribute, `std::istream & operator >> (std::istream &is, ObjectFactory &factory)` defined in src/core/model/object-factory.cc finds the closing ']' character using std::string::find, which gets the first occurrence of ']' rather than the second.  When it tries to parse the value to use for Increment, the closing ']' is missing, so parsing fails.

If I change operator >> to use `std::string::rfind` to get the closing ']' instead of `std::string::find`, the above code seems to work, and the NS3 unit tests all pass.  Here a patch of the change:

--- a/src/core/model/object-factory.cc    Mon Dec 05 17:37:33 2016 -0800
+++ b/src/core/model/object-factory.cc    Wed Dec 14 11:42:29 2016 -0800
@@ -126,7 +126,7 @@
   is >> v;
   std::string::size_type lbracket, rbracket;
   lbracket = v.find ("[");
-  rbracket = v.find ("]");
+  rbracket = v.rfind ("]");
   if (lbracket == std::string::npos && rbracket == std::string::npos)
     {
       factory.SetTypeId (v);

Is this a bug?  That is, are recursive objects allowed in an attribute definition?  Is there a case where using `rfind` instead of `find` will break something else?

FWIW, I am using version 3.26.

Thanks,
Bill.
Comment 2 Peter Barnes 2016-12-14 17:41:04 EST
I agree with Tom's reply on the users' list, I never considered recursive attribute serialization.

It's not clear to me that rfind is a general solution. Does the internal attribute get parsed with a substring? so the last `]' is the closing for the current attribute?  I haven't stared at the code to see what should happen.
Comment 3 Luciano Chaves 2018-09-28 01:15:21 EDT
Hello guys,

One of my students (Seany) recently reported this same bug again as a new one (#2991). Yeah, we should have looked first and found this one here. Anyway, the proposed patch here was also my first idea to solve this problem. Using rfind to look for the right ] (assuming that the string is a valid one with balanced number of [ and ]) seems to be solve the problem, as parsing the inner string will be automatically handled by the indirect recursion on this serialization code. However, when the inner description has more than one attribute, the | will be used to separate them, and the current serialization will look for the next '|' char in the string to identify where one attribute ends and where the next att begins. So, this would lead to a wrong inner description token.

The proposed solution there (#2991) includes a "smart" parsing that considers the number of nested descriptions when looking for the end of the attribute value (using the concept of level). There is a "complex" test case there where you can play with attributes order and recursion on config store file to test the patch.

Cheers, Luciano.
Comment 4 Peter Barnes 2018-09-28 13:52:38 EDT
I'm open to a patch which just fixes the nesting issue, but I wonder whether that covers all cases.  Suppose the ']' found at a level is actually quoted, and should be ignored?  Or the '[' is actually quoted, so the patch increases the level erroneously?What about malformed attribute strings?  To handle all cases properly do we really need a full parser?