[PATCH] ipvsadm: Fix svc->pe_name conditional

[PATCH] ipvsadm: Fix svc->pe_name conditional

[Ryan O'Hara]

The pe_name in ipvs_service_t is an array, so comparing it NULL has no
effect. Use strlen to see if pe_name is set.

Signed-off-by: Ryan O'Hara <rohara@redhat.com>
---
 libipvs/libipvs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libipvs/libipvs.c b/libipvs/libipvs.c
index c3c3b0a..ebc46e9 100644
--- a/libipvs/libipvs.c
+++ b/libipvs/libipvs.c
@@ -229,7 +229,7 @@ static int ipvs_nl_fill_service_attr(struct nl_msg *msg, ipvs_service_t *svc)
 	}
 
 	NLA_PUT_STRING(msg, IPVS_SVC_ATTR_SCHED_NAME, svc->sched_name);
-	if (svc->pe_name)
+	if (strlen(svc->pe_name))
 		NLA_PUT_STRING(msg, IPVS_SVC_ATTR_PE_NAME, svc->pe_name);
 	NLA_PUT(msg, IPVS_SVC_ATTR_FLAGS, sizeof(flags), &flags);
 	NLA_PUT_U32(msg, IPVS_SVC_ATTR_TIMEOUT, svc->timeout);
-- 
1.8.1.4

Re: [PATCH] ipvsadm: Fix svc->pe_name conditional

[Julian Anastasov]

	Hello,

On Sun, 18 Aug 2013, Ryan O'Hara wrote:

> The pe_name in ipvs_service_t is an array, so comparing it NULL has no
> effect. Use strlen to see if pe_name is set.
> 
> Signed-off-by: Ryan O'Hara <rohara@redhat.com>

	Looks good to me.

Acked-by: Julian Anastasov <ja@ssi.bg>

	Or may be we can avoid strlen? Eg. svc->pe_name[0] ?

	It looks like CHECK_PE has similar problem and more:

- Do we really work without NL support if CHECK_PE
always succeeds in 'if (s->pe_name)'? I have a doubt.

- CHECK_PE and CHECK_IPV4 as part of CHECK_COMPAT_SVC use
'return' and we can leak svc in ipvs_get_service()

- in ipvs_get_service() CHECK_PE does not need to be called,
it is already part of CHECK_COMPAT_SVC

- ipvs_get_service uses malloc but later calls CHECK_PE, may be
malloc should be changed with calloc

> ---
>  libipvs/libipvs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libipvs/libipvs.c b/libipvs/libipvs.c
> index c3c3b0a..ebc46e9 100644
> --- a/libipvs/libipvs.c
> +++ b/libipvs/libipvs.c
> @@ -229,7 +229,7 @@ static int ipvs_nl_fill_service_attr(struct nl_msg *msg, ipvs_service_t *svc)
>  	}
>  
>  	NLA_PUT_STRING(msg, IPVS_SVC_ATTR_SCHED_NAME, svc->sched_name);
> -	if (svc->pe_name)
> +	if (strlen(svc->pe_name))
>  		NLA_PUT_STRING(msg, IPVS_SVC_ATTR_PE_NAME, svc->pe_name);
>  	NLA_PUT(msg, IPVS_SVC_ATTR_FLAGS, sizeof(flags), &flags);
>  	NLA_PUT_U32(msg, IPVS_SVC_ATTR_TIMEOUT, svc->timeout);
> -- 
> 1.8.1.4

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipvsadm: Fix svc->pe_name conditional

[Ryan O'Hara]

On Sun, Aug 18, 2013 at 09:13:39PM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Sun, 18 Aug 2013, Ryan O'Hara wrote:
> 
> > The pe_name in ipvs_service_t is an array, so comparing it NULL has no
> > effect. Use strlen to see if pe_name is set.
> > 
> > Signed-off-by: Ryan O'Hara <rohara@redhat.com>
> 
> 	Looks good to me.
> 
> Acked-by: Julian Anastasov <ja@ssi.bg>
> 
> 	Or may be we can avoid strlen? Eg. svc->pe_name[0] ?

Agree. I'll resend the patch without strlen.

> 	It looks like CHECK_PE has similar problem and more:
> 
> - Do we really work without NL support if CHECK_PE
> always succeeds in 'if (s->pe_name)'? I have a doubt.

Not sure about this. I've always been under the impression that NL is
absolutely required.

> - CHECK_PE and CHECK_IPV4 as part of CHECK_COMPAT_SVC use
> 'return' and we can leak svc in ipvs_get_service()
>
> - in ipvs_get_service() CHECK_PE does not need to be called,
> it is already part of CHECK_COMPAT_SVC

Yes. I'm tempted to remove these macros due to the 'return'
issue. Thoughts?

> - ipvs_get_service uses malloc but later calls CHECK_PE, may be
> malloc should be changed with calloc

Aside from the fact that CHECK_PE could return without freeing memory,
I don't see the problem here.

Ryan


> > ---
> >  libipvs/libipvs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libipvs/libipvs.c b/libipvs/libipvs.c
> > index c3c3b0a..ebc46e9 100644
> > --- a/libipvs/libipvs.c
> > +++ b/libipvs/libipvs.c
> > @@ -229,7 +229,7 @@ static int ipvs_nl_fill_service_attr(struct nl_msg *msg, ipvs_service_t *svc)
> >  	}
> >  
> >  	NLA_PUT_STRING(msg, IPVS_SVC_ATTR_SCHED_NAME, svc->sched_name);
> > -	if (svc->pe_name)
> > +	if (strlen(svc->pe_name))
> >  		NLA_PUT_STRING(msg, IPVS_SVC_ATTR_PE_NAME, svc->pe_name);
> >  	NLA_PUT(msg, IPVS_SVC_ATTR_FLAGS, sizeof(flags), &flags);
> >  	NLA_PUT_U32(msg, IPVS_SVC_ATTR_TIMEOUT, svc->timeout);
> > -- 
> > 1.8.1.4
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipvsadm: Fix svc->pe_name conditional

