After talked with Siyuan, we agree to use the int 0-1 value instead of Boolean type so as to keep the same coding style in TcpInput.c.
Thanks, Jiaxin > -----Original Message----- > From: Fu, Siyuan > Sent: Wednesday, December 27, 2017 11:03 AM > To: Wu, Jiaxin <jiaxin...@intel.com>; edk2-devel@lists.01.org > Cc: Ye, Ting <ting...@intel.com>; Wang, Fan <fan.w...@intel.com> > Subject: RE: [Patch 3/3] NetworkPkg/TcpDxe: Check TCP payload for release > version. > > Hi, Jiaxin > > I think it's better to return a Boolean type than int 0-1 value in > TcpTrimSegment(). > Other part of good to me. > > > Reviewed-by: Fu Siyuan <siyuan...@intel.com> > > > > > -----Original Message----- > > From: Wu, Jiaxin > > Sent: Tuesday, December 26, 2017 2:50 PM > > To: edk2-devel@lists.01.org > > Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Wang, > > Fan <fan.w...@intel.com>; Wu, Jiaxin <jiaxin...@intel.com> > > Subject: [Patch 3/3] NetworkPkg/TcpDxe: Check TCP payload for release > > version. > > > > TCP payload check is implemented by TcpVerifySegment(), but all the > > function > > calls of TcpVerifySegment() are placed in ASSERT(), which is only valid > > for > > debug version: > > ASSERT (TcpVerifySegment (Nbuf) != 0); > > > > This patch is to enable the check for release version. > > > > Cc: Ye Ting <ting...@intel.com> > > Cc: Fu Siyuan <siyuan...@intel.com> > > Cc: Wang Fan <fan.w...@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Wu Jiaxin <jiaxin...@intel.com> > > --- > > NetworkPkg/TcpDxe/TcpInput.c | 125 > +++++++++++++++++++++++++++++++++---- > > ----- > > NetworkPkg/TcpDxe/TcpOutput.c | 29 +++++++--- > > 2 files changed, 122 insertions(+), 32 deletions(-) > > > > diff --git a/NetworkPkg/TcpDxe/TcpInput.c > b/NetworkPkg/TcpDxe/TcpInput.c > > index f8845dc..92a0ab8 100644 > > --- a/NetworkPkg/TcpDxe/TcpInput.c > > +++ b/NetworkPkg/TcpDxe/TcpInput.c > > @@ -279,12 +279,15 @@ TcpComputeRtt ( > > > > @param[in] Nbuf The buffer that contains a received TCP segment > > without an IP header. > > @param[in] Left The sequence number of the window's left edge. > > @param[in] Right The sequence number of the window's right edge. > > > > + @retval 0 The segment is broken. > > + @retval 1 The segment is in good shape. > > + > > **/ > > -VOID > > +INTN > > TcpTrimSegment ( > > IN NET_BUF *Nbuf, > > IN TCP_SEQNO Left, > > IN TCP_SEQNO Right > > ) > > @@ -304,11 +307,11 @@ TcpTrimSegment ( > > TCP_CLEAR_FLG (Seg->Flag, TCP_FLG_SYN); > > TCP_CLEAR_FLG (Seg->Flag, TCP_FLG_FIN); > > > > Seg->Seq = Seg->End; > > NetbufTrim (Nbuf, Nbuf->TotalSize, NET_BUF_HEAD); > > - return; > > + return 1; > > } > > > > // > > // Adjust the buffer header > > // > > @@ -357,27 +360,30 @@ TcpTrimSegment ( > > if (Drop != 0) { > > NetbufTrim (Nbuf, Drop, NET_BUF_TAIL); > > } > > } > > > > - ASSERT (TcpVerifySegment (Nbuf) != 0); > > + return TcpVerifySegment (Nbuf); > > } > > > > /** > > Trim off the data outside the tcb's receive window. > > > > @param[in] Tcb Pointer to the TCP_CB of this TCP instance. > > @param[in] Nbuf Pointer to the NET_BUF containing the received tcp > > segment. > > > > + @retval 0 The segment is broken. > > + @retval 1 The segment is in good shape. > > + > > **/ > > -VOID > > +INTN > > TcpTrimInWnd ( > > IN TCP_CB *Tcb, > > IN NET_BUF *Nbuf > > ) > > { > > - TcpTrimSegment (Nbuf, Tcb->RcvNxt, Tcb->RcvWl2 + Tcb->RcvWnd); > > + return TcpTrimSegment (Nbuf, Tcb->RcvNxt, Tcb->RcvWl2 + Tcb- > >RcvWnd); > > } > > > > /** > > Process the data and FIN flag, and check whether to deliver > > data to the socket layer. > > @@ -419,11 +425,20 @@ TcpDeliverData ( > > > > while (Entry != &Tcb->RcvQue) { > > Nbuf = NET_LIST_USER_STRUCT (Entry, NET_BUF, List); > > Seg = TCPSEG_NETBUF (Nbuf); > > > > - ASSERT (TcpVerifySegment (Nbuf) != 0); > > + if (TcpVerifySegment (Nbuf) == 0) { > > + DEBUG ( > > + (EFI_D_ERROR, > > + "TcpToSendData: discard a broken segment for TCB %p\n", > > + Tcb) > > + ); > > + NetbufFree (Nbuf); > > + return -1; > > + } > > + > > ASSERT (Nbuf->Tcp == NULL); > > > > if (TCP_SEQ_GT (Seg->Seq, Seq)) { > > break; > > } > > @@ -559,12 +574,15 @@ TcpDeliverData ( > > Store the data into the reassemble queue. > > > > @param[in, out] Tcb Pointer to the TCP_CB of this TCP instance. > > @param[in] Nbuf Pointer to the buffer containing the data to be > > queued. > > > > + @retval 0 An error condition occurred. > > + @retval 1 No error occurred to queue data. > > + > > **/ > > -VOID > > +INTN > > TcpQueueData ( > > IN OUT TCP_CB *Tcb, > > IN NET_BUF *Nbuf > > ) > > { > > @@ -586,11 +604,11 @@ TcpQueueData ( > > // no out-of-order segments are received. > > // > > if (IsListEmpty (Head)) { > > > > InsertTailList (Head, &Nbuf->List); > > - return; > > + return 1; > > } > > > > // > > // Find the point to insert the buffer > > // > > @@ -613,16 +631,16 @@ TcpQueueData ( > > Node = NET_LIST_USER_STRUCT (Prev, NET_BUF, List); > > > > if (TCP_SEQ_LT (Seg->Seq, TCPSEG_NETBUF (Node)->End)) { > > > > if (TCP_SEQ_LEQ (Seg->End, TCPSEG_NETBUF (Node)->End)) { > > - > > - NetbufFree (Nbuf); > > - return; > > + return 1; > > } > > > > - TcpTrimSegment (Nbuf, TCPSEG_NETBUF (Node)->End, Seg->End); > > + if (TcpTrimSegment (Nbuf, TCPSEG_NETBUF (Node)->End, Seg->End) > == 0) > > { > > + return 0; > > + } > > } > > } > > > > InsertHeadList (Prev, &Nbuf->List); > > > > @@ -646,31 +664,38 @@ TcpQueueData ( > > if (TCP_SEQ_LT (TCPSEG_NETBUF (Node)->Seq, Seg->End)) { > > > > if (TCP_SEQ_LEQ (TCPSEG_NETBUF (Node)->Seq, Seg->Seq)) { > > > > RemoveEntryList (&Nbuf->List); > > - NetbufFree (Nbuf); > > - return; > > + return 1; > > } > > > > - TcpTrimSegment (Nbuf, Seg->Seq, TCPSEG_NETBUF (Node)->Seq); > > + if (TcpTrimSegment (Nbuf, Seg->Seq, TCPSEG_NETBUF (Node)->Seq) > == 0) > > { > > + RemoveEntryList (&Nbuf->List); > > + return 0; > > + } > > break; > > } > > > > Cur = Cur->ForwardLink; > > } > > + > > + return 1; > > } > > > > > > /** > > Adjust the send queue or the retransmit queue. > > > > @param[in] Tcb Pointer to the TCP_CB of this TCP instance. > > @param[in] Ack The acknowledge seuqence number of the received > > segment. > > > > + @retval 0 An error condition occurred. > > + @retval 1 No error occurred. > > + > > **/ > > -VOID > > +INTN > > TcpAdjustSndQue ( > > IN TCP_CB *Tcb, > > IN TCP_SEQNO Ack > > ) > > { > > @@ -699,13 +724,14 @@ TcpAdjustSndQue ( > > RemoveEntryList (&Node->List); > > NetbufFree (Node); > > continue; > > } > > > > - TcpTrimSegment (Node, Ack, Seg->End); > > - break; > > + return TcpTrimSegment (Node, Ack, Seg->End); > > } > > + > > + return 1; > > } > > > > /** > > Process the received TCP segments. > > > > @@ -891,11 +917,19 @@ TcpInput ( > > TcpInitTcbLocal (Tcb); > > TcpInitTcbPeer (Tcb, Seg, &Option); > > > > TcpSetState (Tcb, TCP_SYN_RCVD); > > TcpSetTimer (Tcb, TCP_TIMER_CONNECT, Tcb->ConnectTimeout); > > - TcpTrimInWnd (Tcb, Nbuf); > > + if (TcpTrimInWnd (Tcb, Nbuf) == 0) { > > + DEBUG ( > > + (EFI_D_ERROR, > > + "TcpInput: discard a broken segment for TCB %p\n", > > + Tcb) > > + ); > > + > > + goto DISCARD; > > + } > > > > goto StepSix; > > } > > > > goto DISCARD; > > @@ -973,11 +1007,19 @@ TcpInput ( > > > > TcpComputeRtt (Tcb, Tcb->RttMeasure); > > TCP_CLEAR_FLG (Tcb->CtrlFlag, TCP_CTRL_RTT_ON); > > } > > > > - TcpTrimInWnd (Tcb, Nbuf); > > + if (TcpTrimInWnd (Tcb, Nbuf) == 0) { > > + DEBUG ( > > + (EFI_D_ERROR, > > + "TcpInput: discard a broken segment for TCB %p\n", > > + Tcb) > > + ); > > + > > + goto DISCARD; > > + } > > > > TCP_SET_FLG (Tcb->CtrlFlag, TCP_CTRL_ACK_NOW); > > > > DEBUG ( > > (EFI_D_NET, > > @@ -991,13 +1033,20 @@ TcpInput ( > > // Received a SYN segment without ACK, simultanous open. > > // > > TcpSetState (Tcb, TCP_SYN_RCVD); > > > > ASSERT (Tcb->SndNxt == Tcb->Iss + 1); > > - TcpAdjustSndQue (Tcb, Tcb->SndNxt); > > > > - TcpTrimInWnd (Tcb, Nbuf); > > + if (TcpAdjustSndQue (Tcb, Tcb->SndNxt) == 0 || TcpTrimInWnd (Tcb, > > Nbuf) == 0) { > > + DEBUG ( > > + (EFI_D_ERROR, > > + "TcpInput: discard a broken segment for TCB %p\n", > > + Tcb) > > + ); > > + > > + goto DISCARD; > > + } > > > > DEBUG ( > > (EFI_D_WARN, > > "TcpInput: simultaneous open for TCB %p in SYN_SENT\n", > > Tcb) > > @@ -1079,11 +1128,19 @@ TcpInput ( > > } > > > > // > > // Trim the data and flags. > > // > > - TcpTrimInWnd (Tcb, Nbuf); > > + if (TcpTrimInWnd (Tcb, Nbuf) == 0) { > > + DEBUG ( > > + (EFI_D_ERROR, > > + "TcpInput: discard a broken segment for TCB %p\n", > > + Tcb) > > + ); > > + > > + goto DISCARD; > > + } > > > > // > > // Third step: Check security and precedence, Ignored > > // > > > > @@ -1254,11 +1311,20 @@ TcpInput ( > > TcpFastRecover (Tcb, Seg); > > } > > > > if (TCP_SEQ_GT (Seg->Ack, Tcb->SndUna)) { > > > > - TcpAdjustSndQue (Tcb, Seg->Ack); > > + if (TcpAdjustSndQue (Tcb, Seg->Ack) == 0) { > > + DEBUG ( > > + (EFI_D_ERROR, > > + "TcpInput: discard a broken segment for TCB %p\n", > > + Tcb) > > + ); > > + > > + goto DISCARD; > > + } > > + > > Tcb->SndUna = Seg->Ack; > > > > if (TCP_FLG_ON (Tcb->CtrlFlag, TCP_CTRL_SND_URG) && > > TCP_SEQ_LT (Tcb->SndUp, Seg->Ack)) > > { > > @@ -1487,11 +1553,20 @@ StepSix: > > ); > > > > goto RESET_THEN_DROP; > > } > > > > - TcpQueueData (Tcb, Nbuf); > > + if (TcpQueueData (Tcb, Nbuf) == 0) { > > + DEBUG ( > > + (EFI_D_ERROR, > > + "TcpInput: discard a broken segment for TCB %p\n", > > + Tcb) > > + ); > > + > > + goto DISCARD; > > + } > > + > > if (TcpDeliverData (Tcb) == -1) { > > goto RESET_THEN_DROP; > > } > > > > if (!IsListEmpty (&Tcb->RcvQue)) { > > diff --git a/NetworkPkg/TcpDxe/TcpOutput.c > b/NetworkPkg/TcpDxe/TcpOutput.c > > index a7e59f0..1697514 100644 > > --- a/NetworkPkg/TcpDxe/TcpOutput.c > > +++ b/NetworkPkg/TcpDxe/TcpOutput.c > > @@ -290,11 +290,15 @@ TcpTransmitSegment ( > > TCP_HEAD *Head; > > TCP_SEG *Seg; > > BOOLEAN Syn; > > UINT32 DataLen; > > > > - ASSERT ((Nbuf != NULL) && (Nbuf->Tcp == NULL) && (TcpVerifySegment > > (Nbuf) != 0)); > > + ASSERT ((Nbuf != NULL) && (Nbuf->Tcp == NULL)); > > + > > + if (TcpVerifySegment (Nbuf) == 0) { > > + return -1; > > + } > > > > DataLen = Nbuf->TotalSize; > > > > Seg = TCPSEG_NETBUF (Nbuf); > > Syn = TCP_FLG_ON (Seg->Flag, TCP_FLG_SYN); > > @@ -632,11 +636,15 @@ TcpGetSegment ( > > } else { > > > > Nbuf = TcpGetSegmentSock (Tcb, Seq, Len); > > } > > > > - ASSERT (TcpVerifySegment (Nbuf) != 0); > > + if (TcpVerifySegment (Nbuf) == 0) { > > + NetbufFree (Nbuf); > > + return NULL; > > + } > > + > > return Nbuf; > > } > > > > /** > > Retransmit the segment from sequence Seq. > > @@ -699,11 +707,13 @@ TcpRetransmit ( > > Nbuf = TcpGetSegmentSndQue (Tcb, Seq, Len); > > if (Nbuf == NULL) { > > return -1; > > } > > > > - ASSERT (TcpVerifySegment (Nbuf) != 0); > > + if (TcpVerifySegment (Nbuf) == 0) { > > + goto OnError; > > + } > > > > if (TcpTransmitSegment (Tcb, Nbuf) != 0) { > > goto OnError; > > } > > > > @@ -884,12 +894,18 @@ TcpToSendData ( > > > > Seg->Seq = Seq; > > Seg->End = End; > > Seg->Flag = Flag; > > > > - ASSERT (TcpVerifySegment (Nbuf) != 0); > > - ASSERT (TcpCheckSndQue (&Tcb->SndQue) != 0); > > + if (TcpVerifySegment (Nbuf) == 0 || TcpCheckSndQue (&Tcb->SndQue) > == > > 0) { > > + DEBUG ( > > + (EFI_D_ERROR, > > + "TcpToSendData: discard a broken segment for TCB %p\n", > > + Tcb) > > + ); > > + goto OnError; > > + } > > > > // > > // Don't send an empty segment here. > > // > > if (Seg->End == Seg->Seq) { > > @@ -897,12 +913,11 @@ TcpToSendData ( > > (EFI_D_WARN, > > "TcpToSendData: created a empty segment for TCB %p, free it > > now\n", > > Tcb) > > ); > > > > - NetbufFree (Nbuf); > > - return Sent; > > + goto OnError; > > } > > > > if (TcpTransmitSegment (Tcb, Nbuf) != 0) { > > NetbufTrim (Nbuf, (Nbuf->Tcp->HeadLen << 2), NET_BUF_HEAD); > > Nbuf->Tcp = NULL; > > -- > > 1.9.5.msysgit.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel