Data corruption from #93 still relevant #186
Description
Data corruption from #93 still relevant and situation worse than it looks.
if (tlen <= sbspace(&so->so_rcv)) {
if (th->th_seq == tp->rcv_nxt &&
OFP_LIST_EMPTY(&tp->t_segq) &&
TCPS_HAVEESTABLISHED(tp->t_state)) {
if (DELAY_ACK(tp))
t_flags_or(tp->t_flags, TF_DELACK);
else
t_flags_or(tp->t_flags, TF_ACKNOW);
tp->rcv_nxt += tlen;
thflags = th->th_flags & OFP_TH_FIN;
TCPSTAT_INC(tcps_rcvpack);
TCPSTAT_ADD(tcps_rcvbyte, tlen);
ND6_HINT(tp);
SOCKBUF_LOCK(&so->so_rcv);
if (so->so_rcv.sb_state & SBS_CANTRCVMORE)
odp_packet_free(m);
else
ofp_sbappendstream_locked(&so->so_rcv,
m);
/* NB: sorwakeup_locked() does an implicit unlock. */
sorwakeup_locked(so);
} else {
/*
* XXX: Due to the header drop above "th" is
* theoretically invalid by now. Fortunately
* m_adj() doesn't actually frees any mbufs
* when trimming from the head.
*/
thflags = ofp_tcp_reass(tp, th, &tlen, m);
t_flags_or(tp->t_flags, TF_ACKNOW);
}
There are few moment:
First, ofp_sbappendstream_locked can drop packet. So tcp controll block shouldn't be updated before ofp_sbappendstream_locked called and return value checked. Currently it doesn't return anything, but int ofp_sockbuf_put_last can return -1 in case of error and why it not passed here is a big mistery.
Next, this implementation will call ofp_tcp_reass even if there are space only for 1 packet. But this function can try to add a lot more packets than 1. And it will try to do this with ofp_sbappendstream_locked, which will drop all that packets with succesfull updating of tcp controll block. So check of socket buffer should be added to ofp_tcp_reass too.