[PATCH 0/2] Coverity fixes for EAL

[PATCH 0/2] Coverity fixes for EAL

[Anatoly Burakov]

This patchset fixes a few Coverity fixes in EAL introduced
by recent DPDK memory hotplug patchset.

Coverity issues fixed:
- 272600 - negative return not handled
- 272607 - error condition not handled

One of existing coverity issues is not fixed by this patchset:
- 272585 - memory leak

due to being independently discovered and addressed in a
separate patch [1].

[1] http://dpdk.org/dev/patchwork/patch/38301/

Anatoly Burakov (2):
  eal: fix potential negative return
  eal: fix not checking unlock result

 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH 1/2] eal: fix potential negative return

[Anatoly Burakov]

Value returned by rte_socket_id_by_idx() may be negative, which would
result in negative array index access.

Coverity issue: 272600

Fixes: b666f17858a3 ("mem: read hugepage counts from node-specific sysfs path")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
index fb4b667..009f963 100644
--- a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
+++ b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
@@ -402,9 +402,16 @@ hugepage_info_init(void)
 		/* we also don't want to do this for legacy init */
 		if (!internal_config.legacy_mem)
 			for (i = 0; i < rte_socket_count(); i++) {
-				int socket = rte_socket_id_by_idx(i);
-				unsigned int num_pages =
-						get_num_hugepages_on_node(
+				int socket;
+				unsigned int num_pages;
+
+				socket = rte_socket_id_by_idx(i);
+				if (socket < 0) {
+					RTE_LOG(ERR, EAL, "Invalid socket index: %u\n",
+							i);
+					continue;
+				}
+				num_pages = get_num_hugepages_on_node(
 							dirent->d_name, socket);
 				hpi->num_pages[socket] = num_pages;
 				total_pages += num_pages;
-- 
2.7.4

[PATCH 2/2] eal: fix not checking unlock result

[Anatoly Burakov]

Coverity issue: 272607

Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
index 009f963..01fee51 100644
--- a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
+++ b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
@@ -304,8 +304,11 @@ clear_hugedir(const char * hugedir)
 		/* if lock succeeds, unlock and remove the file */
 		if (lck_result != -1) {
 			lck.l_type = F_UNLCK;
-			fcntl(fd, F_SETLK, &lck);
-			unlinkat(dir_fd, dirent->d_name, 0);
+			if (fcntl(fd, F_SETLK, &lck) < 0)
+				RTE_LOG(ERR, EAL, "Couldn't unlock %s: %s\n",
+					dirent->d_name, strerror(errno));
+			else
+				unlinkat(dir_fd, dirent->d_name, 0);
 		}
 		close (fd);
 		dirent = readdir(dir);
-- 
2.7.4

Re: [PATCH 1/2] eal: fix potential negative return

[Tan, Jianfeng]

On 4/17/2018 11:42 PM, Anatoly Burakov wrote:
> Value returned by rte_socket_id_by_idx() may be negative, which would
> result in negative array index access.
>
> Coverity issue: 272600
>
> Fixes: b666f17858a3 ("mem: read hugepage counts from node-specific sysfs path")
> Cc: anatoly.burakov@intel.com
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>

Re: [PATCH 2/2] eal: fix not checking unlock result

[Tan, Jianfeng]

On 4/17/2018 11:42 PM, Anatoly Burakov wrote:
> Coverity issue: 272607
>
> Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
> Cc: anatoly.burakov@intel.com
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>   lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
> index 009f963..01fee51 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
> @@ -304,8 +304,11 @@ clear_hugedir(const char * hugedir)
>   		/* if lock succeeds, unlock and remove the file */
>   		if (lck_result != -1) {
>   			lck.l_type = F_UNLCK;
> -			fcntl(fd, F_SETLK, &lck);
> -			unlinkat(dir_fd, dirent->d_name, 0);
> +			if (fcntl(fd, F_SETLK, &lck) < 0)
> +				RTE_LOG(ERR, EAL, "Couldn't unlock %s: %s\n",
> +					dirent->d_name, strerror(errno));

It seems that we shall return error if this nearly-impossible error 
happens, no?

> +			else
> +				unlinkat(dir_fd, dirent->d_name, 0);
>   		}
>   		close (fd);
>   		dirent = readdir(dir);

Re: [PATCH 2/2] eal: fix not checking unlock result

[Burakov, Anatoly]

On 25-Apr-18 8:09 AM, Tan, Jianfeng wrote:
> 
> 
> On 4/17/2018 11:42 PM, Anatoly Burakov wrote:
>> Coverity issue: 272607
>>
>> Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
>> Cc: anatoly.burakov@intel.com
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>   lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c 
>> b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
>> index 009f963..01fee51 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
>> @@ -304,8 +304,11 @@ clear_hugedir(const char * hugedir)
>>           /* if lock succeeds, unlock and remove the file */
>>           if (lck_result != -1) {
>>               lck.l_type = F_UNLCK;
>> -            fcntl(fd, F_SETLK, &lck);
>> -            unlinkat(dir_fd, dirent->d_name, 0);
>> +            if (fcntl(fd, F_SETLK, &lck) < 0)
>> +                RTE_LOG(ERR, EAL, "Couldn't unlock %s: %s\n",
>> +                    dirent->d_name, strerror(errno));
> 
> It seems that we shall return error if this nearly-impossible error 
> happens, no?

I'm not sure if we should. In any case, lock will be dropped by close(), 
so the proper fix would've been just remove the fcntl() call altogether.

I'll respin.

> 
>> +            else
>> +                unlinkat(dir_fd, dirent->d_name, 0);
>>           }
>>           close (fd);
>>           dirent = readdir(dir);
> 
> 


-- 
Thanks,
Anatoly

[PATCH v2] Coverity fixes for EAL

[Anatoly Burakov]

This patchset fixes a few Coverity fixes in EAL introduced
by recent DPDK memory hotplug patchset.

Coverity issues fixed:
- 272607 - error condition not handled

Coverity issues not fixed:
- 272600 - negative return not handled
  - Proper usage of API guarantees no negative return

One of existing coverity issues is not fixed by this patchset:
- 272585 - memory leak

due to being independently discovered and addressed in a
separate patch [1].

[1] http://dpdk.org/dev/patchwork/patch/38301/

v2:
- Dropped fix for 272600 and marked it as false positive

Anatoly Burakov (1):
  eal: remove call to unlock

 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

-- 
2.7.4

[PATCH v2] eal: remove call to unlock

[Anatoly Burakov]

Coverity was complaining about not checking result of call to
fcntl() for unlocking the file. Disregarding the fact that error
value returned from fcntl() unlock call is highly unlikely in the
first place, we are subsequently calling close() on that same fd,
which will drop the lock, which makes call to fcntl() unnecessary.

Fix this by removing a call to fcntl() altogether.

Coverity issue: 272607

Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---

Notes:
    v2:
    - Removed call to fcntl() instead of handling return value

 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
index db5aabd..485a89e 100644
--- a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
+++ b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
@@ -304,11 +304,8 @@ clear_hugedir(const char * hugedir)
 		lck_result = fcntl(fd, F_SETLK, &lck);
 
 		/* if lock succeeds, unlock and remove the file */
-		if (lck_result != -1) {
-			lck.l_type = F_UNLCK;
-			fcntl(fd, F_SETLK, &lck);
+		if (lck_result != -1)
 			unlinkat(dir_fd, dirent->d_name, 0);
-		}
 		close (fd);
 		dirent = readdir(dir);
 	}
-- 
2.7.4

Re: [PATCH v2] eal: remove call to unlock

[Thomas Monjalon]

25/04/2018 12:08, Anatoly Burakov:
> Coverity was complaining about not checking result of call to
> fcntl() for unlocking the file. Disregarding the fact that error
> value returned from fcntl() unlock call is highly unlikely in the
> first place, we are subsequently calling close() on that same fd,
> which will drop the lock, which makes call to fcntl() unnecessary.
> 
> Fix this by removing a call to fcntl() altogether.
> 
> Coverity issue: 272607
> 
> Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

Applied, thanks