diff mbox series

Fix off-by-one errors in snprintf in various files

Message ID 20190510145312.409-1-msantana@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series Fix off-by-one errors in snprintf in various files |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Michael Santana May 10, 2019, 2:53 p.m. UTC 
  snprintf guarantees to always correctly place a null terminator in the buffer
string. So manually placing a null terminator in a buffer right after a call
to snprintf is redundant code.

Additionally, there is no need to use 'sizeof(buffer) - 1' in snprintf as this
means we are not using the last character in the buffer. 'sizeof(buffer)' is
enough.

Cc: stable@dpdk.org

Signed-off-by: Michael Santana <msantana@redhat.com>
---
 drivers/net/qede/base/bcm_osal.c                        | 4 ++--
 drivers/net/qede/qede_filter.c                          | 2 +-
 drivers/net/vdev_netvsc/vdev_netvsc.c                   | 2 +-
 examples/multi_process/client_server_mp/shared/common.h | 2 +-
 examples/server_node_efd/shared/common.h                | 2 +-
 lib/librte_eal/common/eal_common_options.c              | 3 +--
 lib/librte_eal/common/eal_filesystem.h                  | 9 ++++-----
 lib/librte_eal/common/malloc_heap.c                     | 4 ++--
 8 files changed, 13 insertions(+), 15 deletions(-)

Comments

Bruce Richardson May 10, 2019, 2:55 p.m. UTC | #1 
On Fri, May 10, 2019 at 10:53:12AM -0400, Michael Santana wrote:
> snprintf guarantees to always correctly place a null terminator in the buffer
> string. So manually placing a null terminator in a buffer right after a call
> to snprintf is redundant code.
> 
> Additionally, there is no need to use 'sizeof(buffer) - 1' in snprintf as this
> means we are not using the last character in the buffer. 'sizeof(buffer)' is
> enough.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Michael Santana <msantana@redhat.com>
> ---
>  drivers/net/qede/base/bcm_osal.c                        | 4 ++--
>  drivers/net/qede/qede_filter.c                          | 2 +-
>  drivers/net/vdev_netvsc/vdev_netvsc.c                   | 2 +-
>  examples/multi_process/client_server_mp/shared/common.h | 2 +-
>  examples/server_node_efd/shared/common.h                | 2 +-
>  lib/librte_eal/common/eal_common_options.c              | 3 +--
>  lib/librte_eal/common/eal_filesystem.h                  | 9 ++++-----
>  lib/librte_eal/common/malloc_heap.c                     | 4 ++--
>  8 files changed, 13 insertions(+), 15 deletions(-)
> 
Looks a good cleanup, thanks.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Burakov, Anatoly May 10, 2019, 3:28 p.m. UTC | #2 
On 10-May-19 3:53 PM, Michael Santana wrote:
> snprintf guarantees to always correctly place a null terminator in the buffer
> string. So manually placing a null terminator in a buffer right after a call
> to snprintf is redundant code.
> 
> Additionally, there is no need to use 'sizeof(buffer) - 1' in snprintf as this
> means we are not using the last character in the buffer. 'sizeof(buffer)' is
> enough.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Michael Santana <msantana@redhat.com>
> ---

LGTM

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
Michael Santana May 28, 2019, 1:33 p.m. UTC | #3 
On 5/10/19 11:28 AM, Burakov, Anatoly wrote:
> On 10-May-19 3:53 PM, Michael Santana wrote:
>> snprintf guarantees to always correctly place a null terminator in 
>> the buffer
>> string. So manually placing a null terminator in a buffer right after 
>> a call
>> to snprintf is redundant code.
>>
>> Additionally, there is no need to use 'sizeof(buffer) - 1' in 
>> snprintf as this
>> means we are not using the last character in the buffer. 
>> 'sizeof(buffer)' is
>> enough.
>>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Michael Santana <msantana@redhat.com>
>> ---
>
> LGTM
>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
>
ping
Thomas Monjalon May 29, 2019, 10:51 a.m. UTC | #4 
10/05/2019 16:55, Bruce Richardson:
> On Fri, May 10, 2019 at 10:53:12AM -0400, Michael Santana wrote:
> > snprintf guarantees to always correctly place a null terminator in the buffer
> > string. So manually placing a null terminator in a buffer right after a call
> > to snprintf is redundant code.
> > 
> > Additionally, there is no need to use 'sizeof(buffer) - 1' in snprintf as this
> > means we are not using the last character in the buffer. 'sizeof(buffer)' is
> > enough.
> > 
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Michael Santana <msantana@redhat.com>
> > 
> Looks a good cleanup, thanks.
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Applied, thanks
diff mbox series

Patch

diff --git a/drivers/net/qede/base/bcm_osal.c b/drivers/net/qede/base/bcm_osal.c
index 693328f11..9915df44f 100644
--- a/drivers/net/qede/base/bcm_osal.c
+++ b/drivers/net/qede/base/bcm_osal.c
@@ -128,7 +128,7 @@  void *osal_dma_alloc_coherent(struct ecore_dev *p_dev,
 	}
 
 	OSAL_MEM_ZERO(mz_name, sizeof(*mz_name));
-	snprintf(mz_name, sizeof(mz_name) - 1, "%lx",
+	snprintf(mz_name, sizeof(mz_name), "%lx",
 					(unsigned long)rte_get_timer_cycles());
 	if (core_id == (unsigned int)LCORE_ID_ANY)
 		core_id = rte_get_master_lcore();
@@ -167,7 +167,7 @@  void *osal_dma_alloc_coherent_aligned(struct ecore_dev *p_dev,
 	}
 
 	OSAL_MEM_ZERO(mz_name, sizeof(*mz_name));
-	snprintf(mz_name, sizeof(mz_name) - 1, "%lx",
+	snprintf(mz_name, sizeof(mz_name), "%lx",
 					(unsigned long)rte_get_timer_cycles());
 	if (core_id == (unsigned int)LCORE_ID_ANY)
 		core_id = rte_get_master_lcore();
diff --git a/drivers/net/qede/qede_filter.c b/drivers/net/qede/qede_filter.c
index 7aeef700d..9b4d77777 100644
--- a/drivers/net/qede/qede_filter.c
+++ b/drivers/net/qede/qede_filter.c
@@ -290,7 +290,7 @@  qede_config_arfs_filter(struct rte_eth_dev *eth_dev,
 	/* soft_id could have been used as memzone string, but soft_id is
 	 * not currently used so it has no significance.
 	 */
-	snprintf(mz_name, sizeof(mz_name) - 1, "%lx",
+	snprintf(mz_name, sizeof(mz_name), "%lx",
 		 (unsigned long)rte_get_timer_cycles());
 	mz = rte_memzone_reserve_aligned(mz_name, QEDE_MAX_FDIR_PKT_LEN,
 					 SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c b/drivers/net/vdev_netvsc/vdev_netvsc.c
index 801f54c96..eee242b5c 100644
--- a/drivers/net/vdev_netvsc/vdev_netvsc.c
+++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
@@ -330,7 +330,7 @@  vdev_netvsc_sysfs_readlink(char *buf, size_t size, const char *if_name,
 	char in[RTE_MAX(sizeof(ctx->yield), 256u)];
 	int ret;
 
-	ret = snprintf(in, sizeof(in) - 1, "/sys/class/net/%s/%s",
+	ret = snprintf(in, sizeof(in), "/sys/class/net/%s/%s",
 		       if_name, relpath);
 	if (ret == -1 || (size_t)ret >= sizeof(in))
 		return -ENOBUFS;
diff --git a/examples/multi_process/client_server_mp/shared/common.h b/examples/multi_process/client_server_mp/shared/common.h
index ac9175524..6dd43fcac 100644
--- a/examples/multi_process/client_server_mp/shared/common.h
+++ b/examples/multi_process/client_server_mp/shared/common.h
@@ -49,7 +49,7 @@  get_rx_queue_name(unsigned id)
 	 * by maximum 3 digits (plus an extra byte for safety) */
 	static char buffer[sizeof(MP_CLIENT_RXQ_NAME) + 2];
 
-	snprintf(buffer, sizeof(buffer) - 1, MP_CLIENT_RXQ_NAME, id);
+	snprintf(buffer, sizeof(buffer), MP_CLIENT_RXQ_NAME, id);
 	return buffer;
 }
 
diff --git a/examples/server_node_efd/shared/common.h b/examples/server_node_efd/shared/common.h
index b8b533d8c..130fd4f4e 100644
--- a/examples/server_node_efd/shared/common.h
+++ b/examples/server_node_efd/shared/common.h
@@ -61,7 +61,7 @@  get_rx_queue_name(unsigned int id)
 	 */
 	static char buffer[sizeof(MP_NODE_RXQ_NAME) + 2];
 
-	snprintf(buffer, sizeof(buffer) - 1, MP_NODE_RXQ_NAME, id);
+	snprintf(buffer, sizeof(buffer), MP_NODE_RXQ_NAME, id);
 	return buffer;
 }
 
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 0c91024c4..512d5088e 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -260,8 +260,7 @@  eal_plugindir_init(const char *path)
 	while ((dent = readdir(d)) != NULL) {
 		struct stat sb;
 
-		snprintf(sopath, PATH_MAX-1, "%s/%s", path, dent->d_name);
-		sopath[PATH_MAX-1] = 0;
+		snprintf(sopath, sizeof(sopath), "%s/%s", path, dent->d_name);
 
 		if (!(stat(sopath, &sb) == 0 && S_ISREG(sb.st_mode)))
 			continue;
diff --git a/lib/librte_eal/common/eal_filesystem.h b/lib/librte_eal/common/eal_filesystem.h
index f2f83e712..5d21f07c2 100644
--- a/lib/librte_eal/common/eal_filesystem.h
+++ b/lib/librte_eal/common/eal_filesystem.h
@@ -38,7 +38,7 @@  eal_runtime_config_path(void)
 {
 	static char buffer[PATH_MAX]; /* static so auto-zeroed */
 
-	snprintf(buffer, sizeof(buffer) - 1, "%s/%s", rte_eal_get_runtime_dir(),
+	snprintf(buffer, sizeof(buffer), "%s/%s", rte_eal_get_runtime_dir(),
 			RUNTIME_CONFIG_FNAME);
 	return buffer;
 }
@@ -50,7 +50,7 @@  eal_mp_socket_path(void)
 {
 	static char buffer[PATH_MAX]; /* static so auto-zeroed */
 
-	snprintf(buffer, sizeof(buffer) - 1, "%s/%s", rte_eal_get_runtime_dir(),
+	snprintf(buffer, sizeof(buffer), "%s/%s", rte_eal_get_runtime_dir(),
 			MP_SOCKET_FNAME);
 	return buffer;
 }
@@ -70,7 +70,7 @@  eal_hugepage_info_path(void)
 {
 	static char buffer[PATH_MAX]; /* static so auto-zeroed */
 
-	snprintf(buffer, sizeof(buffer) - 1, "%s/%s", rte_eal_get_runtime_dir(),
+	snprintf(buffer, sizeof(buffer), "%s/%s", rte_eal_get_runtime_dir(),
 			HUGEPAGE_INFO_FNAME);
 	return buffer;
 }
@@ -82,7 +82,7 @@  eal_hugepage_data_path(void)
 {
 	static char buffer[PATH_MAX]; /* static so auto-zeroed */
 
-	snprintf(buffer, sizeof(buffer) - 1, "%s/%s", rte_eal_get_runtime_dir(),
+	snprintf(buffer, sizeof(buffer), "%s/%s", rte_eal_get_runtime_dir(),
 			HUGEPAGE_DATA_FNAME);
 	return buffer;
 }
@@ -94,7 +94,6 @@  eal_get_hugefile_path(char *buffer, size_t buflen, const char *hugedir, int f_id
 {
 	snprintf(buffer, buflen, HUGEFILE_FMT, hugedir,
 			eal_get_hugefile_prefix(), f_id);
-	buffer[buflen - 1] = '\0';
 	return buffer;
 }
 
diff --git a/lib/librte_eal/common/malloc_heap.c b/lib/librte_eal/common/malloc_heap.c
index c5d254d8a..f9235932e 100644
--- a/lib/librte_eal/common/malloc_heap.c
+++ b/lib/librte_eal/common/malloc_heap.c
@@ -1118,7 +1118,7 @@  malloc_heap_create_external_seg(void *va_addr, rte_iova_t iova_addrs[],
 		return NULL;
 	}
 
-	snprintf(fbarray_name, sizeof(fbarray_name) - 1, "%s_%p",
+	snprintf(fbarray_name, sizeof(fbarray_name), "%s_%p",
 			seg_name, va_addr);
 
 	/* create the backing fbarray */
@@ -1334,7 +1334,7 @@  rte_eal_malloc_heap_init(void)
 			char heap_name[RTE_HEAP_NAME_MAX_LEN];
 			int socket_id = rte_socket_id_by_idx(i);
 
-			snprintf(heap_name, sizeof(heap_name) - 1,
+			snprintf(heap_name, sizeof(heap_name),
 					"socket_%i", socket_id);
 			strlcpy(heap->name, heap_name, RTE_HEAP_NAME_MAX_LEN);
 			heap->socket_id = socket_id;