Bug 238

Created attachment 184 [details]
detect bug

Created attachment 186 [details]
add a non-violable assertion AND fix the issue

I'm not sure patch 184 will correctly detect the issue.  Once we break into "bad" territory in this loop (after the else is hit for the first time and doesn't break the loop), it will only enter the else repeatedly, making the assertion added in the if never get hit.

Anyway, I think correct thing to do is add an assertion on the fundamental condition which cannot be violated by this code, and that is that this loop shouldn't continue to tack on data to the return packet beyond the maxSize parameter.  Adding this assertion would detect the issue. 

Beyond detection, as the writer of this code, I know I made a mistake and missed a break statement.  This is easy to correct.  I don't see the need to "document" a what's tantamount to a typo...unless a bug is suspected in the else block in question.  Is this the case?

Mathieu's patch attempting to detect but not fix the issue indicates that he is reluctant to check in the fix without a test case.  This bug's P1 status would then mean that until a test is provided, ns3.1 is deferred?  This seems like an awful lot of hassle over a typo I checked in.  I propose an added assertion and fix be checked in simultaneously, and ASAP.

(In reply to comment #2)
> Created an attachment (id=186) [edit]
> add a non-violable assertion AND fix the issue
> 
> I'm not sure patch 184 will correctly detect the issue.  Once we break into
> "bad" territory in this loop (after the else is hit for the first time and
> doesn't break the loop), it will only enter the else repeatedly, making the
> assertion added in the if never get hit.
> 
> Anyway, I think correct thing to do is add an assertion on the fundamental
> condition which cannot be violated by this code, and that is that this loop
> shouldn't continue to tack on data to the return packet beyond the maxSize
> parameter.  Adding this assertion would detect the issue. 
> 
> Beyond detection, as the writer of this code, I know I made a mistake and
> missed a break statement.  This is easy to correct.  I don't see the need to
> "document" a what's tantamount to a typo...unless a bug is suspected in the
> else block in question.  Is this the case?
> 
> Mathieu's patch attempting to detect but not fix the issue indicates that he is
> reluctant to check in the fix without a test case.  This bug's P1 status would
> then mean that until a test is provided, ns3.1 is deferred?  This seems like an
> awful lot of hassle over a typo I checked in.  I propose an added assertion and
> fix be checked in simultaneously, and ASAP.

This is your call but, again, I think that if you really understand this code, you should be able to demonstrate a testcase which hits this bug.

After very carefully reviewing this code with a test case, there is no crash when this code is hit, and no logical mistake whatsoever.  In other words, the code that is checked in does exactly what it is supposed to do, although it is not obvious, and I forgot the subtleties of how I intended it to work.  The logic in TcpSocketImpl::Recv may seem convoluted, but it is doing the right thing without the break.  Adding the break here would actually cause m_rxAvailable to be incorrect when this method exits, which you can verify for yourself.  I can explain the code in more detail if requested.

Test case at: 
http://code.nsnam.org/raj/ns-3-dev-http/
./waf --run=simple-http

This code is very non-optimal anyway in terms of efficiency, so it needs a reworking eventually; hopefully it will become more understandable at that time.

Shall I mark this bug as INVALID and close it?

looks like we should, yes.