[PATCHv3,2/2] config: Fix memory initialization

Submitted by Radostin Stoyanov on Jan. 22, 2019, 9:12 a.m.

Details

Message ID 20190122091240.9995-2-rstoyanov1@gmail.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Radostin Stoyanov Jan. 22, 2019, 9:12 a.m.
Initialize allocated memory when parsing configuration file.

v3: Use calloc() instead of malloc() followed by memset()

Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
---
 criu/config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/criu/config.c b/criu/config.c
index 89afdd748..a0bfae10a 100644
--- a/criu/config.c
+++ b/criu/config.c
@@ -54,7 +54,7 @@  static char ** parse_config(char *filepath)
 	if (!configfile) {
 		return NULL;
 	}
-	configuration = xmalloc(config_size * sizeof(char *));
+	configuration = xzalloc(config_size * sizeof(char *));
 	if (configuration == NULL) {
 		fclose(configfile);
 		exit(1);

Comments

Andrei Vagin Jan. 24, 2019, 5 p.m.
On Tue, Jan 22, 2019 at 09:12:40AM +0000, Radostin Stoyanov wrote:
> Initialize allocated memory when parsing configuration file.
> 
> v3: Use calloc() instead of malloc() followed by memset()
> 
> Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
> ---
>  criu/config.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/criu/config.c b/criu/config.c
> index 89afdd748..a0bfae10a 100644
> --- a/criu/config.c
> +++ b/criu/config.c
> @@ -54,7 +54,7 @@ static char ** parse_config(char *filepath)
>  	if (!configfile) {
>  		return NULL;
>  	}
> -	configuration = xmalloc(config_size * sizeof(char *));
> +	configuration = xzalloc(config_size * sizeof(char *));

Why does it have to be initialized?

>  	if (configuration == NULL) {
>  		fclose(configfile);
>  		exit(1);
> -- 
> 2.20.1
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Andrei Vagin Jan. 24, 2019, 5:08 p.m.
On Thu, Jan 24, 2019 at 09:00:46AM -0800, Andrei Vagin wrote:
> On Tue, Jan 22, 2019 at 09:12:40AM +0000, Radostin Stoyanov wrote:
> > Initialize allocated memory when parsing configuration file.
> > 
> > v3: Use calloc() instead of malloc() followed by memset()
> > 
> > Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
> > ---
> >  criu/config.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/criu/config.c b/criu/config.c
> > index 89afdd748..a0bfae10a 100644
> > --- a/criu/config.c
> > +++ b/criu/config.c
> > @@ -54,7 +54,7 @@ static char ** parse_config(char *filepath)
> >  	if (!configfile) {
> >  		return NULL;
> >  	}
> > -	configuration = xmalloc(config_size * sizeof(char *));
> > +	configuration = xzalloc(config_size * sizeof(char *));
> 
> Why does it have to be initialized?

There is  xrealloc, which doesn't initialize memory.

I think you need to set only the last element of this array to NULL.

I don't like a style of this function. I think we need to split it on
smaller parts and write more comments. We have a lot of if-s there, I
would like to have a comment before mostly each of them.

Thanks,
AV

> 
> >  	if (configuration == NULL) {
> >  		fclose(configfile);
> >  		exit(1);
> > -- 
> > 2.20.1
> > 
> > _______________________________________________
> > CRIU mailing list
> > CRIU@openvz.org
> > https://lists.openvz.org/mailman/listinfo/criu
Radostin Stoyanov Jan. 24, 2019, 6:39 p.m.
On 24/01/2019 17:08, Andrei Vagin wrote:
> On Thu, Jan 24, 2019 at 09:00:46AM -0800, Andrei Vagin wrote:
>> On Tue, Jan 22, 2019 at 09:12:40AM +0000, Radostin Stoyanov wrote:
>>> Initialize allocated memory when parsing configuration file.
>>>
>>> v3: Use calloc() instead of malloc() followed by memset()
>>>
>>> Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
>>> ---
>>>  criu/config.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/criu/config.c b/criu/config.c
>>> index 89afdd748..a0bfae10a 100644
>>> --- a/criu/config.c
>>> +++ b/criu/config.c
>>> @@ -54,7 +54,7 @@ static char ** parse_config(char *filepath)
>>>  	if (!configfile) {
>>>  		return NULL;
>>>  	}
>>> -	configuration = xmalloc(config_size * sizeof(char *));
>>> +	configuration = xzalloc(config_size * sizeof(char *));
>> Why does it have to be initialized?

I tried to describe why in
https://lists.openvz.org/pipermail/criu/2019-January/043307.html
but it is a good point that the description should be in the commit message.

> There is  xrealloc, which doesn't initialize memory.
>
> I think you need to set only the last element of this array to NULL.
>
> I don't like a style of this function. I think we need to split it on
> smaller parts and write more comments. We have a lot of if-s there, I
> would like to have a comment before mostly each of them.
I will send a "refactor configuration file parsing" patch in which
we could split the function into smaller parts and add comments.

Radostin
Andrei Vagin Feb. 1, 2019, 5:49 p.m.
On Thu, Jan 24, 2019 at 06:39:24PM +0000, Radostin Stoyanov wrote:
> On 24/01/2019 17:08, Andrei Vagin wrote:
> > On Thu, Jan 24, 2019 at 09:00:46AM -0800, Andrei Vagin wrote:
> >> On Tue, Jan 22, 2019 at 09:12:40AM +0000, Radostin Stoyanov wrote:
> >>> Initialize allocated memory when parsing configuration file.
> >>>
> >>> v3: Use calloc() instead of malloc() followed by memset()
> >>>
> >>> Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>
> >>> ---
> >>>  criu/config.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/criu/config.c b/criu/config.c
> >>> index 89afdd748..a0bfae10a 100644
> >>> --- a/criu/config.c
> >>> +++ b/criu/config.c
> >>> @@ -54,7 +54,7 @@ static char ** parse_config(char *filepath)
> >>>  	if (!configfile) {
> >>>  		return NULL;
> >>>  	}
> >>> -	configuration = xmalloc(config_size * sizeof(char *));
> >>> +	configuration = xzalloc(config_size * sizeof(char *));
> >> Why does it have to be initialized?
> 
> I tried to describe why in
> https://lists.openvz.org/pipermail/criu/2019-January/043307.html
> but it is a good point that the description should be in the commit message.

Pls update the commit message and resend this patch. Thanks.

> 
> > There is  xrealloc, which doesn't initialize memory.
> >
> > I think you need to set only the last element of this array to NULL.
> >
> > I don't like a style of this function. I think we need to split it on
> > smaller parts and write more comments. We have a lot of if-s there, I
> > would like to have a comment before mostly each of them.
> I will send a "refactor configuration file parsing" patch in which
> we could split the function into smaller parts and add comments.
> 
> Radostin