Created attachment 192 [details]
Fix TCP checksum crash

Here a patch to fix checksum crash with TCP.

In TcpHeader::Deserialize, we calulate the checksum and we need the payload size and the previous checksum from the pseudo-header (calculating by TcpHeader::InitializeChecksum).

(In reply to comment #1)
> Created an attachment (id=192) [details]
> Fix TCP checksum crash
> 
> Here a patch to fix checksum crash with TCP.
> 
> In TcpHeader::Deserialize, we calulate the checksum and we need the payload
> size and the previous checksum from the pseudo-header (calculating by
> TcpHeader::InitializeChecksum).

How do know the size of the tcp header before reading it (there could well be tcp options in the header)? I realize that we do the same thing for the Udp Header in UdpL4Protocol::Receive but that is not right.

I have to confess that it is not clear to me what we should do here. An option might be to add a Buffer::Iterator::GetSize method (pick a better name, please) and use that instead of setting and using the m_payloadSize member.

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=192) [details] [details]
> > Fix TCP checksum crash
> > 
> > Here a patch to fix checksum crash with TCP.
> > 
> > In TcpHeader::Deserialize, we calulate the checksum and we need the payload
> > size and the previous checksum from the pseudo-header (calculating by
> > TcpHeader::InitializeChecksum).
> 
> How do know the size of the tcp header before reading it (there could well be
> tcp options in the header)? I realize that we do the same thing for the Udp
> Header in UdpL4Protocol::Receive but that is not right.
> 
> I have to confess that it is not clear to me what we should do here. An option
> might be to add a Buffer::Iterator::GetSize method (pick a better name, please)
> and use that instead of setting and using the m_payloadSize member.

Another option, again, for TCP only, would be to change the signature of the InitializeChecksum method to take an extra argument, the size of the complete packet such that, in Deserialize, you could do the right thing without using m_payloadSize.

In either case, we should work harder to document the relatively subtle behavior of the TcpHeader, UdpHeader, and , Ipv4Header classes.

vincent, can you give feedback on my last comments ?

The TCP solution (change InitializeChecksum) is interresting and easy to do.

The UDP header is always 8 bytes (i.e. there are no UDP options in header), am I right ? So no need to change it.

But the best would be to have a unique way for UDP/TCP. With that if others want to do a new L4 Protocol (such a SCTP), they have a model to follow.

Could you explain a little bit your way to proceed with Buffer::iterator::GetSize ? I mean in the different classes (*L4Protocol, *Header).

(In reply to comment #5)
> The TCP solution (change InitializeChecksum) is interresting and easy to do.
> 
> The UDP header is always 8 bytes (i.e. there are no UDP options in header), am
> I right ? So no need to change it.

I think so, yes.

> 
> But the best would be to have a unique way for UDP/TCP. With that if others
> want to do a new L4 Protocol (such a SCTP), they have a model to follow.

yes.

> 
> Could you explain a little bit your way to proceed with
> Buffer::iterator::GetSize ? I mean in the different classes (*L4Protocol,

Well, if we provided a Buffer::Iterator::GetSize method which were to return the size of the underlying byte buffer, you could use that in ::Deserialize and ::Serialize, and potentially, get rid of the SetPayloadSize method. 

I can't make myself like this very much but I suspect that it would pretty easy to get up and running.

> *Header).
> 

Created attachment 197 [details]
Use Buffer::Iterator::Getsize

Here is a patch which implements this for TCP and UDP. Tested with udp-echo and tcp-large-transfer with both --ns3::UdpL4Protocl::CalcChecksum=true --ns3::TcpL4Protocol::CalcChecksum=true --ns3::Ipv4L3Protocol::CalcChecksum=true

changeset: b5d4a04c7b68