[dpdk-dev] [PATCH] rte_eth_af_packet: refactor error handling to avoid NULL pointer dereference

[dpdk-dev] [PATCH] rte_eth_af_packet: refactor error handling to avoid NULL pointer dereference

[John W. Linville]

Coverity CID # 13321

Checking *internals != NULL before accessing req is not good enough,
because **internals is a function argument and the function doesn't
really know what is passed-in.  We can close our eyes and ignore the
warning on the basis of controlling all the calling code, or we can
refactor the error exit to avoid the issue entirely...

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 drivers/net/af_packet/rte_eth_af_packet.c | 44 +++++++++++++++----------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index bdd9628674cb..cac26e533813 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -454,7 +454,7 @@ rte_pmd_init_internals(const char *name,
 		RTE_LOG(ERR, PMD,
 			"%s: no interface specified for AF_PACKET ethdev\n",
 		        name);
-		goto error;
+		goto error_early;
 	}
 
 	RTE_LOG(INFO, PMD,
@@ -467,16 +467,16 @@ rte_pmd_init_internals(const char *name,
 	 */
 	data = rte_zmalloc_socket(name, sizeof(*data), 0, numa_node);
 	if (data == NULL)
-		goto error;
+		goto error_early;
 
 	pci_dev = rte_zmalloc_socket(name, sizeof(*pci_dev), 0, numa_node);
 	if (pci_dev == NULL)
-		goto error;
+		goto error_early;
 
 	*internals = rte_zmalloc_socket(name, sizeof(**internals),
 	                                0, numa_node);
 	if (*internals == NULL)
-		goto error;
+		goto error_early;
 
 	for (q = 0; q < nb_queues; q++) {
 		(*internals)->rx_queue[q].map = MAP_FAILED;
@@ -498,13 +498,13 @@ rte_pmd_init_internals(const char *name,
 		RTE_LOG(ERR, PMD,
 			"%s: I/F name too long (%s)\n",
 			name, pair->value);
-		goto error;
+		goto error_early;
 	}
 	if (ioctl(sockfd, SIOCGIFINDEX, &ifr) == -1) {
 		RTE_LOG(ERR, PMD,
 			"%s: ioctl failed (SIOCGIFINDEX)\n",
 		        name);
-		goto error;
+		goto error_early;
 	}
 	(*internals)->if_index = ifr.ifr_ifindex;
 
@@ -512,7 +512,7 @@ rte_pmd_init_internals(const char *name,
 		RTE_LOG(ERR, PMD,
 			"%s: ioctl failed (SIOCGIFHWADDR)\n",
 		        name);
-		goto error;
+		goto error_early;
 	}
 	memcpy(&(*internals)->eth_addr, ifr.ifr_hwaddr.sa_data, ETH_ALEN);
 
@@ -678,24 +678,22 @@ rte_pmd_init_internals(const char *name,
 	return 0;
 
 error:
-	rte_free(data);
-	rte_free(pci_dev);
-
-	if (*internals) {
-		for (q = 0; q < nb_queues; q++) {
-			munmap((*internals)->rx_queue[q].map,
-			       2 * req->tp_block_size * req->tp_block_nr);
-
-			rte_free((*internals)->rx_queue[q].rd);
-			rte_free((*internals)->tx_queue[q].rd);
-			if (((*internals)->rx_queue[q].sockfd != 0) &&
-				((*internals)->rx_queue[q].sockfd != qsockfd))
-				close((*internals)->rx_queue[q].sockfd);
-		}
-		rte_free(*internals);
-	}
 	if (qsockfd != -1)
 		close(qsockfd);
+	for (q = 0; q < nb_queues; q++) {
+		munmap((*internals)->rx_queue[q].map,
+		       2 * req->tp_block_size * req->tp_block_nr);
+
+		rte_free((*internals)->rx_queue[q].rd);
+		rte_free((*internals)->tx_queue[q].rd);
+		if (((*internals)->rx_queue[q].sockfd != 0) &&
+			((*internals)->rx_queue[q].sockfd != qsockfd))
+			close((*internals)->rx_queue[q].sockfd);
+	}
+	rte_free(*internals);
+error_early:
+	rte_free(pci_dev);
+	rte_free(data);
 	return -1;
 }
 
-- 
2.4.3

Re: [dpdk-dev] [PATCH] rte_eth_af_packet: refactor error handling to avoid NULL pointer dereference

[Neil Horman]

On Thu, Aug 27, 2015 at 01:17:13PM -0400, John W. Linville wrote:
> Coverity CID # 13321
> 
> Checking *internals != NULL before accessing req is not good enough,
> because **internals is a function argument and the function doesn't
> really know what is passed-in.  We can close our eyes and ignore the
> warning on the basis of controlling all the calling code, or we can
> refactor the error exit to avoid the issue entirely...
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>

Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [dpdk-dev] [PATCH] rte_eth_af_packet: refactor error handling to avoid NULL pointer dereference

[Thomas Monjalon]

2015-08-28 06:27, Neil Horman:
> On Thu, Aug 27, 2015 at 01:17:13PM -0400, John W. Linville wrote:
> > Coverity CID # 13321
> > 
> > Checking *internals != NULL before accessing req is not good enough,
> > because **internals is a function argument and the function doesn't
> > really know what is passed-in.  We can close our eyes and ignore the
> > warning on the basis of controlling all the calling code, or we can
> > refactor the error exit to avoid the issue entirely...
> > 
> > Signed-off-by: John W. Linville <linville@tuxdriver.com>
> 
> Acked-by: Neil Horman <nhorman@tuxdriver.com>

Applied, thanks