Created an attachment (id=251) [details]
fix

If there is no comment I will push by the end of the day.

Looking carefully at the code, there is something obviously wrong with it. For
example, the following is completely braindead and there are many instances of
it.

memset (m_data, 0, m_len);
memcpy (m_data, buffer, m_len);

So, there is something fishy in this code (and yes, it's all mine so, it's all
my fault) and we need to look at it carefully rather than just get rid of the
compiler warning mindlessly. We should figure out what was the intent of the
code.

This has the "signature" of a replace-string or sed type of error.  It seems
that the memset is trying to clear the data buffer and the memcpy is trying to
copy the new (possibly fewer) bits in.

Now, 

  memset (m_data, 0, sizeof(m_data));
  memcpy (m_data, buffer, m_len);

would make a whole lot more sense.  As Mathieu observes, though, was something
else going on, or did sizeof(m_data) or the equivalent just get caught up in a
sed script?

It looks like copy-paste type of error to me.  Maybe you started by trying to
enforce a good practice of initializing all of your memory (which was fine when
m_len > 0 and you have no data to copy).  Then the cases came up where you DID
have data to copy, and a few copy and pastes later, that practice becomes
erroneous and we have the current code.

The alternative to the "just get rid of the compiler warning" solution then is
remove all of these superfluous memsets.

Craig I'm not sure I follow your comment...isn't sizeof(m_data) always a
constant, the size of a pointer on your architecture (probably 4 bytes)?

Scratch that last comment Craig, I had a brain fart :-)

(In reply to comment #3)

>   memset (m_data, 0, sizeof(m_data));
>   memcpy (m_data, buffer, m_len);

I had something similar in mind with sizeof (m_data) replaced with MAX_LEN but
I have not spent enough time looking at the code to figure out whether it would
make sense.

Heh, I can see myself making exactly this error very clearly.

Given, 

  NS_ASSERT (m_len <= MAX_SIZE);
  memset (m_data, 0, sizeof(m_data));
  memcpy (m_data, address.m_data, m_len);

I might think to myself, why use sizeof(m_data) when I use MAX_SIZE on the line
before.  I'll just change it to make it clearer.

s/sizeof(m_data)/

*ring*  *ring* [conversation about buffer overflows and lengths]

Where was I?  Oh, about m_len and and that sizeof ...

s/sizeof(m_data)/m_len/g

:-)

The underlying issue is that the code in address.cc has redundant bit sets and
has done this since the beginning of time (wrt this file).  However it is
generally harmless.  The new GCC optimizer in 4.3.2 as configured by Ubuntu
8.10 seems to try and optimize one of the cases out and complains that it
doesn't make sense to try and zero out zero bytes (which it doesn't).

At this point, however, the cost/benefit analysis may tend to recommend that it
is more risky to change it now and possibly uncover other strange dependencies
than to try and make the optimizations turned on in alpha 5 of Ubuntu Insane
Ibix work (vanill GCC 4.3.2 does work fine according to ML).

We can chance it, or we can turn this bug down to P3 and add a release note
saying that there is an optimized build issue with Ubuntu 8.10

After all of the recent chaos, I tend to agree with ML -- turn it down to P3
and don't take any more chances.

(In reply to comment #8)
> The underlying issue is that the code in address.cc has redundant bit sets and
> has done this since the beginning of time (wrt this file).  However it is
> generally harmless.  The new GCC optimizer in 4.3.2 as configured by Ubuntu
> 8.10 seems to try and optimize one of the cases out and complains that it
> doesn't make sense to try and zero out zero bytes (which it doesn't).
> 

Yes, something like this.  Upon looking into the Ubuntu string.h header, there
is in fact an additional warning attribute that says something along the lines
of "check to make sure parameter such and such is not zero".  I haven't looked,
but I am guessing that other implementations don't have this warning, and I'm
at a loss for why only opt builds trigger the warning...

> At this point, however, the cost/benefit analysis may tend to recommend that it
> is more risky to change it now and possibly uncover other strange dependencies
> than to try and make the optimizations turned on in alpha 5 of Ubuntu Insane
> Ibix work (vanill GCC 4.3.2 does work fine according to ML).

I would think that if we somehow broke the Address class, we would see all
sorts of changes in our traces, i.e. regression failures.  The code as written
is also clearly redundant but harmless.  That said...

> We can chance it, or we can turn this bug down to P3 and add a release note
> saying that there is an optimized build issue with Ubuntu 8.10
> 
> After all of the recent chaos, I tend to agree with ML -- turn it down to P3
> and don't take any more chances.
> 

I would be fine with this, provided the issue is fixed before ns-2.3.

erm, ns-3.3

Fix in 3.3 

Verified fix on ns-intrepid-ubuntu