Bug 2442 - TrafficControl makes ConfigStore crash
TrafficControl makes ConfigStore crash
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: traffic-control
ns-3-dev
All All
: P5 blocker
Assigned To: Stefano Avallone
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-06-18 19:08 UTC by Tommaso Pecorella
Modified: 2016-06-29 12:32 UTC (History)
3 users (show)

See Also:


Attachments
crash example (7.45 KB, text/x-csrc)
2016-06-23 15:30 UTC, Tommaso Pecorella
Details
Fix for AttributeIterator (1.09 KB, patch)
2016-06-24 13:32 UTC, Stefano Avallone
Details | Diff
Fix for traffic control (1.28 KB, patch)
2016-06-24 13:56 UTC, Stefano Avallone
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tommaso Pecorella 2016-06-18 19:08:33 UTC
The error has been reported here:
https://groups.google.com/forum/#!topic/ns-3-users/TR9LT4bgeQg

It is not limited to GtkConfigStore. Also "simply" saving the default object configs will make a program crash.

As an example open any script and add the following:

  Config::SetDefault ("ns3::ConfigStore::Filename", StringValue ("output-attributes.txt"));
  Config::SetDefault ("ns3::ConfigStore::FileFormat", StringValue ("RawText"));
  Config::SetDefault ("ns3::ConfigStore::Mode", StringValue ("Save"));

  ConfigStore config;
Comment 1 Stefano Avallone 2016-06-23 13:27:23 UTC
I cannot reproduce the crashes with the current ns-3-dev (neither with ns-3.25).

I added the lines you mention to a couple of examples (traffic-control and mixed-wireless) and they both run fine.

I ran the lena-simple-epc example shipped with ns-3 (i.e., without GtkConfigStore enabled) and worked fine.

The example attached by the user, which differs from the one shipped by ns-3 in that GtkConfigStore is enabled, crashes instead. Thus, it seems that GtkConfigStore actually does the difference. Any clue about possible reasons?
Comment 2 Tommaso Pecorella 2016-06-23 15:30:14 UTC
Created attachment 2481 [details]
crash example

This is an example of the crash with the plain ConfigStore
Comment 3 Tom Henderson 2016-06-23 16:05:38 UTC
This attribute seems to be wrong (using wrong accessor methods):

    .AddAttribute ("RootQueueDiscList", "The list of root queue discs associated to this Traffic Control layer.",
                   ObjectMapValue (),
                   MakeObjectMapAccessor (&TrafficControlLayer::GetNDevices,
                                          &TrafficControlLayer::GetRootQueueDiscOnDeviceByIndex),
                   MakeObjectMapChecker<QueueDisc> ())

The underlying type that this ObjectMap handles is a std::map <T, U > where U is a Ptr to an ns3::Object, but these methods do not handle that type.

