diff mbox

[dpdk-dev,5/5] Fix usage of fgets in various places

Message ID 1424700600-1765-6-git-send-email-pawelx.wodkowski@intel.com (mailing list archive)
State Rejected, archived
Headers

Commit Message

Wodkowski, PawelX Feb. 23, 2015, 2:10 p.m. UTC 
  Declaration of fgets() is
char *fgets(char *str, int size, FILE *stream);

Klocwork complain about passing "sizeof()" as size parameter since
implicit casting size_t to int might cause loss of precision.

Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
---
 lib/librte_cfgfile/rte_cfgfile.c                |  2 +-
 lib/librte_eal/bsdapp/eal/eal.c                 |  2 +-
 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c |  4 ++--
 lib/librte_eal/linuxapp/eal/eal_memory.c        |  2 +-
 lib/librte_eal/linuxapp/eal/eal_pci.c           |  2 +-
 lib/librte_eal/linuxapp/eal/eal_timer.c         |  2 +-
 lib/librte_pmd_virtio/virtio_ethdev.c           |  2 +-
 lib/librte_power/rte_power_acpi_cpufreq.c       | 10 +++++-----
 8 files changed, 13 insertions(+), 13 deletions(-)

Comments

Stephen Hemminger Feb. 23, 2015, 4 p.m. UTC | #1 
On Mon, 23 Feb 2015 15:10:00 +0100
Pawel Wodkowski <pawelx.wodkowski@intel.com> wrote:

> Declaration of fgets() is
> char *fgets(char *str, int size, FILE *stream);
> 
> Klocwork complain about passing "sizeof()" as size parameter since
> implicit casting size_t to int might cause loss of precision.
> 
> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>

NAK this is shooting at Unicorns.
The tool is the problem not the code.
Panu Matilainen Feb. 24, 2015, 9:20 a.m. UTC | #2 
On 02/23/2015 06:00 PM, Stephen Hemminger wrote:
> On Mon, 23 Feb 2015 15:10:00 +0100
> Pawel Wodkowski <pawelx.wodkowski@intel.com> wrote:
>
>> Declaration of fgets() is
>> char *fgets(char *str, int size, FILE *stream);
>>
>> Klocwork complain about passing "sizeof()" as size parameter since
>> implicit casting size_t to int might cause loss of precision.
>>
>> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
>
> NAK this is shooting at Unicorns.
> The tool is the problem not the code.

The tool is technically correct, even if loss of precision might be 
unlikely to occur in this context.

However the patch is incorrect, the problem doesn't go away by adding an 
explict cast even if it shuts up the tool.

	- Panu -
Stephen Hemminger Feb. 24, 2015, 7:01 p.m. UTC | #3 
On Tue, 24 Feb 2015 11:20:33 +0200
Panu Matilainen <pmatilai@redhat.com> wrote:

> The tool is technically correct, even if loss of precision might be 
> unlikely to occur in this context

Overflow is not there in the code.
That is why I said "shooting Unicorns"; this is all about
about fixing bugs that don't exist because there is nothing there
in the real world.

In this code buffer is always something normal in size and does
not exceed 2^32-1.
Panu Matilainen Feb. 25, 2015, 5:37 a.m. UTC | #4 
On 02/24/2015 09:01 PM, Stephen Hemminger wrote:
> On Tue, 24 Feb 2015 11:20:33 +0200
> Panu Matilainen <pmatilai@redhat.com> wrote:
>
>> The tool is technically correct, even if loss of precision might be
>> unlikely to occur in this context
>
> Overflow is not there in the code.
> That is why I said "shooting Unicorns"; this is all about
> about fixing bugs that don't exist because there is nothing there
> in the real world.
>
> In this code buffer is always something normal in size and does
> not exceed 2^32-1.

Oh, if the tool is complaining about fixed-size buffers then yeah the 
the tool is being silly, sorry I didn't actually look up the cases.

	- Panu -
diff mbox

Patch

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index b81c273..15ef447 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -107,7 +107,7 @@  rte_cfgfile_load(const char *filename, int flags)
 
 	memset(cfg->sections, 0, sizeof(cfg->sections[0]) * allocated_sections);
 
-	while (fgets(buffer, sizeof(buffer), f) != NULL) {
+	while (fgets(buffer, (int)sizeof(buffer), f) != NULL) {
 		char *pos = NULL;
 		size_t len = strnlen(buffer, sizeof(buffer));
 		lineno++;
diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 69f3c03..ca51868 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -134,7 +134,7 @@  eal_parse_sysfs_value(const char *filename, unsigned long *val)
 		return -1;
 	}
 
-	if (fgets(buf, sizeof(buf), f) == NULL) {
+	if (fgets(buf, (int)sizeof(buf), f) == NULL) {
 		RTE_LOG(ERR, EAL, "%s(): cannot read sysfs value %s\n",
 			__func__, filename);
 		fclose(f);
diff --git a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
index 590cb56..551472c 100644
--- a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
+++ b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
@@ -115,7 +115,7 @@  get_default_hp_size(void)
 	FILE *fd = fopen(proc_meminfo, "r");
 	if (fd == NULL)
 		rte_panic("Cannot open %s\n", proc_meminfo);
-	while(fgets(buffer, sizeof(buffer), fd)){
+	while (fgets(buffer, (int)sizeof(buffer), fd)) {
 		if (strncmp(buffer, str_hugepagesz, hugepagesz_len) == 0){
 			size = rte_str_to_size(&buffer[hugepagesz_len]);
 			break;
@@ -155,7 +155,7 @@  get_hugepage_dir(uint64_t hugepage_sz)
 	if (default_size == 0)
 		default_size = get_default_hp_size();
 
-	while (fgets(buf, sizeof(buf), fd)){
+	while (fgets(buf, (int)sizeof(buf), fd)) {
 		if (rte_strsplit(buf, sizeof(buf), splitstr, _FIELDNAME_MAX,
 				split_tok) != _FIELDNAME_MAX) {
 			RTE_LOG(ERR, EAL, "Error parsing %s\n", proc_mounts);
diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index a67a1b0..0c7f8ce 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -614,7 +614,7 @@  find_numasocket(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi)
 			"%s/%s", hpi->hugedir, internal_config.hugefile_prefix);
 
 	/* parse numa map */
-	while (fgets(buf, sizeof(buf), f) != NULL) {
+	while (fgets(buf, (int)sizeof(buf), f) != NULL) {
 
 		/* ignore non huge page */
 		if (strstr(buf, " huge ") == NULL &&
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 63bcbce..ee4e1d8 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -162,7 +162,7 @@  pci_parse_sysfs_resource(const char *filename, struct rte_pci_device *dev)
 
 	for (i = 0; i<PCI_MAX_RESOURCE; i++) {
 
-		if (fgets(buf, sizeof(buf), f) == NULL) {
+		if (fgets(buf, (int)sizeof(buf), f) == NULL) {
 			RTE_LOG(ERR, EAL,
 				"%s(): cannot read resource\n", __func__);
 			goto error;
diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c b/lib/librte_eal/linuxapp/eal/eal_timer.c
index ca57916..d5419b9 100644
--- a/lib/librte_eal/linuxapp/eal/eal_timer.c
+++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
@@ -253,7 +253,7 @@  check_tsc_flags(void)
 		return;
 	}
 
-	while (fgets(line, sizeof line, stream)) {
+	while (fgets(line, (int)sizeof(line), stream)) {
 		char *constant_tsc;
 		char *nonstop_tsc;
 
diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c
index 9eb0217..0cd0665 100644
--- a/lib/librte_pmd_virtio/virtio_ethdev.c
+++ b/lib/librte_pmd_virtio/virtio_ethdev.c
@@ -832,7 +832,7 @@  parse_sysfs_value(const char *filename, unsigned long *val)
 		return -1;
 	}
 
-	if (fgets(buf, sizeof(buf), f) == NULL) {
+	if (fgets(buf, (int)sizeof(buf), f) == NULL) {
 		PMD_INIT_LOG(ERR, "%s(): cannot read sysfs value %s",
 			     __func__, filename);
 		fclose(f);
diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c b/lib/librte_power/rte_power_acpi_cpufreq.c
index a56c9b5..a5b46bb 100644
--- a/lib/librte_power/rte_power_acpi_cpufreq.c
+++ b/lib/librte_power/rte_power_acpi_cpufreq.c
@@ -164,7 +164,7 @@  power_set_governor_userspace(struct rte_power_info *pi)
 	f = fopen(fullpath, "rw+");
 	FOPEN_OR_ERR_RET(f, ret);
 
-	s = fgets(buf, sizeof(buf), f);
+	s = fgets(buf, (int)sizeof(buf), f);
 	FOPS_OR_NULL_GOTO(s, out);
 
 	/* Check if current governor is userspace */
@@ -214,7 +214,7 @@  power_get_available_freqs(struct rte_power_info *pi)
 	f = fopen(fullpath, "r");
 	FOPEN_OR_ERR_RET(f, ret);
 
-	s = fgets(buf, sizeof(buf), f);
+	s = fgets(buf, (int)sizeof(buf), f);
 	FOPS_OR_NULL_GOTO(s, out);
 
 	/* Strip the line break if there is */
@@ -223,7 +223,7 @@  power_get_available_freqs(struct rte_power_info *pi)
 		*p = 0;
 
 	/* Split string into at most RTE_MAX_LCORE_FREQS frequencies */
-	count = rte_strsplit(buf, sizeof(buf), freqs,
+	count = rte_strsplit(buf, (int)sizeof(buf), freqs,
 			RTE_MAX_LCORE_FREQS, ' ');
 	if (count <= 0) {
 		RTE_LOG(ERR, POWER, "No available frequency in "
@@ -270,7 +270,7 @@  power_init_for_setting_freq(struct rte_power_info *pi)
 	f = fopen(fullpath, "rw+");
 	FOPEN_OR_ERR_RET(f, -1);
 
-	s = fgets(buf, sizeof(buf), f);
+	s = fgets(buf, (int)sizeof(buf), f);
 	FOPS_OR_NULL_GOTO(s, out);
 
 	freq = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
@@ -367,7 +367,7 @@  power_set_governor_original(struct rte_power_info *pi)
 	f = fopen(fullpath, "rw+");
 	FOPEN_OR_ERR_RET(f, ret);
 
-	s = fgets(buf, sizeof(buf), f);
+	s = fgets(buf, (int)sizeof(buf), f);
 	FOPS_OR_NULL_GOTO(s, out);
 
 	/* Check if the governor to be set is the same as current */