Bug 1084

Summary: MgtProbeRequestHeader::SetSupportedRates: operator= missing?
Product: ns-3 Reporter: Nicola Baldo <nicola>
Component: wifiAssignee: sebastien.deronne
Status: PATCH PENDING ---    
Severity: normal CC: deanarm, john.abraham.in, mathieu.lacage, ns-bugs, tomh
Priority: P3    
Version: ns-3-dev   
Hardware: All   
OS: All   
Attachments: First draft of a proposed fix for this bug

Description Nicola Baldo 2011-03-28 12:21:17 EDT
Reported from John Abraham on ns-developers:

Hi,

While I was porting

void 
MgtProbeRequestHeader::SetSupportedRates (SupportedRates rates)
{
  m_rates = rates;
}

To Visual C++ I found, I needed to overload "operator=" ,something like

SupportedRates & SupportedRates::operator= (const SupportedRates & sr)
{
  this->m_nRates = sr.m_nRates ;
  for (int i = 0 ; i < MAX_SUPPORTED_RATES ; i++) 
  {
         this->m_rates[i] = sr.m_rates[i];
  }
  this->extended = ExtendedSupportedRatesIE (this);
  return *this;
}

Particularly, once SetSupportedRates is popped , "rates" being out of scope,
may leave 
m_rates.extended->m_supportedRates having a pointer to an out of scope
object.

Can someone confirm this theory, because this code works ok on g++ but not
on Visual C++?
Comment 1 Nicola Baldo 2011-03-28 12:21:57 EDT
Comment posted by Mathieu on ns-developers:

Note that I did not try to really investigate this issue but my 10
second look at the API and your description of the problem hints at
the following...

On Thu, Mar 17, 2011 at 13:33, John Abraham <jabraham3@mail.gatech.edu> wrote:

> > Particularly, once SetSupportedRates is popped , "rates" being out of scope,
> > may leave
> >
> > m_rates.extended->m_supportedRates having a pointer to an out of scope
> > object.
> >
> >
> >
> > Can someone confirm this theory, because this code works ok on g++ but not
> > on Visual C++?
Probably just luck in the way gcc allocates stack memory.

The problem here seems to be just bad API, not a missing operator =
that should have been generated by your compiler.

Bad API in this header:

class ExtendedSupportedRatesIE : public WifiInformationElement {
public:
  ExtendedSupportedRatesIE (SupportedRates *rates);

private:
  SupportedRates *m_supportedRates;

};

The above transfers ownership from the caller to the callee which is
going to be the source of subtle bugs such as the one you found (?).
It should be instead:

class ExtendedSupportedRatesIE : public WifiInformationElement {
public:
  ExtendedSupportedRatesIE (const SupportedRates *rates)
    : WifiInformationElement (),
      m_rates (*rates)
  {}

private:
  SupportedRates m_supportedRates;
};

It seems that people like to inflict pain on others of the kind you
just suffered, I don't understand why.

(lengthy "I told you so" rant elided because I did not read the code
carefully enough to be sure this is really the problem)
Comment 2 Nicola Baldo 2011-03-28 12:23:38 EDT
Dean, I am CCing you as you are the author of the ExtendedSupportedRatesIE code. Could you please give a look?
Comment 3 Nicola Baldo 2011-04-10 15:07:24 EDT
I agree with John that after the assignment is performed m_rates.extended.m_supportedRates still points at rates, while it should point at m_rates. Hence I think that the assignment operator should be overloaded (unless we change the design).

The solution proposed by Mathieu won't work because SupportedRates has a member ExtendedSupportedRatesIE, and at the same time the latter has a member which is a pointer to the former. Turning the pointer into a complete object introduces a circular dependency. 

As for the "bad API design" argument, actually ExtendedSupportedRatesIE is only used as a member of SupportedRates. Mathieu, does this change your opinion on "ownership transfer"?
Comment 4 Mathieu Lacage 2011-05-16 08:10:59 EDT
(In reply to comment #3)
> I agree with John that after the assignment is performed
> m_rates.extended.m_supportedRates still points at rates, while it should point
> at m_rates. Hence I think that the assignment operator should be overloaded
> (unless we change the design).

I see 3 horrible things: 

1) a member pointer should not point to an object on which you have no ownership. i.e. you must not transfer ownership in the API.

2) there is a circular dependency ?

3) data members that are public ???

4) friend ?

All 4 items are symptoms of bad design... That is a lot...

> 
> The solution proposed by Mathieu won't work because SupportedRates has a member
> ExtendedSupportedRatesIE, and at the same time the latter has a member which is
> a pointer to the former. Turning the pointer into a complete object introduces
> a circular dependency. 
> 
> As for the "bad API design" argument, actually ExtendedSupportedRatesIE is only
> used as a member of SupportedRates. Mathieu, does this change your opinion on
> "ownership transfer"?

No: looking at the code more re-inforces my opinion. My suggestion is that you need to decouple the data structure that holds the actual data from the classes that are used for serialization/deserialization.
Comment 5 Dean Armstrong 2011-07-03 21:25:25 EDT
Created attachment 1184 [details]
First draft of a proposed fix for this bug

I've attempted to address this bug in the attached patch by taking the approach Mathieu suggested of trying to separate storage of the supported rates from the Information Element classes.

I think I've addressed Mathieu's (2) and (3) from comment #4 - there is no longer a circular dependency, nor are there any public data members. 

The new SupportedRatesIE and ExtendedSupportedRatesIE classes are friends of SupportedRates, so if friend classes are bad (Mathieu's item (4); regrettably I need to be educated on the reasons) then I could extend the SupportedRates interface so that the requirements of the SupportedRatesIE and ExtendedSupportedRatesIE classes can be met through the SupportedRates public API. If this is preferred it isn't a major job.

With regard to Mathieu's item (1), there's still a pointer to a SupportedRates object being passed in and stored on construction of the SupportedRatesIE and ExtendedSupportedRatesIE objects. I want to argue that this is okay because these objects are not being used properly if they are being copied and, in fact, their lifespan should be very short - they are intended to be created in order to serialise or deserialise the relevant IE, and destroyed thereafter. There is, of course, nothing that enforces this usage, and I suppose this approach may be contentious.

In general, I think the cleanliness of this all is constrained by the fact that the WifiInformationElement API was not obviously designed to support storage and (de)serialisation related to IEs being split across multiple classes.

I'd appreciate comment on my patch and on alternate approaches from any and all who have opinions on the cleanest way to do this (especially you two, Mathieu and Nicola).
Comment 6 sebastien.deronne 2015-05-27 10:12:57 EDT
Could Nicola and Mathieu provide a feedback on the provided patch? Thanks!
Comment 7 Tom Henderson 2020-04-20 09:47:05 EDT
The original issue of lack of operator= was addressed in commit 63c9018c81.

The other issues raised by Mathieu on API, and discussed also by Dean, are not addressed yet.