[Julian Anastasov]

	Hello,

On Mon, 19 Aug 2013, Ryan O'Hara wrote:

> On Sun, Aug 18, 2013 at 09:13:39PM +0300, Julian Anastasov wrote:
> 
> > - CHECK_PE and CHECK_IPV4 as part of CHECK_COMPAT_SVC use
> > 'return' and we can leak svc in ipvs_get_service()
> >
> > - in ipvs_get_service() CHECK_PE does not need to be called,
> > it is already part of CHECK_COMPAT_SVC
> 
> Yes. I'm tempted to remove these macros due to the 'return'
> issue. Thoughts?

	May be they can use 'goto' instead of 'return':

	if (condition) {
		errno = XXX;
		goto out_err;
	}

	and funcs can have such exit point:

out_err:
	free stuff on error
	return ret;

> > - ipvs_get_service uses malloc but later calls CHECK_PE, may be
> > malloc should be changed with calloc
> 
> Aside from the fact that CHECK_PE could return without freeing memory,
> I don't see the problem here.

	malloc does not initialize the memory and checking of
svc->pe_name[0] in CHECK_PE would give random results.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipvsadm: Fix svc->pe_name conditional

[Ryan O'Hara]

On Mon, Aug 19, 2013 at 06:27:21PM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Mon, 19 Aug 2013, Ryan O'Hara wrote:
> 
> > On Sun, Aug 18, 2013 at 09:13:39PM +0300, Julian Anastasov wrote:
> > 
> > > - CHECK_PE and CHECK_IPV4 as part of CHECK_COMPAT_SVC use
> > > 'return' and we can leak svc in ipvs_get_service()
> > >
> > > - in ipvs_get_service() CHECK_PE does not need to be called,
> > > it is already part of CHECK_COMPAT_SVC
> > 
> > Yes. I'm tempted to remove these macros due to the 'return'
> > issue. Thoughts?
> 
> 	May be they can use 'goto' instead of 'return':
> 
> 	if (condition) {
> 		errno = XXX;
> 		goto out_err;
> 	}
> 
> 	and funcs can have such exit point:
> 
> out_err:
> 	free stuff on error
> 	return ret;

OK. It looks like ipvs_get_service() is the only function with a
potential leak. The other functions that call any of the CHECK_*
macros will just get an 'out_err: return -1'.

I also noticed that CHECK_COMPAT_DEST can be removed. This macro
simply calls CHECK_IPV4, which is also called by CHECK_COMPAT_SVC. I
noticed that CHECK_COMPAT_DEST is only called after CHECK_COMPAT_SVC,
so this macro can be removed entirely. Agree?

> > > - ipvs_get_service uses malloc but later calls CHECK_PE, may be
> > > malloc should be changed with calloc
> > 
> > Aside from the fact that CHECK_PE could return without freeing memory,
> > I don't see the problem here.
> 
> 	malloc does not initialize the memory and checking of
> svc->pe_name[0] in CHECK_PE would give random results.

I see. Thanks.

Ryan

Re: [PATCH] ipvsadm: Fix svc->pe_name conditional

[Simon Horman]

On Mon, Aug 19, 2013 at 03:40:44PM -0500, Ryan O'Hara wrote:
> On Mon, Aug 19, 2013 at 06:27:21PM +0300, Julian Anastasov wrote:
> > 
> > 	Hello,
> > 
> > On Mon, 19 Aug 2013, Ryan O'Hara wrote:
> > 
> > > On Sun, Aug 18, 2013 at 09:13:39PM +0300, Julian Anastasov wrote:
> > > 
> > > > - CHECK_PE and CHECK_IPV4 as part of CHECK_COMPAT_SVC use
> > > > 'return' and we can leak svc in ipvs_get_service()
> > > >
> > > > - in ipvs_get_service() CHECK_PE does not need to be called,
> > > > it is already part of CHECK_COMPAT_SVC
> > > 
> > > Yes. I'm tempted to remove these macros due to the 'return'
> > > issue. Thoughts?
> > 
> > 	May be they can use 'goto' instead of 'return':
> > 
> > 	if (condition) {
> > 		errno = XXX;
> > 		goto out_err;
> > 	}
> > 
> > 	and funcs can have such exit point:
> > 
> > out_err:
> > 	free stuff on error
> > 	return ret;
> 
> OK. It looks like ipvs_get_service() is the only function with a
> potential leak. The other functions that call any of the CHECK_*
> macros will just get an 'out_err: return -1'.
> 
> I also noticed that CHECK_COMPAT_DEST can be removed. This macro
> simply calls CHECK_IPV4, which is also called by CHECK_COMPAT_SVC. I
> noticed that CHECK_COMPAT_DEST is only called after CHECK_COMPAT_SVC,
> so this macro can be removed entirely. Agree?

That sounds reasonable to me.

> 
> > > > - ipvs_get_service uses malloc but later calls CHECK_PE, may be
> > > > malloc should be changed with calloc
> > > 
> > > Aside from the fact that CHECK_PE could return without freeing memory,
> > > I don't see the problem here.
> > 
> > 	malloc does not initialize the memory and checking of
> > svc->pe_name[0] in CHECK_PE would give random results.
> 
> I see. Thanks.
> 
> Ryan
> 

Re: [PATCH] ipvsadm: Fix svc->pe_name conditional

[Julian Anastasov]

	Hello,

On Mon, 19 Aug 2013, Ryan O'Hara wrote:

> On Mon, Aug 19, 2013 at 06:27:21PM +0300, Julian Anastasov wrote:
> > 
> > 	May be they can use 'goto' instead of 'return':
> > 
> > 	if (condition) {
> > 		errno = XXX;
> > 		goto out_err;
> > 	}
> > 
> > 	and funcs can have such exit point:
> > 
> > out_err:
> > 	free stuff on error
> > 	return ret;
> 
> OK. It looks like ipvs_get_service() is the only function with a
> potential leak. The other functions that call any of the CHECK_*
> macros will just get an 'out_err: return -1'.

	Yes

> I also noticed that CHECK_COMPAT_DEST can be removed. This macro
> simply calls CHECK_IPV4, which is also called by CHECK_COMPAT_SVC. I
> noticed that CHECK_COMPAT_DEST is only called after CHECK_COMPAT_SVC,
> so this macro can be removed entirely. Agree?

	No. It looks like the macros work with more
than one structure: svc and dest. Same was for CHECK_PE
where both structs were supported: ipvs_service_entry_t and
ipvs_service_t when CHECK_PE was called from CHECK_COMPAT_SVC.

	CHECK_COMPAT_SVC does not check the dest->af,
it checks svc->af. Without both checks, svc can come
with address from AF_INET family while -r option has address
from AF_INET6 family. IMHO, we need just the return->goto
change.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipvsadm: Fix svc->pe_name conditional

[Ryan O'Hara]

On Tue, Aug 20, 2013 at 09:35:49AM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Mon, 19 Aug 2013, Ryan O'Hara wrote:
> 
> > On Mon, Aug 19, 2013 at 06:27:21PM +0300, Julian Anastasov wrote:
> > > 
> > > 	May be they can use 'goto' instead of 'return':
> > > 
> > > 	if (condition) {
> > > 		errno = XXX;
> > > 		goto out_err;
> > > 	}
> > > 
> > > 	and funcs can have such exit point:
> > > 
> > > out_err:
> > > 	free stuff on error
> > > 	return ret;
> > 
> > OK. It looks like ipvs_get_service() is the only function with a
> > potential leak. The other functions that call any of the CHECK_*
> > macros will just get an 'out_err: return -1'.
> 
> 	Yes
> 
> > I also noticed that CHECK_COMPAT_DEST can be removed. This macro
> > simply calls CHECK_IPV4, which is also called by CHECK_COMPAT_SVC. I
> > noticed that CHECK_COMPAT_DEST is only called after CHECK_COMPAT_SVC,
> > so this macro can be removed entirely. Agree?
> 
> 	No. It looks like the macros work with more
> than one structure: svc and dest. Same was for CHECK_PE
> where both structs were supported: ipvs_service_entry_t and
> ipvs_service_t when CHECK_PE was called from CHECK_COMPAT_SVC.
> 
> 	CHECK_COMPAT_SVC does not check the dest->af,
> it checks svc->af. Without both checks, svc can come
> with address from AF_INET family while -r option has address
> from AF_INET6 family. IMHO, we need just the return->goto
> change.

You're right. Not sure how I missed that.

Ryan