[edk2] [patch] NetworkPkg: Fix a bug that return type differs from the left one when assigned, another is redefinition when checking whether the string is null.

[Zhang Lubo]

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Zhang Lubo <***@intel.com>
---
NetworkPkg/Application/IfConfig6/IfConfig6.c | 2 +-
NetworkPkg/Mtftp6Dxe/Mtftp6Support.c | 47 ++++++++++++++++++++++------
2 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/NetworkPkg/Application/IfConfig6/IfConfig6.c b/NetworkPkg/Application/IfConfig6/IfConfig6.c
index 4cec44f..e66d52a 100644
--- a/NetworkPkg/Application/IfConfig6/IfConfig6.c
+++ b/NetworkPkg/Application/IfConfig6/IfConfig6.c
@@ -125,11 +125,11 @@ SplitStrToList (
CHAR16 *Str;
CHAR16 *ArgStr;
ARG_LIST *ArgList;
ARG_LIST *ArgNode;

- if (*String == L'\0' || *String == NULL) {
+ if (*String == L'\0') {
return NULL;
}

//
// Copy the CONST string to a local copy.
diff --git a/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c b/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c
index 91680b2..fc8a469 100644
--- a/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c
+++ b/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c
@@ -450,10 +450,11 @@ Mtftp6SendRequest (
)
{
EFI_MTFTP6_PACKET *Packet;
EFI_MTFTP6_OPTION *Options;
EFI_MTFTP6_TOKEN *Token;
+ RETURN_STATUS Status;
NET_BUF *Nbuf;
UINT8 *Mode;
UINT8 *Cur;
UINT32 Len1;
UINT32 Len2;
@@ -510,26 +511,50 @@ Mtftp6SendRequest (
Packet = (EFI_MTFTP6_PACKET *) NetbufAllocSpace (Nbuf, Len, FALSE);
ASSERT (Packet != NULL);

Packet->OpCode = HTONS (Operation);
Cur = Packet->Rrq.Filename;
- Cur = (UINT8 *) AsciiStrCpyS ((CHAR8 *) Cur, Len - 2, (CHAR8 *) Token->Filename);
+ Status = AsciiStrCpyS ((CHAR8 *) Cur, Len - 2, (CHAR8 *) Token->Filename);
+
+ if (EFI_ERROR (Status)) {
+ goto ERROR_EXIT;
+ }
+
+ Cur = (UINT8 *) Cur;
Cur += AsciiStrLen ((CHAR8 *) Token->Filename) + 1;
- Cur = (UINT8 *) AsciiStrCpyS ((CHAR8 *) Cur, Len - 2 - (AsciiStrLen ((CHAR8 *) Token->Filename) + 1), (CHAR8 *) Mode);
+ Status = AsciiStrCpyS ((CHAR8 *) Cur, Len - 2 - (AsciiStrLen ((CHAR8 *) Token->Filename) + 1), (CHAR8 *) Mode);
+
+ if (EFI_ERROR (Status)) {
+ goto ERROR_EXIT;
+ }
+
+ Cur = (UINT8 *) Cur;
Cur += AsciiStrLen ((CHAR8 *) Mode) + 1;
- Len -= ((UINT32) AsciiStrLen ((CHAR8 *) Token->Filename) + (UINT32) AsciiStrLen ((CHAR8 *) Mode) + 4);
+ Len -= ((UINT32) AsciiStrLen ((CHAR8 *) Token->Filename) + (UINT32) AsciiStrLen ((CHAR8 *) Mode) + 4);

//
// Copy all the extension options into the packet.
//
for (Index = 0; Index < Token->OptionCount; ++Index) {
- Cur = (UINT8 *) AsciiStrCpyS ((CHAR8 *) Cur, Len, (CHAR8 *) Options[Index].OptionStr);
- Cur += AsciiStrLen ((CHAR8 *) Options[Index].OptionStr) + 1;
- Len -= (UINT32)(AsciiStrLen ((CHAR8 *) Options[Index].OptionStr) + 1);
- Cur = (UINT8 *) AsciiStrCpyS ((CHAR8 *) Cur, Len, (CHAR8 *) Options[Index].ValueStr);
- Cur += AsciiStrLen ((CHAR8 *) (CHAR8 *) Options[Index].ValueStr) + 1;
- Len -= (UINT32)(AsciiStrLen ((CHAR8 *) Options[Index].ValueStr) + 1);
+ Status = AsciiStrCpyS ((CHAR8 *) Cur, Len, (CHAR8 *) Options[Index].OptionStr);
+
+ if (EFI_ERROR (Status)) {
+ goto ERROR_EXIT;
+ }
+
+ Cur = (UINT8 *) Cur;
+ Cur += AsciiStrLen ((CHAR8 *) Options[Index].OptionStr) + 1;
+ Len -= (UINT32)(AsciiStrLen ((CHAR8 *) Options[Index].OptionStr) + 1);
+ Status = (UINT8 *) AsciiStrCpyS ((CHAR8 *) Cur, Len, (CHAR8 *) Options[Index].ValueStr);
+
+ if (EFI_ERROR (Status)) {
+ goto ERROR_EXIT;
+ }
+
+ Cur = (UINT8 *) Cur;
+ Cur += AsciiStrLen ((CHAR8 *) (CHAR8 *) Options[Index].ValueStr) + 1;
+ Len -= (UINT32)(AsciiStrLen ((CHAR8 *) Options[Index].ValueStr) + 1);
}

//
// Save the packet buf for retransmit
//
@@ -539,10 +564,14 @@ Mtftp6SendRequest (

Instance->LastPacket = Nbuf;
Instance->CurRetry = 0;

return Mtftp6TransmitPacket (Instance, Nbuf);
+
+ERROR_EXIT:
+ NetbufFree (Nbuf);
+ return Status;
}


/**
Build and send an error packet.

[Fu, Siyuan]

Hi, Lubo

There is a lot of string length calculate in Mtftp4SendRequest() function, for example " AsciiStrLen ((CHAR8 *) Token->Filename)" is call four times, please record these length in local variable to save API call overhead and also make code easy to understand.
And please remove the redundant " Cur = (UINT8 *) Cur" from your code.

Siyuan

-----Original Message-----
From: Zhang, Lubo
Sent: Thursday, July 09, 2015 4:04 PM
To: edk2-***@lists.sourceforge.net; Qiu, Shumin; Fu, Siyuan
Subject: [patch] NetworkPkg: Fix a bug that return type differs from the left one when assigned, another is redefinition when checking whether the string is null.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Zhang Lubo <***@intel.com>
---
NetworkPkg/Application/IfConfig6/IfConfig6.c | 2 +-
NetworkPkg/Mtftp6Dxe/Mtftp6Support.c | 47 ++++++++++++++++++++++------
2 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/NetworkPkg/Application/IfConfig6/IfConfig6.c b/NetworkPkg/Application/IfConfig6/IfConfig6.c
index 4cec44f..e66d52a 100644
--- a/NetworkPkg/Application/IfConfig6/IfConfig6.c
+++ b/NetworkPkg/Application/IfConfig6/IfConfig6.c
@@ -125,11 +125,11 @@ SplitStrToList (
CHAR16 *Str;
CHAR16 *ArgStr;
ARG_LIST *ArgList;
ARG_LIST *ArgNode;

- if (*String == L'\0' || *String == NULL) {
+ if (*String == L'\0') {
return NULL;
}

//
// Copy the CONST string to a local copy.
diff --git a/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c b/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c
index 91680b2..fc8a469 100644
--- a/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c
+++ b/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c
@@ -450,10 +450,11 @@ Mtftp6SendRequest (
)
{
EFI_MTFTP6_PACKET *Packet;
EFI_MTFTP6_OPTION *Options;
EFI_MTFTP6_TOKEN *Token;
+ RETURN_STATUS Status;
NET_BUF *Nbuf;
UINT8 *Mode;
UINT8 *Cur;
UINT32 Len1;
UINT32 Len2;
@@ -510,26 +511,50 @@ Mtftp6SendRequest (
Packet = (EFI_MTFTP6_PACKET *) NetbufAllocSpace (Nbuf, Len, FALSE);
ASSERT (Packet != NULL);

Packet->OpCode = HTONS (Operation);
Cur = Packet->Rrq.Filename;
- Cur = (UINT8 *) AsciiStrCpyS ((CHAR8 *) Cur, Len - 2, (CHAR8 *) Token->Filename);
+ Status = AsciiStrCpyS ((CHAR8 *) Cur, Len - 2, (CHAR8 *) Token->Filename);
+
+ if (EFI_ERROR (Status)) {
+ goto ERROR_EXIT;
+ }
+
+ Cur = (UINT8 *) Cur;
Cur += AsciiStrLen ((CHAR8 *) Token->Filename) + 1;
- Cur = (UINT8 *) AsciiStrCpyS ((CHAR8 *) Cur, Len - 2 - (AsciiStrLen ((CHAR8 *) Token->Filename) + 1), (CHAR8 *) Mode);
+ Status = AsciiStrCpyS ((CHAR8 *) Cur, Len - 2 - (AsciiStrLen ((CHAR8 *) Token->Filename) + 1), (CHAR8 *) Mode);
+
+ if (EFI_ERROR (Status)) {
+ goto ERROR_EXIT;
+ }
+
+ Cur = (UINT8 *) Cur;
Cur += AsciiStrLen ((CHAR8 *) Mode) + 1;
- Len -= ((UINT32) AsciiStrLen ((CHAR8 *) Token->Filename) + (UINT32) AsciiStrLen ((CHAR8 *) Mode) + 4);
+ Len -= ((UINT32) AsciiStrLen ((CHAR8 *) Token->Filename) + (UINT32) AsciiStrLen ((CHAR8 *) Mode) + 4);

//
// Copy all the extension options into the packet.
//
for (Index = 0; Index < Token->OptionCount; ++Index) {
- Cur = (UINT8 *) AsciiStrCpyS ((CHAR8 *) Cur, Len, (CHAR8 *) Options[Index].OptionStr);
- Cur += AsciiStrLen ((CHAR8 *) Options[Index].OptionStr) + 1;
- Len -= (UINT32)(AsciiStrLen ((CHAR8 *) Options[Index].OptionStr) + 1);
- Cur = (UINT8 *) AsciiStrCpyS ((CHAR8 *) Cur, Len, (CHAR8 *) Options[Index].ValueStr);
- Cur += AsciiStrLen ((CHAR8 *) (CHAR8 *) Options[Index].ValueStr) + 1;
- Len -= (UINT32)(AsciiStrLen ((CHAR8 *) Options[Index].ValueStr) + 1);
+ Status = AsciiStrCpyS ((CHAR8 *) Cur, Len, (CHAR8 *) Options[Index].OptionStr);
+
+ if (EFI_ERROR (Status)) {
+ goto ERROR_EXIT;
+ }
+
+ Cur = (UINT8 *) Cur;
+ Cur += AsciiStrLen ((CHAR8 *) Options[Index].OptionStr) + 1;
+ Len -= (UINT32)(AsciiStrLen ((CHAR8 *) Options[Index].OptionStr) + 1);
+ Status = (UINT8 *) AsciiStrCpyS ((CHAR8 *) Cur, Len, (CHAR8 *) Options[Index].ValueStr);
+
+ if (EFI_ERROR (Status)) {
+ goto ERROR_EXIT;
+ }
+
+ Cur = (UINT8 *) Cur;
+ Cur += AsciiStrLen ((CHAR8 *) (CHAR8 *) Options[Index].ValueStr) + 1;
+ Len -= (UINT32)(AsciiStrLen ((CHAR8 *) Options[Index].ValueStr) + 1);
}

//
// Save the packet buf for retransmit
//
@@ -539,10 +564,14 @@ Mtftp6SendRequest (

Instance->LastPacket = Nbuf;
Instance->CurRetry = 0;

return Mtftp6TransmitPacket (Instance, Nbuf);
+
+ERROR_EXIT:
+ NetbufFree (Nbuf);
+ return Status;
}


/**
Build and send an error packet.

[Fu, Siyuan]

Hi, Lubo

For Ifconfig6.c, it's not a correct fix, I guess the original code want to check whether String is NULL or not. Simply remove it is not safe.

Mtftp6Support patch has the same issue as I said in Mtftp4Support. And since you always allocate length enough buffer, the AsciiStrCpyS should not return an error so you should use ASSERT_EFI_ERROR instead of check the return code of it.

Siyuan

-----Original Message-----
From: Zhang, Lubo
Sent: Thursday, July 09, 2015 4:04 PM
To: edk2-***@lists.sourceforge.net; Qiu, Shumin; Fu, Siyuan
Subject: [patch] NetworkPkg: Fix a bug that return type differs from the left one when assigned, another is redefinition when checking whether the string is null.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Zhang Lubo <***@intel.com>
---
NetworkPkg/Application/IfConfig6/IfConfig6.c | 2 +-
NetworkPkg/Mtftp6Dxe/Mtftp6Support.c | 47 ++++++++++++++++++++++------
2 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/NetworkPkg/Application/IfConfig6/IfConfig6.c b/NetworkPkg/Application/IfConfig6/IfConfig6.c
index 4cec44f..e66d52a 100644
--- a/NetworkPkg/Application/IfConfig6/IfConfig6.c
+++ b/NetworkPkg/Application/IfConfig6/IfConfig6.c
@@ -125,11 +125,11 @@ SplitStrToList (
CHAR16 *Str;
CHAR16 *ArgStr;
ARG_LIST *ArgList;
ARG_LIST *ArgNode;

- if (*String == L'\0' || *String == NULL) {
+ if (*String == L'\0') {
return NULL;
}

//
// Copy the CONST string to a local copy.
diff --git a/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c b/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c
index 91680b2..fc8a469 100644
--- a/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c
+++ b/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c
@@ -450,10 +450,11 @@ Mtftp6SendRequest (
)
{
EFI_MTFTP6_PACKET *Packet;
EFI_MTFTP6_OPTION *Options;
EFI_MTFTP6_TOKEN *Token;
+ RETURN_STATUS Status;
NET_BUF *Nbuf;
UINT8 *Mode;
UINT8 *Cur;
UINT32 Len1;
UINT32 Len2;
@@ -510,26 +511,50 @@ Mtftp6SendRequest (
Packet = (EFI_MTFTP6_PACKET *) NetbufAllocSpace (Nbuf, Len, FALSE);
ASSERT (Packet != NULL);

Packet->OpCode = HTONS (Operation);
Cur = Packet->Rrq.Filename;
- Cur = (UINT8 *) AsciiStrCpyS ((CHAR8 *) Cur, Len - 2, (CHAR8 *) Token->Filename);
+ Status = AsciiStrCpyS ((CHAR8 *) Cur, Len - 2, (CHAR8 *) Token->Filename);
+
+ if (EFI_ERROR (Status)) {
+ goto ERROR_EXIT;
+ }
+
+ Cur = (UINT8 *) Cur;
Cur += AsciiStrLen ((CHAR8 *) Token->Filename) + 1;
- Cur = (UINT8 *) AsciiStrCpyS ((CHAR8 *) Cur, Len - 2 - (AsciiStrLen ((CHAR8 *) Token->Filename) + 1), (CHAR8 *) Mode);
+ Status = AsciiStrCpyS ((CHAR8 *) Cur, Len - 2 - (AsciiStrLen ((CHAR8 *) Token->Filename) + 1), (CHAR8 *) Mode);
+
+ if (EFI_ERROR (Status)) {
+ goto ERROR_EXIT;
+ }
+
+ Cur = (UINT8 *) Cur;
Cur += AsciiStrLen ((CHAR8 *) Mode) + 1;
- Len -= ((UINT32) AsciiStrLen ((CHAR8 *) Token->Filename) + (UINT32) AsciiStrLen ((CHAR8 *) Mode) + 4);
+ Len -= ((UINT32) AsciiStrLen ((CHAR8 *) Token->Filename) + (UINT32) AsciiStrLen ((CHAR8 *) Mode) + 4);

//
// Copy all the extension options into the packet.
//
for (Index = 0; Index < Token->OptionCount; ++Index) {
- Cur = (UINT8 *) AsciiStrCpyS ((CHAR8 *) Cur, Len, (CHAR8 *) Options[Index].OptionStr);
- Cur += AsciiStrLen ((CHAR8 *) Options[Index].OptionStr) + 1;
- Len -= (UINT32)(AsciiStrLen ((CHAR8 *) Options[Index].OptionStr) + 1);
- Cur = (UINT8 *) AsciiStrCpyS ((CHAR8 *) Cur, Len, (CHAR8 *) Options[Index].ValueStr);
- Cur += AsciiStrLen ((CHAR8 *) (CHAR8 *) Options[Index].ValueStr) + 1;
- Len -= (UINT32)(AsciiStrLen ((CHAR8 *) Options[Index].ValueStr) + 1);
+ Status = AsciiStrCpyS ((CHAR8 *) Cur, Len, (CHAR8 *) Options[Index].OptionStr);
+
+ if (EFI_ERROR (Status)) {
+ goto ERROR_EXIT;
+ }
+
+ Cur = (UINT8 *) Cur;
+ Cur += AsciiStrLen ((CHAR8 *) Options[Index].OptionStr) + 1;
+ Len -= (UINT32)(AsciiStrLen ((CHAR8 *) Options[Index].OptionStr) + 1);
+ Status = (UINT8 *) AsciiStrCpyS ((CHAR8 *) Cur, Len, (CHAR8 *) Options[Index].ValueStr);
+
+ if (EFI_ERROR (Status)) {
+ goto ERROR_EXIT;
+ }
+
+ Cur = (UINT8 *) Cur;
+ Cur += AsciiStrLen ((CHAR8 *) (CHAR8 *) Options[Index].ValueStr) + 1;
+ Len -= (UINT32)(AsciiStrLen ((CHAR8 *) Options[Index].ValueStr) + 1);
}

//
// Save the packet buf for retransmit
//
@@ -539,10 +564,14 @@ Mtftp6SendRequest (

Instance->LastPacket = Nbuf;
Instance->CurRetry = 0;

return Mtftp6TransmitPacket (Instance, Nbuf);
+
+ERROR_EXIT:
+ NetbufFree (Nbuf);
+ return Status;
}


/**
Build and send an error packet.