Here is how it is being used (note, I don't know whether this code is even working properly):

Config::ConnectWithoutContext ("/NodeList/2/$ns3::TrafficControlLayer/RootQueueDiscList/0/$ns3::CoDelQueueDisc/DropState", MakeBoundCallback (&DroppingStateTracer, stream));

I think what is needed here is ObjectVectorValue such as how NodeList is done.  It seems to me that the vector that you want here is that RootQueueDiscList is a std::vector<Ptr<QueueDisc> > indexed by NetDevice index, so you probably need to create and maintain that member variable and then clone the approach done for NodeList.
Comment 4 Stefano Avallone 2016-06-24 13:32:19 UTC
Created attachment 2484 [details]
Fix for AttributeIterator

There are actually two bugs in my opinion.

One is in AttributeIterator::DoIterate when handling an attribute that is an object container:

              NS_LOG_DEBUG ("ObjectPtrContainer attribute " << info.name);
              ObjectPtrContainerValue vector;
              object->GetAttribute (info.name, vector);
              StartVisitArrayAttribute (object, info.name, vector);
              ObjectPtrContainerValue::Iterator it;
              for (it = vector.Begin (); it != vector.End (); ++it)
                {
                  uint32_t j = (*it).first;
                  NS_LOG_DEBUG ("ObjectPtrContainer attribute item " << j);
                  Ptr<Object> tmp = (*it).second;
-->          StartVisitArrayItem (vector, j, tmp);
                  m_examined.push_back (object);
                  DoIterate (tmp);
                  m_examined.pop_back ();
                  EndVisitArrayItem ();
                }
              EndVisitArrayAttribute ();
              continue;

If an attribute is a container of Ptr<Object>, I think it is perfectly fine that one item may be a null pointer. In such a case, tmp is null and StartVisitArrayItem segfaults when doing item->GetInstanceTypeId (). This case is triggered by RootQueueDiscList when no queue disc is installed on a netdevice (see next comment).

I attach a simple fix, which I verified avoids that the script attached by Tommaso crashes.
Comment 5 Stefano Avallone 2016-06-24 13:56:45 UTC
Created attachment 2485 [details]
Fix for traffic control

The second bug is in the traffic control. I believe the definition of the accessor functions is correct:

template <typename T, typename U, typename INDEX>
Ptr<const AttributeAccessor>
MakeObjectMapAccessor (Ptr<U> (T::*get)(INDEX) const,
                          INDEX (T::*getN)(void) const);

In TrafficControlLayer, we have:

Ptr<QueueDisc> GetRootQueueDiscOnDeviceByIndex (uint32_t index) const;
uint32_t GetNDevices (void) const;

thus:

T = TrafficControlLayer
U = QueueDisc
INDEX = uint32_t

My idea was to have a map<Ptr<NetDevice>, NetDeviceInfo> (which is TrafficControlLayer::m_netDevices) so that TrafficControlLayer::Send (Ptr<NetDevice> device, Ptr<QueueDiscItem> item) can directly use device to access the map and find the queue disc where the item must be enqueued. On the other hand, I wanted to allow users to access the traced attributes of queue discs by using the config system:

  Config::ConnectWithoutContext ("/NodeList/1/$ns3::TrafficControlLayer/RootQueueDiscList/0/PacketsInQueue", MakeCallback (&TcPacketsInQueueTrace));

Here, the user provides the id of a netdevice (its id in NodeList) and gets the root queue disc installed on that device. This is done by means of the RootQueueDiscList attribute, which therefore must be an object container storing m_node->GetNDevices () items. The i-th item stores the root queue disc installed on the i-th (as in NodeList) device. This is correctly done:

Ptr<QueueDisc>
TrafficControlLayer::GetRootQueueDiscOnDeviceByIndex (uint32_t index) const
{
  NS_LOG_FUNCTION (this << index);
  return GetRootQueueDiscOnDevice (m_node->GetDevice (index));
}

However, the GetNDevices () was not correct:

uint32_t
TrafficControlLayer::GetNDevices (void) const
{
  return m_netDevices.size();
}

m_node->GetNDevices () must be returned instead (m_netDevices.size () is smaller than m_node->GetNDevices () when a queue disc is not installed on some netdevice, e.g., loopback).

The attached patch fixes this bug. Also, it shows how the config system can be used in the traffic-control example.
Comment 6 Tom Henderson 2016-06-24 14:21:24 UTC
(In reply to Stefano Avallone from comment #5)
> Created attachment 2485 [details]
> Fix for traffic control
> 
> The second bug is in the traffic control. I believe the definition of the
> accessor functions is correct:
> 
> template <typename T, typename U, typename INDEX>
> Ptr<const AttributeAccessor>
> MakeObjectMapAccessor (Ptr<U> (T::*get)(INDEX) const,
>                           INDEX (T::*getN)(void) const);

I stand corrected; I did not read beyond the definition that the rest of ns-3 codebase is using for ObjectMapAccessor:

template <typename T, typename U>
Ptr<const AttributeAccessor>
MakeObjectMapAccessor (U T::*memberVariable);

but there is also the variant that you make use of, plus one that inverts the order of the two methods.

> 
> In TrafficControlLayer, we have:
> 
> Ptr<QueueDisc> GetRootQueueDiscOnDeviceByIndex (uint32_t index) const;
> uint32_t GetNDevices (void) const;
> 
> thus:
> 
> T = TrafficControlLayer
> U = QueueDisc
> INDEX = uint32_t
> 
> My idea was to have a map<Ptr<NetDevice>, NetDeviceInfo> (which is
> TrafficControlLayer::m_netDevices) so that TrafficControlLayer::Send
> (Ptr<NetDevice> device, Ptr<QueueDiscItem> item) can directly use device to
> access the map and find the queue disc where the item must be enqueued. On
> the other hand, I wanted to allow users to access the traced attributes of
> queue discs by using the config system:
> 
>   Config::ConnectWithoutContext
> ("/NodeList/1/$ns3::TrafficControlLayer/RootQueueDiscList/0/PacketsInQueue",
> MakeCallback (&TcPacketsInQueueTrace));
> 
> Here, the user provides the id of a netdevice (its id in NodeList) and gets
> the root queue disc installed on that device. This is done by means of the
> RootQueueDiscList attribute, which therefore must be an object container
> storing m_node->GetNDevices () items. The i-th item stores the root queue
> disc installed on the i-th (as in NodeList) device. This is correctly done:
> 
> Ptr<QueueDisc>
> TrafficControlLayer::GetRootQueueDiscOnDeviceByIndex (uint32_t index) const
> {
>   NS_LOG_FUNCTION (this << index);
>   return GetRootQueueDiscOnDevice (m_node->GetDevice (index));
> }
> 
> However, the GetNDevices () was not correct:
> 
> uint32_t
> TrafficControlLayer::GetNDevices (void) const
> {
>   return m_netDevices.size();
> }
> 
> m_node->GetNDevices () must be returned instead (m_netDevices.size () is
> smaller than m_node->GetNDevices () when a queue disc is not installed on
> some netdevice, e.g., loopback).
> 
> The attached patch fixes this bug. Also, it shows how the config system can
> be used in the traffic-control example.

Agree.
Comment 7 Stefano Avallone 2016-06-28 05:31:51 UTC
What about pushing the fix for the config-store attribute iterator? I do not know if anyone else can have a look at it (looks like there is no maintainer for the config-store module).
Comment 8 Tom Henderson 2016-06-28 17:37:24 UTC
(In reply to Stefano Avallone from comment #7)
> What about pushing the fix for the config-store attribute iterator? I do not
> know if anyone else can have a look at it (looks like there is no maintainer
> for the config-store module).

Please push it; there is no maintainer for config-store other than the project itself (Mathieu said a while back that he did not want to maintain it further).  I agree with the patch and the rationale for it.
Comment 9 Stefano Avallone 2016-06-29 12:32:13 UTC
Pushed the proposed fixes with changesets 12171 and 12172.