[dpdk-dev] [PATCH 0/5] Coverity fixes for malloc

[dpdk-dev] [PATCH 0/5] Coverity fixes for malloc

[Anatoly Burakov]

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

Coverity issues fixed:
- 272571 - negative return not handled
- 272597 - negative return not handled
- 272574 - use value before verification
- 272590 - negative return not handled
- 272566 - possible null dereference

The following coverity issues were not fixed:
- 272573 - calling memset with size 0
  - This is intentional, size will not be 0 in malloc debug

Also, replace all instaces of snprintf with strlcpy.

Anatoly Burakov (5):
  malloc: replace snprintf with strlcpy
  malloc: fix potential out-of-bounds array access
  malloc: fix potential negative return
  malloc: fix potential dereferencing of NULL pointer
  malloc: fix potential negative return

 lib/librte_eal/common/malloc_elem.c |  6 ++++++
 lib/librte_eal/common/malloc_heap.c |  7 ++++++-
 lib/librte_eal/common/malloc_mp.c   | 23 +++++++++++------------
 lib/librte_eal/common/rte_malloc.c  |  6 +++++-
 4 files changed, 28 insertions(+), 14 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH 1/5] malloc: replace snprintf with strlcpy

[Anatoly Burakov]

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/malloc_mp.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/lib/librte_eal/common/malloc_mp.c b/lib/librte_eal/common/malloc_mp.c
index 72b1f4c..931c14b 100644
--- a/lib/librte_eal/common/malloc_mp.c
+++ b/lib/librte_eal/common/malloc_mp.c
@@ -7,6 +7,7 @@
 
 #include <rte_alarm.h>
 #include <rte_errno.h>
+#include <rte_string_fns.h>
 
 #include "eal_memalloc.h"
 
@@ -159,7 +160,7 @@ handle_sync(const struct rte_mp_msg *msg, const void *peer)
 	memset(&reply, 0, sizeof(reply));
 
 	reply.num_fds = 0;
-	snprintf(reply.name, sizeof(reply.name), "%s", msg->name);
+	strlcpy(reply.name, msg->name, sizeof(reply.name));
 	reply.len_param = sizeof(*resp);
 
 	ret = eal_memalloc_sync_with_primary();
@@ -274,8 +275,8 @@ handle_request(const struct rte_mp_msg *msg, const void *peer __rte_unused)
 		/* send failure message straight away */
 		resp_msg.num_fds = 0;
 		resp_msg.len_param = sizeof(*resp);
-		snprintf(resp_msg.name, sizeof(resp_msg.name), "%s",
-				MP_ACTION_RESPONSE);
+		strlcpy(resp_msg.name, MP_ACTION_RESPONSE,
+				sizeof(resp_msg.name));
 
 		resp->t = m->t;
 		resp->result = REQ_RESULT_FAIL;
@@ -298,8 +299,7 @@ handle_request(const struct rte_mp_msg *msg, const void *peer __rte_unused)
 		/* we can do something, so send sync request asynchronously */
 		sr_msg.num_fds = 0;
 		sr_msg.len_param = sizeof(*sr);
-		snprintf(sr_msg.name, sizeof(sr_msg.name), "%s",
-				MP_ACTION_SYNC);
+		strlcpy(sr_msg.name, MP_ACTION_SYNC, sizeof(sr_msg.name));
 
 		ts.tv_nsec = 0;
 		ts.tv_sec = MP_TIMEOUT_S;
@@ -393,7 +393,7 @@ handle_sync_response(const struct rte_mp_msg *request,
 		resp->id = entry->user_req.id;
 		msg.num_fds = 0;
 		msg.len_param = sizeof(*resp);
-		snprintf(msg.name, sizeof(msg.name), "%s", MP_ACTION_RESPONSE);
+		strlcpy(msg.name, MP_ACTION_RESPONSE, sizeof(msg.name));
 
 		if (rte_mp_sendmsg(&msg))
 			RTE_LOG(ERR, EAL, "Could not send message to secondary process\n");
@@ -417,7 +417,7 @@ handle_sync_response(const struct rte_mp_msg *request,
 		resp->id = entry->user_req.id;
 		msg.num_fds = 0;
 		msg.len_param = sizeof(*resp);
-		snprintf(msg.name, sizeof(msg.name), "%s", MP_ACTION_RESPONSE);
+		strlcpy(msg.name, MP_ACTION_RESPONSE, sizeof(msg.name));
 
 		if (rte_mp_sendmsg(&msg))
 			RTE_LOG(ERR, EAL, "Could not send message to secondary process\n");
@@ -444,8 +444,7 @@ handle_sync_response(const struct rte_mp_msg *request,
 		/* send rollback request */
 		rb_msg.num_fds = 0;
 		rb_msg.len_param = sizeof(*rb);
-		snprintf(rb_msg.name, sizeof(rb_msg.name), "%s",
-				MP_ACTION_ROLLBACK);
+		strlcpy(rb_msg.name, MP_ACTION_ROLLBACK, sizeof(rb_msg.name));
 
 		ts.tv_nsec = 0;
 		ts.tv_sec = MP_TIMEOUT_S;
@@ -515,7 +514,7 @@ handle_rollback_response(const struct rte_mp_msg *request,
 	resp->id = mpreq->id;
 	msg.num_fds = 0;
 	msg.len_param = sizeof(*resp);
-	snprintf(msg.name, sizeof(msg.name), "%s", MP_ACTION_RESPONSE);
+	strlcpy(msg.name, MP_ACTION_RESPONSE, sizeof(msg.name));
 
 	if (rte_mp_sendmsg(&msg))
 		RTE_LOG(ERR, EAL, "Could not send message to secondary process\n");
@@ -577,7 +576,7 @@ request_sync(void)
 
 	msg.num_fds = 0;
 	msg.len_param = sizeof(*req);
-	snprintf(msg.name, sizeof(msg.name), "%s", MP_ACTION_SYNC);
+	strlcpy(msg.name, MP_ACTION_SYNC, sizeof(msg.name));
 
 	/* sync request carries no data */
 	req->t = REQ_TYPE_SYNC;
@@ -668,7 +667,7 @@ request_to_primary(struct malloc_mp_req *user_req)
 
 	msg.num_fds = 0;
 	msg.len_param = sizeof(*msg_req);
-	snprintf(msg.name, sizeof(msg.name), "%s", MP_ACTION_REQUEST);
+	strlcpy(msg.name, MP_ACTION_REQUEST, sizeof(msg.name));
 
 	/* (attempt to) get a unique id */
 	user_req->id = get_unique_id();
-- 
2.7.4

[dpdk-dev] [PATCH 2/5] malloc: fix potential out-of-bounds array access

[Anatoly Burakov]

Technically, while the pointer would've been invalid if msl_idx
were invalid, we wouldn't have actually attempted to access the
pointer until verifying the index. Fix it by moving array access
to after we've verified validity of the index.

Coverity issue: 272574

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/common/malloc_heap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/malloc_heap.c b/lib/librte_eal/common/malloc_heap.c
index 590e9e3..5cf7231 100644
--- a/lib/librte_eal/common/malloc_heap.c
+++ b/lib/librte_eal/common/malloc_heap.c
@@ -99,11 +99,12 @@ malloc_add_seg(const struct rte_memseg_list *msl,
 
 	/* msl is const, so find it */
 	msl_idx = msl - mcfg->memsegs;
-	found_msl = &mcfg->memsegs[msl_idx];
 
 	if (msl_idx < 0 || msl_idx >= RTE_MAX_MEMSEG_LISTS)
 		return -1;
 
+	found_msl = &mcfg->memsegs[msl_idx];
+
 	malloc_heap_add_memory(heap, found_msl, ms->addr, len);
 
 	RTE_LOG(DEBUG, EAL, "Added %zuM to heap on socket %i\n", len >> 20,
-- 
2.7.4

[dpdk-dev] [PATCH 3/5] malloc: fix potential negative return

[Anatoly Burakov]

Return from rte_socket_id_by_idx() may be negative, which would
result in negative array index.

Coverity issue: 272590

Fixes: 1403f87d4fb8 ("malloc: enable memory hotplug support")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/malloc_heap.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_eal/common/malloc_heap.c b/lib/librte_eal/common/malloc_heap.c
index 5cf7231..f81aaf3 100644
--- a/lib/librte_eal/common/malloc_heap.c
+++ b/lib/librte_eal/common/malloc_heap.c
@@ -563,6 +563,10 @@ malloc_heap_alloc(const char *type, size_t size, int socket_arg,
 	/* try other heaps */
 	for (i = 0; i < (int) rte_socket_count(); i++) {
 		cur_socket = rte_socket_id_by_idx(i);
+		if (cur_socket < 0) {
+			RTE_LOG(ERR, EAL, "Invalid socket index: %i\n", i);
+			continue;
+		}
 		if (cur_socket == socket)
 			continue;
 		ret = heap_alloc_on_socket(type, size, cur_socket, flags,
-- 
2.7.4

[dpdk-dev] [PATCH 4/5] malloc: fix potential dereferencing of NULL pointer

[Anatoly Burakov]

Previous code checked for both first/last elements being NULL,
but if they weren't, the expectation was that they're both
non-NULL, which will be the case under normal conditions, but
may not be the case due to heap structure corruption.

Coverity issue: 272566

Fixes: bb372060dad4 ("malloc: make heap a doubly-linked list")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/malloc_elem.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_eal/common/malloc_elem.c b/lib/librte_eal/common/malloc_elem.c
index ee79dcd..af81961 100644
--- a/lib/librte_eal/common/malloc_elem.c
+++ b/lib/librte_eal/common/malloc_elem.c
@@ -49,6 +49,12 @@ malloc_elem_insert(struct malloc_elem *elem)
 	struct malloc_elem *prev_elem, *next_elem;
 	struct malloc_heap *heap = elem->heap;
 
+	/* first and last elements must be both NULL or both non-NULL */
+	if ((heap->first == NULL) != (heap->last == NULL)) {
+		RTE_LOG(ERR, EAL, "Heap is probably corrupt\n");
+		return;
+	}
+
 	if (heap->first == NULL && heap->last == NULL) {
 		/* if empty heap */
 		heap->first = elem;
-- 
2.7.4

[dpdk-dev] [PATCH 5/5] malloc: fix potential negative return

[Anatoly Burakov]

Return value from rte_socket_id_by_idx() may be negative, which would
result in negative index access.

Additionally, return value was of mismatched type (function returns
signed int, socket id was unsigned).

Coverity issue: 272571
Coverity issue: 272597

Fixes: 30bc6bf0d516 ("malloc: add function to dump heap contents")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/rte_malloc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/rte_malloc.c b/lib/librte_eal/common/rte_malloc.c
index b51a6d1..f207ba2 100644
--- a/lib/librte_eal/common/rte_malloc.c
+++ b/lib/librte_eal/common/rte_malloc.c
@@ -169,7 +169,11 @@ rte_malloc_dump_heaps(FILE *f)
 	unsigned int idx;
 
 	for (idx = 0; idx < rte_socket_count(); idx++) {
-		unsigned int socket = rte_socket_id_by_idx(idx);
+		int socket = rte_socket_id_by_idx(idx);
+		if (socket < 0) {
+			RTE_LOG(ERR, EAL, "Invalid socket index: %u\n", idx);
+			continue;
+		}
 		fprintf(f, "Heap on socket %i:\n", socket);
 		malloc_heap_dump(&mcfg->malloc_heaps[socket], f);
 	}
-- 
2.7.4

Re: [dpdk-dev] [PATCH 5/5] malloc: fix potential negative return

[Tan, Jianfeng]

On 4/17/2018 11:48 PM, Anatoly Burakov wrote:
> Return value from rte_socket_id_by_idx() may be negative, which would
> result in negative index access.
>
> Additionally, return value was of mismatched type (function returns
> signed int, socket id was unsigned).
>
> Coverity issue: 272571
> Coverity issue: 272597
>
> Fixes: 30bc6bf0d516 ("malloc: add function to dump heap contents")
> Cc: anatoly.burakov@intel.com
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>   lib/librte_eal/common/rte_malloc.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/common/rte_malloc.c b/lib/librte_eal/common/rte_malloc.c
> index b51a6d1..f207ba2 100644
> --- a/lib/librte_eal/common/rte_malloc.c
> +++ b/lib/librte_eal/common/rte_malloc.c
> @@ -169,7 +169,11 @@ rte_malloc_dump_heaps(FILE *f)
>   	unsigned int idx;
>   
>   	for (idx = 0; idx < rte_socket_count(); idx++) {
> -		unsigned int socket = rte_socket_id_by_idx(idx);
> +		int socket = rte_socket_id_by_idx(idx);
> +		if (socket < 0) {
> +			RTE_LOG(ERR, EAL, "Invalid socket index: %u\n", idx);
> +			continue;
> +		}

For such check (and many others), we are clear that idx is guaranteed by 
rte_socket_count(), so rte_socket_id_by_idx() can never return -1. So 
why not just reporting this as false-positive?

Thanks,
Jianfeng

>   		fprintf(f, "Heap on socket %i:\n", socket);
>   		malloc_heap_dump(&mcfg->malloc_heaps[socket], f);
>   	}

Re: [dpdk-dev] [PATCH 5/5] malloc: fix potential negative return

[Burakov, Anatoly]

On 25-Apr-18 9:24 AM, Tan, Jianfeng wrote:
> 
> 
> On 4/17/2018 11:48 PM, Anatoly Burakov wrote:
>> Return value from rte_socket_id_by_idx() may be negative, which would
>> result in negative index access.
>>
>> Additionally, return value was of mismatched type (function returns
>> signed int, socket id was unsigned).
>>
>> Coverity issue: 272571
>> Coverity issue: 272597
>>
>> Fixes: 30bc6bf0d516 ("malloc: add function to dump heap contents")
>> Cc: anatoly.burakov@intel.com
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>   lib/librte_eal/common/rte_malloc.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_eal/common/rte_malloc.c 
>> b/lib/librte_eal/common/rte_malloc.c
>> index b51a6d1..f207ba2 100644
>> --- a/lib/librte_eal/common/rte_malloc.c
>> +++ b/lib/librte_eal/common/rte_malloc.c
>> @@ -169,7 +169,11 @@ rte_malloc_dump_heaps(FILE *f)
>>       unsigned int idx;
>>       for (idx = 0; idx < rte_socket_count(); idx++) {
>> -        unsigned int socket = rte_socket_id_by_idx(idx);
>> +        int socket = rte_socket_id_by_idx(idx);
>> +        if (socket < 0) {
>> +            RTE_LOG(ERR, EAL, "Invalid socket index: %u\n", idx);
>> +            continue;
>> +        }
> 
> For such check (and many others), we are clear that idx is guaranteed by 
> rte_socket_count(), so rte_socket_id_by_idx() can never return -1. So 
> why not just reporting this as false-positive?

Well, technically, if someone were to corrupt rte_config, it would 
introduce a possibility of a negative return. However, i guess, at that 
point we've got bigger problems, so perhaps you're right and i should 
drop these fixes.

> 
> Thanks,
> Jianfeng
> 
>>           fprintf(f, "Heap on socket %i:\n", socket);
>>           malloc_heap_dump(&mcfg->malloc_heaps[socket], f);
>>       }
> 
> 


-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v2 0/3] Coverity fixes for malloc

[Anatoly Burakov]

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

Coverity issues fixed:
- 272566 - possible null dereference
- 272574 - use value before verification

The following coverity issues were not fixed:
- 272573 - calling memset with size 0
  - This is intentional, size will not be 0 in malloc debug
- 272571 - negative return not handled
  - False positive, proper API usage ensures no negative returns
- 272590 - negative return not handled
  - Same as above
- 272597 - negative return not handled
  - Same as above

Also, replace all instaces of snprintf with strlcpy.

v2:
- Dropped fixes for 272571, 272590, 272597 as false positives

Anatoly Burakov (3):
  malloc: replace snprintf with strlcpy
  malloc: fix potential out-of-bounds array access
  malloc: fix potential dereferencing of NULL pointer

 lib/librte_eal/common/malloc_elem.c |  6 ++++++
 lib/librte_eal/common/malloc_heap.c |  3 ++-
 lib/librte_eal/common/malloc_mp.c   | 23 +++++++++++------------
 3 files changed, 19 insertions(+), 13 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH v2 1/3] malloc: replace snprintf with strlcpy

[Anatoly Burakov]

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/malloc_mp.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/lib/librte_eal/common/malloc_mp.c b/lib/librte_eal/common/malloc_mp.c
index 72b1f4c..931c14b 100644
--- a/lib/librte_eal/common/malloc_mp.c
+++ b/lib/librte_eal/common/malloc_mp.c
@@ -7,6 +7,7 @@
 
 #include <rte_alarm.h>
 #include <rte_errno.h>
+#include <rte_string_fns.h>
 
 #include "eal_memalloc.h"
 
@@ -159,7 +160,7 @@ handle_sync(const struct rte_mp_msg *msg, const void *peer)
 	memset(&reply, 0, sizeof(reply));
 
 	reply.num_fds = 0;
-	snprintf(reply.name, sizeof(reply.name), "%s", msg->name);
+	strlcpy(reply.name, msg->name, sizeof(reply.name));
 	reply.len_param = sizeof(*resp);
 
 	ret = eal_memalloc_sync_with_primary();
@@ -274,8 +275,8 @@ handle_request(const struct rte_mp_msg *msg, const void *peer __rte_unused)
 		/* send failure message straight away */
 		resp_msg.num_fds = 0;
 		resp_msg.len_param = sizeof(*resp);
-		snprintf(resp_msg.name, sizeof(resp_msg.name), "%s",
-				MP_ACTION_RESPONSE);
+		strlcpy(resp_msg.name, MP_ACTION_RESPONSE,
+				sizeof(resp_msg.name));
 
 		resp->t = m->t;
 		resp->result = REQ_RESULT_FAIL;
@@ -298,8 +299,7 @@ handle_request(const struct rte_mp_msg *msg, const void *peer __rte_unused)
 		/* we can do something, so send sync request asynchronously */
 		sr_msg.num_fds = 0;
 		sr_msg.len_param = sizeof(*sr);
-		snprintf(sr_msg.name, sizeof(sr_msg.name), "%s",
-				MP_ACTION_SYNC);
+		strlcpy(sr_msg.name, MP_ACTION_SYNC, sizeof(sr_msg.name));
 
 		ts.tv_nsec = 0;
 		ts.tv_sec = MP_TIMEOUT_S;
@@ -393,7 +393,7 @@ handle_sync_response(const struct rte_mp_msg *request,
 		resp->id = entry->user_req.id;
 		msg.num_fds = 0;
 		msg.len_param = sizeof(*resp);
-		snprintf(msg.name, sizeof(msg.name), "%s", MP_ACTION_RESPONSE);
+		strlcpy(msg.name, MP_ACTION_RESPONSE, sizeof(msg.name));
 
 		if (rte_mp_sendmsg(&msg))
 			RTE_LOG(ERR, EAL, "Could not send message to secondary process\n");
@@ -417,7 +417,7 @@ handle_sync_response(const struct rte_mp_msg *request,
 		resp->id = entry->user_req.id;
 		msg.num_fds = 0;
 		msg.len_param = sizeof(*resp);
-		snprintf(msg.name, sizeof(msg.name), "%s", MP_ACTION_RESPONSE);
+		strlcpy(msg.name, MP_ACTION_RESPONSE, sizeof(msg.name));
 
 		if (rte_mp_sendmsg(&msg))
 			RTE_LOG(ERR, EAL, "Could not send message to secondary process\n");
@@ -444,8 +444,7 @@ handle_sync_response(const struct rte_mp_msg *request,
 		/* send rollback request */
 		rb_msg.num_fds = 0;
 		rb_msg.len_param = sizeof(*rb);
-		snprintf(rb_msg.name, sizeof(rb_msg.name), "%s",
-				MP_ACTION_ROLLBACK);
+		strlcpy(rb_msg.name, MP_ACTION_ROLLBACK, sizeof(rb_msg.name));
 
 		ts.tv_nsec = 0;
 		ts.tv_sec = MP_TIMEOUT_S;
@@ -515,7 +514,7 @@ handle_rollback_response(const struct rte_mp_msg *request,
 	resp->id = mpreq->id;
 	msg.num_fds = 0;
 	msg.len_param = sizeof(*resp);
-	snprintf(msg.name, sizeof(msg.name), "%s", MP_ACTION_RESPONSE);
+	strlcpy(msg.name, MP_ACTION_RESPONSE, sizeof(msg.name));
 
 	if (rte_mp_sendmsg(&msg))
 		RTE_LOG(ERR, EAL, "Could not send message to secondary process\n");
@@ -577,7 +576,7 @@ request_sync(void)
 
 	msg.num_fds = 0;
 	msg.len_param = sizeof(*req);
-	snprintf(msg.name, sizeof(msg.name), "%s", MP_ACTION_SYNC);
+	strlcpy(msg.name, MP_ACTION_SYNC, sizeof(msg.name));
 
 	/* sync request carries no data */
 	req->t = REQ_TYPE_SYNC;
@@ -668,7 +667,7 @@ request_to_primary(struct malloc_mp_req *user_req)
 
 	msg.num_fds = 0;
 	msg.len_param = sizeof(*msg_req);
-	snprintf(msg.name, sizeof(msg.name), "%s", MP_ACTION_REQUEST);
+	strlcpy(msg.name, MP_ACTION_REQUEST, sizeof(msg.name));
 
 	/* (attempt to) get a unique id */
 	user_req->id = get_unique_id();
-- 
2.7.4

[dpdk-dev] [PATCH v2 2/3] malloc: fix potential out-of-bounds array access

[Anatoly Burakov]

Technically, while the pointer would've been invalid if msl_idx
were invalid, we wouldn't have actually attempted to access the
pointer until verifying the index. Fix it by moving array access
to after we've verified validity of the index.

Coverity issue: 272574

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/common/malloc_heap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/malloc_heap.c b/lib/librte_eal/common/malloc_heap.c
index 590e9e3..5cf7231 100644
--- a/lib/librte_eal/common/malloc_heap.c
+++ b/lib/librte_eal/common/malloc_heap.c
@@ -99,11 +99,12 @@ malloc_add_seg(const struct rte_memseg_list *msl,
 
 	/* msl is const, so find it */
 	msl_idx = msl - mcfg->memsegs;
-	found_msl = &mcfg->memsegs[msl_idx];
 
 	if (msl_idx < 0 || msl_idx >= RTE_MAX_MEMSEG_LISTS)
 		return -1;
 
+	found_msl = &mcfg->memsegs[msl_idx];
+
 	malloc_heap_add_memory(heap, found_msl, ms->addr, len);
 
 	RTE_LOG(DEBUG, EAL, "Added %zuM to heap on socket %i\n", len >> 20,
-- 
2.7.4

[dpdk-dev] [PATCH v2 3/3] malloc: fix potential dereferencing of NULL pointer

[Anatoly Burakov]

Previous code checked for both first/last elements being NULL,
but if they weren't, the expectation was that they're both
non-NULL, which will be the case under normal conditions, but
may not be the case due to heap structure corruption.

Coverity issue: 272566

Fixes: bb372060dad4 ("malloc: make heap a doubly-linked list")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/malloc_elem.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_eal/common/malloc_elem.c b/lib/librte_eal/common/malloc_elem.c
index ee79dcd..af81961 100644
--- a/lib/librte_eal/common/malloc_elem.c
+++ b/lib/librte_eal/common/malloc_elem.c
@@ -49,6 +49,12 @@ malloc_elem_insert(struct malloc_elem *elem)
 	struct malloc_elem *prev_elem, *next_elem;
 	struct malloc_heap *heap = elem->heap;
 
+	/* first and last elements must be both NULL or both non-NULL */
+	if ((heap->first == NULL) != (heap->last == NULL)) {
+		RTE_LOG(ERR, EAL, "Heap is probably corrupt\n");
+		return;
+	}
+
 	if (heap->first == NULL && heap->last == NULL) {
 		/* if empty heap */
 		heap->first = elem;
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 1/3] malloc: replace snprintf with strlcpy

[Van Haaren, Harry]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Anatoly Burakov
> Sent: Wednesday, April 25, 2018 11:16 AM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net
> Subject: [dpdk-dev] [PATCH v2 1/3] malloc: replace snprintf with strlcpy
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Re: [dpdk-dev] [PATCH v2 2/3] malloc: fix potential out-of-bounds array access

[Van Haaren, Harry]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Anatoly Burakov
> Sent: Wednesday, April 25, 2018 11:16 AM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; Burakov, Anatoly <anatoly.burakov@intel.com>
> Subject: [dpdk-dev] [PATCH v2 2/3] malloc: fix potential out-of-bounds array
> access
> 
> Technically, while the pointer would've been invalid if msl_idx
> were invalid, we wouldn't have actually attempted to access the
> pointer until verifying the index. Fix it by moving array access
> to after we've verified validity of the index.
> 
> Coverity issue: 272574
> 
> Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>


Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Re: [dpdk-dev] [PATCH v2 3/3] malloc: fix potential dereferencing of NULL pointer

[Van Haaren, Harry]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Anatoly Burakov
> Sent: Wednesday, April 25, 2018 11:16 AM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; Burakov, Anatoly <anatoly.burakov@intel.com>
> Subject: [dpdk-dev] [PATCH v2 3/3] malloc: fix potential dereferencing of NULL
> pointer
> 
> Previous code checked for both first/last elements being NULL,
> but if they weren't, the expectation was that they're both
> non-NULL, which will be the case under normal conditions, but
> may not be the case due to heap structure corruption.
> 
> Coverity issue: 272566
> 
> Fixes: bb372060dad4 ("malloc: make heap a doubly-linked list")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

Had to do a double-take there - that's a novel way of checking
pointers - but it actually makes sense here :)

Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Re: [dpdk-dev] [PATCH v2 3/3] malloc: fix potential dereferencing of NULL pointer

[Burakov, Anatoly]

On 27-Apr-18 4:57 PM, Van Haaren, Harry wrote:
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Anatoly Burakov
>> Sent: Wednesday, April 25, 2018 11:16 AM
>> To: dev@dpdk.org
>> Cc: thomas@monjalon.net; Burakov, Anatoly <anatoly.burakov@intel.com>
>> Subject: [dpdk-dev] [PATCH v2 3/3] malloc: fix potential dereferencing of NULL
>> pointer
>>
>> Previous code checked for both first/last elements being NULL,
>> but if they weren't, the expectation was that they're both
>> non-NULL, which will be the case under normal conditions, but
>> may not be the case due to heap structure corruption.
>>
>> Coverity issue: 272566
>>
>> Fixes: bb372060dad4 ("malloc: make heap a doubly-linked list")
>> Cc: anatoly.burakov@intel.com
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> Had to do a double-take there - that's a novel way of checking
> pointers - but it actually makes sense here :)
> 
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
> 

It's basically a logical XOR :)

Thanks for reviewing!

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2 0/3] Coverity fixes for malloc

[Thomas Monjalon]

25/04/2018 12:15, Anatoly Burakov:
> This patchset fixes a few Coverity issues in malloc
> introduced by recent DPDK memory hotplug patchset.
> 
> Coverity issues fixed:
> - 272566 - possible null dereference
> - 272574 - use value before verification
> 
> The following coverity issues were not fixed:
> - 272573 - calling memset with size 0
>   - This is intentional, size will not be 0 in malloc debug
> - 272571 - negative return not handled
>   - False positive, proper API usage ensures no negative returns
> - 272590 - negative return not handled
>   - Same as above
> - 272597 - negative return not handled
>   - Same as above
> 
> Also, replace all instaces of snprintf with strlcpy.
> 
> v2:
> - Dropped fixes for 272571, 272590, 272597 as false positives
> 
> Anatoly Burakov (3):
>   malloc: replace snprintf with strlcpy
>   malloc: fix potential out-of-bounds array access
>   malloc: fix potential dereferencing of NULL pointer

Applied, thanks