[dpdk-dev] [PATCH] cfgfile: fix unitialised buffer and improve reading from nfs filesystem.

[dpdk-dev] [PATCH] cfgfile: fix unitialised buffer and improve reading from nfs filesystem.

[Daniel Mrzyglod]

Nature of the problem was not initialised buffer[256] on special condition
there were probability that program will work on unitialised data that
could provide unexpected program behaviour.

Adding additional transparent I/O buffer for I/O operations
improve reading on heavyloaded enviroments with NFS.

Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
---
 lib/librte_cfgfile/rte_cfgfile.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index b81c273..88fcb46 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -93,10 +93,14 @@ rte_cfgfile_load(const char *filename, int flags)
 	int curr_section = -1;
 	int curr_entry = -1;
 	char buffer[256];
+	char f_streambuff[BUFSIZ];
 	int lineno = 0;
 	struct rte_cfgfile *cfg = NULL;
+	memset(buffer, '\0', 256*sizeof(char));
+	memset(f_streambuff, '\0', BUFSIZ);
 
 	FILE *f = fopen(filename, "r");
+	setbuf(f, f_streambuff);
 	if (f == NULL)
 		return NULL;
 
-- 
2.1.0

Re: [dpdk-dev] [PATCH] cfgfile: fix unitialised buffer and improve reading from nfs filesystem.

[Bruce Richardson]

On Fri, Jun 26, 2015 at 10:37:13AM +0200, Daniel Mrzyglod wrote:
> Nature of the problem was not initialised buffer[256] on special condition
> there were probability that program will work on unitialised data that
> could provide unexpected program behaviour.
> 
> Adding additional transparent I/O buffer for I/O operations
> improve reading on heavyloaded enviroments with NFS.
> 
> Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
> ---
>  lib/librte_cfgfile/rte_cfgfile.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
> index b81c273..88fcb46 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.c
> +++ b/lib/librte_cfgfile/rte_cfgfile.c
> @@ -93,10 +93,14 @@ rte_cfgfile_load(const char *filename, int flags)
>  	int curr_section = -1;
>  	int curr_entry = -1;
>  	char buffer[256];
> +	char f_streambuff[BUFSIZ];
>  	int lineno = 0;
>  	struct rte_cfgfile *cfg = NULL;
> +	memset(buffer, '\0', 256*sizeof(char));
> +	memset(f_streambuff, '\0', BUFSIZ);

Need a blank line before the memsets.
The 256*sizeof(char) needs whitespace around it - however, it's probably best
just replaced by "sizeof(buffer)". For consistency, the same change could be
made to the f_streambuff memset too.

/Bruce

[dpdk-dev] [PATCH v2] cfgfile: fix unitialised buffer

[Daniel Mrzyglod]

Nature of the problem was not initialised buffer[256], there were probability
that operation system will provide previously used memory and on special condition
there were probability that string operations will work on random data that
could provide unexpected program behaviour.

Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
---
 lib/librte_cfgfile/rte_cfgfile.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index b81c273..9c85e9f 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -96,7 +96,10 @@ rte_cfgfile_load(const char *filename, int flags)
 	int lineno = 0;
 	struct rte_cfgfile *cfg = NULL;
 
+	memset(buffer, '\0', sizeof(buffer));
+
 	FILE *f = fopen(filename, "r");
+
 	if (f == NULL)
 		return NULL;
 
-- 
2.1.0

Re: [dpdk-dev] [PATCH v2] cfgfile: fix unitialised buffer

[Bruce Richardson]

On Mon, Jun 29, 2015 at 04:20:25PM +0200, Daniel Mrzyglod wrote:
> Nature of the problem was not initialised buffer[256], there were probability
> that operation system will provide previously used memory and on special condition
> there were probability that string operations will work on random data that
> could provide unexpected program behaviour.
> 
> Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
> ---
>  lib/librte_cfgfile/rte_cfgfile.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
> index b81c273..9c85e9f 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.c
> +++ b/lib/librte_cfgfile/rte_cfgfile.c
> @@ -96,7 +96,10 @@ rte_cfgfile_load(const char *filename, int flags)
>  	int lineno = 0;
>  	struct rte_cfgfile *cfg = NULL;
>  
> +	memset(buffer, '\0', sizeof(buffer));
> +
>  	FILE *f = fopen(filename, "r");
> +
>  	if (f == NULL)
>  		return NULL;
>  
> -- 
> 2.1.0
> 
How about just adding "= {0}" to the end of the definition of buffer?

/Bruce

Re: [dpdk-dev] [PATCH] cfgfile: fix unitialised buffer and improve reading from nfs filesystem.

[Mrzyglod, DanielX T]

I send v2 of this patch. 

I removed in v2 special buffer for file descriptor.
It helped when we haved unitilized buffer[256] because of diferent fgets/fread/fwrite behaviour
when the buffer was set. 

The real and only problem was uninitialized buffer[256] and this workaround IO buffer is not needed for patch.

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Daniel Mrzyglod
> Sent: Friday, June 26, 2015 10:37 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] cfgfile: fix unitialised buffer and improve reading
> from nfs filesystem.
> 
> Nature of the problem was not initialised buffer[256] on special condition
> there were probability that program will work on unitialised data that
> could provide unexpected program behaviour.
> 
> Adding additional transparent I/O buffer for I/O operations
> improve reading on heavyloaded enviroments with NFS.
> 
> Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
> ---
>  lib/librte_cfgfile/rte_cfgfile.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
> index b81c273..88fcb46 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.c
> +++ b/lib/librte_cfgfile/rte_cfgfile.c
> @@ -93,10 +93,14 @@ rte_cfgfile_load(const char *filename, int flags)
>  	int curr_section = -1;
>  	int curr_entry = -1;
>  	char buffer[256];
> +	char f_streambuff[BUFSIZ];
>  	int lineno = 0;
>  	struct rte_cfgfile *cfg = NULL;
> +	memset(buffer, '\0', 256*sizeof(char));
> +	memset(f_streambuff, '\0', BUFSIZ);
> 
>  	FILE *f = fopen(filename, "r");
> +	setbuf(f, f_streambuff);
>  	if (f == NULL)
>  		return NULL;
> 
> --
> 2.1.0

[dpdk-dev] [PATCH v3] cfgfile: fix unitialised buffer

[Daniel Mrzyglod]

Nature of the problem was not initialised buffer[256], there were probability
that operation system will provide previously used memory and on special condition
there were probability that string operations will work on random data that
could provide unexpected program behaviour.

Changes in v3:
-Simplify the initialization of buffer.
Changes in v2:
-Found the real nature of problem. Only buffer was not initilized.
Changes in v1:
-Add additional separate IO buffer and initialize both buffers.

Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
---
 lib/librte_cfgfile/rte_cfgfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index b81c273..a677dad 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -92,7 +92,7 @@ rte_cfgfile_load(const char *filename, int flags)
 	int allocated_entries = 0;
 	int curr_section = -1;
 	int curr_entry = -1;
-	char buffer[256];
+	char buffer[256] = {0};
 	int lineno = 0;
 	struct rte_cfgfile *cfg = NULL;
 
-- 
2.1.0

Re: [dpdk-dev] [PATCH v3] cfgfile: fix unitialised buffer

[Dumitrescu, Cristian]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Daniel Mrzyglod
> Sent: Monday, June 29, 2015 4:06 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3] cfgfile: fix unitialised buffer
> 
> Nature of the problem was not initialised buffer[256], there were probability
> that operation system will provide previously used memory and on special
> condition
> there were probability that string operations will work on random data that
> could provide unexpected program behaviour.
> 
> Changes in v3:
> -Simplify the initialization of buffer.
> Changes in v2:
> -Found the real nature of problem. Only buffer was not initilized.
> Changes in v1:
> -Add additional separate IO buffer and initialize both buffers.
> 
Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Re: [dpdk-dev] [PATCH v3] cfgfile: fix unitialised buffer

[Thomas Monjalon]

2015-06-29 19:47, Dumitrescu, Cristian:
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Daniel Mrzyglod
> > Nature of the problem was not initialised buffer[256], there were probability
> > that operation system will provide previously used memory and on special
> > condition
> > there were probability that string operations will work on random data that
> > could provide unexpected program behaviour.
> > 
> > Changes in v3:
> > -Simplify the initialization of buffer.
> > Changes in v2:
> > -Found the real nature of problem. Only buffer was not initilized.
> > Changes in v1:
> > -Add additional separate IO buffer and initialize both buffers.
> > 
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Applied, thanks