[4/4] multipath: Fix a potential buffer overflow

Commit Message

Martin Wilck June 13, 2017, 7:28 p.m. UTC

Hi Bart,

On Tue, 2017-06-13 at 09:33 -0700, Bart Van Assche wrote:
> Avoid that gcc 7 reports the following warning:
> 
> cli_handlers.c:1340:18: warning: '%d' directive writing between 1 and
> 3 bytes into a region of size 2 [-Wformat-overflow=]
>   sprintf(*reply,"%d",mpp->prflag);
> 
> Signed-off-by: Bart Van Assche <[e-mail]>
> ---
>  multipathd/cli_handlers.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 04c73866..460fea1f 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -1,6 +1,9 @@
>  /*
>   * Copyright (c) 2005 Christophe Varoqui
>   */
> +
> +#define _GNU_SOURCE
> +
>  #include "checkers.h"
>  #include "memory.h"
>  #include "vector.h"
> @@ -1332,14 +1335,9 @@ cli_getprstatus (void * v, char ** reply, int
> * len, void * data)
>  
>  	condlog(3, "%s: prflag = %u", param, (unsigned int)mpp-
> >prflag);
>  
> -	*reply =(char *)malloc(2);
> -	*len = 2;
> -	memset(*reply,0,2);
> -
> -
> -	sprintf(*reply,"%d",mpp->prflag);
> -	(*reply)[1]='\0';
> -
> +	*len = asprintf(reply, "%d", mpp->prflag);
> +	if (*len < 0)
> +		return 1;
>  
>  	condlog(3, "%s: reply = %s", param, *reply);
>  

how about this simpler patch, as prflag is actually a boolean?

Comments

Bart Van Assche June 13, 2017, 7:53 p.m. UTC | #1

On 06/13/17 12:29, Martin Wilck wrote:
> how about this simpler patch, as prflag is actually a boolean?
> 
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 04c73866..c31ebd34 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -1337,7 +1337,7 @@ cli_getprstatus (void * v, char ** reply, int * len, void * data)
>         memset(*reply,0,2);
>  
>  
> -       sprintf(*reply,"%d",mpp->prflag);
> +       sprintf(*reply, "%d", !!mpp->prflag);
>         (*reply)[1]='\0';

Hello Martin,

Every sprintf() call requires careful analysis to see whether or not it
triggers a buffer overflow. I really would like to get rid of that
sprintf() call.

Thanks,

Bart.

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Martin Wilck June 13, 2017, 8:18 p.m. UTC | #2

On Tue, 2017-06-13 at 12:53 -0700, Bart Van Assche wrote:
> On 06/13/17 12:29, Martin Wilck wrote:
> > how about this simpler patch, as prflag is actually a boolean?
> > 
> > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> > index 04c73866..c31ebd34 100644
> > --- a/multipathd/cli_handlers.c
> > +++ b/multipathd/cli_handlers.c
> > @@ -1337,7 +1337,7 @@ cli_getprstatus (void * v, char ** reply, int
> > * len, void * data)
> >         memset(*reply,0,2);
> >  
> >  
> > -       sprintf(*reply,"%d",mpp->prflag);
> > +       sprintf(*reply, "%d", !!mpp->prflag);
> >         (*reply)[1]='\0';
> 
> Hello Martin,
> 
> Every sprintf() call requires careful analysis to see whether or not
> it
> triggers a buffer overflow. I really would like to get rid of that
> sprintf() call.

Then we could write

	snprintf(*reply, 2, "%d", !!mpp->prflag);
  
without needing _GNU_SOURCE.

Martin

Bart Van Assche June 13, 2017, 8:21 p.m. UTC | #3

On 06/13/17 13:18, Martin Wilck wrote:
> On Tue, 2017-06-13 at 12:53 -0700, Bart Van Assche wrote:
>> On 06/13/17 12:29, Martin Wilck wrote:
>>> how about this simpler patch, as prflag is actually a boolean?
>>>
>>> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
>>> index 04c73866..c31ebd34 100644
>>> --- a/multipathd/cli_handlers.c
>>> +++ b/multipathd/cli_handlers.c
>>> @@ -1337,7 +1337,7 @@ cli_getprstatus (void * v, char ** reply, int
>>> * len, void * data)
>>>         memset(*reply,0,2);
>>>  
>>>  
>>> -       sprintf(*reply,"%d",mpp->prflag);
>>> +       sprintf(*reply, "%d", !!mpp->prflag);
>>>         (*reply)[1]='\0';
>>
>> Hello Martin,
>>
>> Every sprintf() call requires careful analysis to see whether or not
>> it
>> triggers a buffer overflow. I really would like to get rid of that
>> sprintf() call.
> 
> Then we could write
> 
> 	snprintf(*reply, 2, "%d", !!mpp->prflag);
>   
> without needing _GNU_SOURCE.

Hello Martin,

There are already three other multipath-tools source files that #define
_GNU_SOURCE so I don't see what's wrong with using _GNU_SOURCE.

Bart.

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Martin Wilck June 13, 2017, 8:35 p.m. UTC | #4

Hello Bart,

On Tue, 2017-06-13 at 13:21 -0700, Bart Van Assche wrote:
> 
> > > Hello Martin,
> > > 
> > > Every sprintf() call requires careful analysis to see whether or
> > > not
> > > it
> > > triggers a buffer overflow. I really would like to get rid of
> > > that
> > > sprintf() call.
> > 
> > Then we could write
> > 
> > 	snprintf(*reply, 2, "%d", !!mpp->prflag);
> >   
> > without needing _GNU_SOURCE.
> 
> Hello Martin,
> 
> There are already three other multipath-tools source files that
> #define
> _GNU_SOURCE so I don't see what's wrong with using _GNU_SOURCE.

Yes, I saw that. I haven't reviewed the reason why _GNU_SOURCE is used
in the other places. In general it's a thing I'd rather avoid for
portability reasons.
In this particular case, I think the problem at hand be easily solved
without resorting to _GNU_SOURCE.

But well, it's not a thing worth fighting about. May Christophe decide.

Martin

Patch

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 04c73866..c31ebd34 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1337,7 +1337,7 @@  cli_getprstatus (void * v, char ** reply, int * len, void * data)
        memset(*reply,0,2);
 
 
-       sprintf(*reply,"%d",mpp->prflag);
+       sprintf(*reply, "%d", !!mpp->prflag);
        (*reply)[1]='\0';