* [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL
@ 2019-10-12 7:43 Zhang, Shenglei
2019-10-12 7:43 ` [PATCH] NetworkPkg/DxeNetLib: Change the order of conditions in IF statement Zhang, Shenglei
2019-10-12 7:58 ` [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL Siyuan, Fu
0 siblings, 2 replies; 6+ messages in thread
From: Zhang, Shenglei @ 2019-10-12 7:43 UTC (permalink / raw)
To: devel; +Cc: Siyuan Fu, Jiaxin Wu
If DpcEntry is NULL, it means it failed to be allocated space.
ReturnStatus should be EFI_OUT_OF_RESOURCES regardless of the
content of mDpcEntryFreeList.
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
NetworkPkg/DpcDxe/Dpc.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/NetworkPkg/DpcDxe/Dpc.c b/NetworkPkg/DpcDxe/Dpc.c
index 8a490949dc8c..ebdb978b254a 100644
--- a/NetworkPkg/DpcDxe/Dpc.c
+++ b/NetworkPkg/DpcDxe/Dpc.c
@@ -137,14 +137,11 @@ DpcQueueDpc (
gBS->RaiseTPL (TPL_HIGH_LEVEL);
//
- // If the allocation of a DPC entry fails, and the free list is empty,
- // then return EFI_OUT_OF_RESOURCES.
+ // If the allocation of a DPC entry fails, return EFI_OUT_OF_RESOURCES.
//
if (DpcEntry == NULL) {
- if (IsListEmpty (&mDpcEntryFreeList)) {
- ReturnStatus = EFI_OUT_OF_RESOURCES;
- goto Done;
- }
+ ReturnStatus = EFI_OUT_OF_RESOURCES;
+ goto Done;
}
//
--
2.18.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] NetworkPkg/DxeNetLib: Change the order of conditions in IF statement
2019-10-12 7:43 [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL Zhang, Shenglei
@ 2019-10-12 7:43 ` Zhang, Shenglei
2019-10-14 2:21 ` Siyuan, Fu
2019-10-14 9:46 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-10-12 7:58 ` [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL Siyuan, Fu
1 sibling, 2 replies; 6+ messages in thread
From: Zhang, Shenglei @ 2019-10-12 7:43 UTC (permalink / raw)
To: devel; +Cc: Siyuan Fu, Jiaxin Wu
The condition, NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len, is meaningless
if Index < 0. So 'Index < 0' should be performed first in the if statement.
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
NetworkPkg/Library/DxeNetLib/NetBuffer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/NetworkPkg/Library/DxeNetLib/NetBuffer.c b/NetworkPkg/Library/DxeNetLib/NetBuffer.c
index 2408e9a10456..a35e67aa1f6c 100644
--- a/NetworkPkg/Library/DxeNetLib/NetBuffer.c
+++ b/NetworkPkg/Library/DxeNetLib/NetBuffer.c
@@ -1063,7 +1063,7 @@ NetbufAllocSpace (
} else {
NetbufGetByte (Nbuf, 0, &Index);
- if ((NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len) && (Index > 0)) {
+ if ((Index > 0) && (NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len)) {
Index--;
}
}
--
2.18.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] NetworkPkg/DxeNetLib: Change the order of conditions in IF statement
2019-10-12 7:43 ` [PATCH] NetworkPkg/DxeNetLib: Change the order of conditions in IF statement Zhang, Shenglei
@ 2019-10-14 2:21 ` Siyuan, Fu
2019-10-14 9:46 ` [edk2-devel] " Philippe Mathieu-Daudé
1 sibling, 0 replies; 6+ messages in thread
From: Siyuan, Fu @ 2019-10-14 2:21 UTC (permalink / raw)
To: Zhang, Shenglei, devel@edk2.groups.io; +Cc: Wu, Jiaxin
Reviewed-by: Siyuan Fu <siyuan.fu@intel.com>
> -----Original Message-----
> From: Zhang, Shenglei <shenglei.zhang@intel.com>
> Sent: 2019年10月12日 15:43
> To: devel@edk2.groups.io
> Cc: Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: [PATCH] NetworkPkg/DxeNetLib: Change the order of conditions in
> IF statement
>
> The condition, NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len, is
> meaningless
> if Index < 0. So 'Index < 0' should be performed first in the if statement.
>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> ---
> NetworkPkg/Library/DxeNetLib/NetBuffer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/NetworkPkg/Library/DxeNetLib/NetBuffer.c
> b/NetworkPkg/Library/DxeNetLib/NetBuffer.c
> index 2408e9a10456..a35e67aa1f6c 100644
> --- a/NetworkPkg/Library/DxeNetLib/NetBuffer.c
> +++ b/NetworkPkg/Library/DxeNetLib/NetBuffer.c
> @@ -1063,7 +1063,7 @@ NetbufAllocSpace (
> } else {
> NetbufGetByte (Nbuf, 0, &Index);
>
> - if ((NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len) && (Index > 0)) {
> + if ((Index > 0) && (NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len)) {
> Index--;
> }
> }
> --
> 2.18.0.windows.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [edk2-devel] [PATCH] NetworkPkg/DxeNetLib: Change the order of conditions in IF statement
2019-10-12 7:43 ` [PATCH] NetworkPkg/DxeNetLib: Change the order of conditions in IF statement Zhang, Shenglei
2019-10-14 2:21 ` Siyuan, Fu
@ 2019-10-14 9:46 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-14 9:46 UTC (permalink / raw)
To: devel, shenglei.zhang; +Cc: Siyuan Fu, Jiaxin Wu, Laszlo Ersek
Hi Zhang,
On 10/12/19 9:43 AM, Zhang, Shenglei wrote:
> The condition, NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len, is meaningless
> if Index < 0. So 'Index < 0' should be performed first in the if statement.
>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> ---
> NetworkPkg/Library/DxeNetLib/NetBuffer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/NetworkPkg/Library/DxeNetLib/NetBuffer.c b/NetworkPkg/Library/DxeNetLib/NetBuffer.c
> index 2408e9a10456..a35e67aa1f6c 100644
> --- a/NetworkPkg/Library/DxeNetLib/NetBuffer.c
> +++ b/NetworkPkg/Library/DxeNetLib/NetBuffer.c
> @@ -1063,7 +1063,7 @@ NetbufAllocSpace (
> } else {
> NetbufGetByte (Nbuf, 0, &Index);
>
> - if ((NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len) && (Index > 0)) {
> + if ((Index > 0) && (NET_HEADSPACE(&(Nbuf->BlockOp[Index])) < Len)) {
I'm not sure this is correct. Index is unsigned, so it won't be
negative. NetbufGetByte() can set Index=0. With your change this case is
not covered anymore.
Maybe your tool is unhappy because the return value of NetbufGetByte()
isn't checked?
> Index--;
> }
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL
2019-10-12 7:43 [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL Zhang, Shenglei
2019-10-12 7:43 ` [PATCH] NetworkPkg/DxeNetLib: Change the order of conditions in IF statement Zhang, Shenglei
@ 2019-10-12 7:58 ` Siyuan, Fu
2019-10-12 8:42 ` Zhang, Shenglei
1 sibling, 1 reply; 6+ messages in thread
From: Siyuan, Fu @ 2019-10-12 7:58 UTC (permalink / raw)
To: Zhang, Shenglei, devel@edk2.groups.io; +Cc: Wu, Jiaxin
> -----Original Message-----
> From: Zhang, Shenglei <shenglei.zhang@intel.com>
> Sent: 2019年10月12日 15:43
> To: devel@edk2.groups.io
> Cc: Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when
> DpcEntry is NULL
>
> If DpcEntry is NULL, it means it failed to be allocated space.
> ReturnStatus should be EFI_OUT_OF_RESOURCES regardless of the
> content of mDpcEntryFreeList.
>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> ---
> NetworkPkg/DpcDxe/Dpc.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/NetworkPkg/DpcDxe/Dpc.c b/NetworkPkg/DpcDxe/Dpc.c
> index 8a490949dc8c..ebdb978b254a 100644
> --- a/NetworkPkg/DpcDxe/Dpc.c
> +++ b/NetworkPkg/DpcDxe/Dpc.c
> @@ -137,14 +137,11 @@ DpcQueueDpc (
> gBS->RaiseTPL (TPL_HIGH_LEVEL);
>
> //
> - // If the allocation of a DPC entry fails, and the free list is empty,
> - // then return EFI_OUT_OF_RESOURCES.
> + // If the allocation of a DPC entry fails, return EFI_OUT_OF_RESOURCES.
> //
> if (DpcEntry == NULL) {
> - if (IsListEmpty (&mDpcEntryFreeList)) {
> - ReturnStatus = EFI_OUT_OF_RESOURCES;
> - goto Done;
> - }
> + ReturnStatus = EFI_OUT_OF_RESOURCES;
> + goto Done;
I don't think it's a correct fix. This DpcEntry allocation is inside a for loop which
tries to allocate 64 new DPC entries, if one of the allocation in the middle of
this loop failed, the mDpcEntryFreeList will contain useable entries for this
DpcQueueDpc(), that's why original code doesn't treat it as an error.
How do you find this issue? By code review or it cause some real problems?
Thanks
Siyuan
> }
>
> //
> --
> 2.18.0.windows.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL
2019-10-12 7:58 ` [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL Siyuan, Fu
@ 2019-10-12 8:42 ` Zhang, Shenglei
0 siblings, 0 replies; 6+ messages in thread
From: Zhang, Shenglei @ 2019-10-12 8:42 UTC (permalink / raw)
To: Fu, Siyuan, devel@edk2.groups.io; +Cc: Wu, Jiaxin
> -----Original Message-----
> From: Fu, Siyuan
> Sent: Saturday, October 12, 2019 3:59 PM
> To: Zhang, Shenglei <shenglei.zhang@intel.com>; devel@edk2.groups.io
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: RE: [PATCH] NetworkPkg/DpcDxe: Update the consequent logic
> when DpcEntry is NULL
>
> > -----Original Message-----
> > From: Zhang, Shenglei <shenglei.zhang@intel.com>
> > Sent: 2019年10月12日 15:43
> > To: devel@edk2.groups.io
> > Cc: Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> > Subject: [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when
> > DpcEntry is NULL
> >
> > If DpcEntry is NULL, it means it failed to be allocated space.
> > ReturnStatus should be EFI_OUT_OF_RESOURCES regardless of the
> > content of mDpcEntryFreeList.
> >
> > Cc: Siyuan Fu <siyuan.fu@intel.com>
> > Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> > Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> > ---
> > NetworkPkg/DpcDxe/Dpc.c | 9 +++------
> > 1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/NetworkPkg/DpcDxe/Dpc.c b/NetworkPkg/DpcDxe/Dpc.c
> > index 8a490949dc8c..ebdb978b254a 100644
> > --- a/NetworkPkg/DpcDxe/Dpc.c
> > +++ b/NetworkPkg/DpcDxe/Dpc.c
> > @@ -137,14 +137,11 @@ DpcQueueDpc (
> > gBS->RaiseTPL (TPL_HIGH_LEVEL);
> >
> > //
> > - // If the allocation of a DPC entry fails, and the free list is empty,
> > - // then return EFI_OUT_OF_RESOURCES.
> > + // If the allocation of a DPC entry fails, return EFI_OUT_OF_RESOURCES.
> > //
> > if (DpcEntry == NULL) {
> > - if (IsListEmpty (&mDpcEntryFreeList)) {
> > - ReturnStatus = EFI_OUT_OF_RESOURCES;
> > - goto Done;
> > - }
> > + ReturnStatus = EFI_OUT_OF_RESOURCES;
> > + goto Done;
>
> I don't think it's a correct fix. This DpcEntry allocation is inside a for loop which
> tries to allocate 64 new DPC entries, if one of the allocation in the middle of
> this loop failed, the mDpcEntryFreeList will contain useable entries for this
> DpcQueueDpc(), that's why original code doesn't treat it as an error.
>
> How do you find this issue? By code review or it cause some real problems?
>
It is my local tool that reported this issue. You are right so I will find another method to
fix this.
Thanks,
Shenglei
> Thanks
> Siyuan
>
> > }
> >
> > //
> > --
> > 2.18.0.windows.1
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-14 9:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-10-12 7:43 [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL Zhang, Shenglei
2019-10-12 7:43 ` [PATCH] NetworkPkg/DxeNetLib: Change the order of conditions in IF statement Zhang, Shenglei
2019-10-14 2:21 ` Siyuan, Fu
2019-10-14 9:46 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-10-12 7:58 ` [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL Siyuan, Fu
2019-10-12 8:42 ` Zhang, Shenglei
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox