Coverity fixes for malloc

PATCH Superseded Anatoly Burakov 4 participants 17 messages Apr 17, 2018 – Apr 27, 2018

lore

Share & tools

lore

Anatoly Burakov

author

2018-04-17 15:48 UTC #1 lore raw

PATCH 1/5 malloc: replace snprintf with strlcpy

In reply to Anatoly Burakov

Signed-off-by Anatoly Burakov <[email protected]>

---
 lib/librte_eal/common/malloc_mp.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

lib/librte_eal/common/malloc_mp.c +11 ~~-12~~

index 72b1f4c..931c14b 100644
@@ -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();

Signature · 1 line

2.7.4

Anatoly Burakov

author

2018-04-17 15:48 UTC #3 lore raw

PATCH 3/5 malloc: fix potential negative return

In reply to 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 [email protected]

Signed-off-by Anatoly Burakov <[email protected]>

---
 lib/librte_eal/common/malloc_heap.c | 4 ++++
 1 file changed, 4 insertions(+)

lib/librte_eal/common/malloc_heap.c +4 -0

index 5cf7231..f81aaf3 100644
@@ -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,

Signature · 1 line

2.7.4

Anatoly Burakov

author

2018-04-17 15:48 UTC #2 lore raw

PATCH 2/5 malloc: fix potential out-of-bounds array access

In reply to 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 [email protected]

Signed-off-by Anatoly Burakov <[email protected]>

---
 lib/librte_eal/common/malloc_heap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

lib/librte_eal/common/malloc_heap.c +2 -1

index 590e9e3..5cf7231 100644
@@ -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,

Signature · 1 line

2.7.4

Anatoly Burakov

author

2018-04-17 15:48 UTC #4 lore raw

PATCH 4/5 malloc: fix potential dereferencing of NULL pointer

In reply to 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 [email protected]

Signed-off-by Anatoly Burakov <[email protected]>

---
 lib/librte_eal/common/malloc_elem.c | 6 ++++++
 1 file changed, 6 insertions(+)

lib/librte_eal/common/malloc_elem.c +6 -0

index ee79dcd..af81961 100644
@@ -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;

Signature · 1 line

2.7.4

Anatoly Burakov

author

2018-04-17 15:48 UTC #5 lore raw

PATCH 5/5 malloc: fix potential negative return

In reply to 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 [email protected]

Signed-off-by Anatoly Burakov <[email protected]>

---
 lib/librte_eal/common/rte_malloc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

lib/librte_eal/common/rte_malloc.c +5 -1

index b51a6d1..f207ba2 100644
@@ -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);
 	}

Signature · 1 line

2.7.4

Anatoly Burakov

author

2018-04-17 15:48 UTC #0 lore raw

cover Coverity fixes for malloc

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(-)

Signature · 1 line

2.7.4

Tan, Jianfeng

2018-04-25 08:24 UTC #6 lore raw

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

In reply to Anatoly Burakov

Quoted 22 lines · 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 [email protected]
Signed-off-by Anatoly Burakov <[email protected]>
---
  lib/librte_eal/common/rte_malloc.c | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)
lib/librte_eal/common/rte_malloc.c +0 -0
index b51a6d1..f207ba2 100644

lib/librte_eal/common/rte_malloc.c rte_malloc_dump_heaps(FILE *f) · line 176

Quoted 9 lines

lib/librte_eal/common/rte_malloc.c +5 -1

@@ -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

Quoted 3 lines

lib/librte_eal/common/rte_malloc.c +0 -0

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

Burakov, Anatoly

author

2018-04-25 08:50 UTC #7 lore raw

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

Quoted 20 lines · Jianfeng

Quoted 17 lines · 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 [email protected]
Signed-off-by Anatoly Burakov <[email protected]>
---
lib/librte_eal/common/rte_malloc.c | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

Quoted 15 lines

lib/librte_eal/common/rte_malloc.c rte_malloc_dump_heaps(FILE *f) · line 175
Quoted 13 lines
lib/librte_eal/common/rte_malloc.c +5 -1
index b51a6d1..f207ba2 100644
@@ -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.

Quoted 9 lines

Thanks,
Jianfeng

Quoted 3 lines

lib/librte_eal/common/rte_malloc.c +0 -0

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

Signature · 2 lines

Thanks,
Anatoly

Anatoly Burakov

author

2018-04-25 10:15 UTC #8 lore raw

v2 cover Coverity fixes for malloc

In reply to 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(-)

Signature · 1 line

2.7.4

Anatoly Burakov

author

2018-04-25 10:15 UTC #9 lore raw

PATCH v2 1/3 malloc: replace snprintf with strlcpy

Signed-off-by Anatoly Burakov <[email protected]>

---
 lib/librte_eal/common/malloc_mp.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

lib/librte_eal/common/malloc_mp.c +11 ~~-12~~

index 72b1f4c..931c14b 100644
@@ -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();

Signature · 1 line

2.7.4

Anatoly Burakov

author

2018-04-25 10:15 UTC #11 lore raw

PATCH v2 2/3 malloc: fix potential out-of-bounds array access

In reply to Anatoly Burakov

Coverity issue: 272574

Fixes 66cc45e293ed ("mem: replace memseg with memseg lists")

Cc [email protected]

Signed-off-by Anatoly Burakov <[email protected]>

---
 lib/librte_eal/common/malloc_heap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

lib/librte_eal/common/malloc_heap.c +2 -1

index 590e9e3..5cf7231 100644
@@ -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,

Signature · 1 line

2.7.4

Anatoly Burakov

author

2018-04-25 10:15 UTC #13 lore raw

PATCH v2 3/3 malloc: fix potential dereferencing of NULL pointer

In reply to Anatoly Burakov

Coverity issue: 272566

Fixes bb372060dad4 ("malloc: make heap a doubly-linked list")

Cc [email protected]

Signed-off-by Anatoly Burakov <[email protected]>

---
 lib/librte_eal/common/malloc_elem.c | 6 ++++++
 1 file changed, 6 insertions(+)

lib/librte_eal/common/malloc_elem.c +6 -0

index ee79dcd..af81961 100644
@@ -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;

Signature · 1 line

2.7.4

Van Haaren, Harry

Acked-by on v2 1/3

2018-04-27 15:57 UTC #10 lore raw

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

In reply to Anatoly Burakov

Quoted 7 lines

From: dev [mailto:[email protected]] On Behalf Of Anatoly Burakov
Sent: Wednesday, April 25, 2018 11:16 AM
To: [email protected]
Cc: [email protected]
Subject: [dpdk-dev] [PATCH v2 1/3] malloc: replace snprintf with strlcpy
Signed-off-by Anatoly Burakov <[email protected]>

Acked-by Harry van Haaren <[email protected]>

Van Haaren, Harry

Acked-by on v2 2/3

2018-04-27 15:57 UTC #12 lore raw

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

In reply to Anatoly Burakov

Quoted 12 lines

From: dev [mailto:[email protected]] On Behalf Of Anatoly Burakov
Sent: Wednesday, April 25, 2018 11:16 AM
To: [email protected]
Cc: [email protected]; Burakov, Anatoly <[email protected]>
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.

Quoted 6 lines

Coverity issue: 272574
Fixes 66cc45e293ed ("mem: replace memseg with memseg lists")
Cc [email protected]
Signed-off-by Anatoly Burakov <[email protected]>

Acked-by Harry van Haaren <[email protected]>

Van Haaren, Harry

Acked-by on v2 3/3

2018-04-27 15:57 UTC #14 lore raw

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

In reply to Anatoly Burakov

Quoted 12 lines

From: dev [mailto:[email protected]] On Behalf Of Anatoly Burakov
Sent: Wednesday, April 25, 2018 11:16 AM
To: [email protected]
Cc: [email protected]; Burakov, Anatoly <[email protected]>
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.

Quoted 6 lines

Coverity issue: 272566
Fixes bb372060dad4 ("malloc: make heap a doubly-linked list")
Cc [email protected]
Signed-off-by Anatoly Burakov <[email protected]>

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 <[email protected]>

Burakov, Anatoly

author

2018-04-27 16:02 UTC #15 lore raw

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

Quoted 17 lines · Harry

Quoted 17 lines
From: dev [mailto:[email protected]] On Behalf Of Anatoly Burakov
Sent: Wednesday, April 25, 2018 11:16 AM
To: [email protected]
Cc: [email protected]; Burakov, Anatoly <[email protected]>
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 [email protected]

Quoted 7 lines

Quoted 1 line
Signed-off-by Anatoly Burakov <[email protected]>
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 <[email protected]>

It's basically a logical XOR :)

Thanks for reviewing!

Signature · 2 lines

Thanks,
Anatoly

Thomas Monjalon

Applied

2018-04-27 21:33 UTC #16 lore raw

In reply to Anatoly Burakov

25/04/2018 12:15, Anatoly Burakov:

Quoted 19 lines

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.

Quoted 7 lines

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