Patchwork [Open-FCoE] fcoeadm: --target segfault with other FC storage present

login
register
mail settings
Submitter Chris Leech
Date Oct. 20, 2016, 8:56 p.m.
Message ID <1476997003-9312-1-git-send-email-cleech@redhat.com>
Download mbox | patch
Permalink /patch/364/
State New
Headers show

Comments

Chris Leech - Oct. 20, 2016, 8:56 p.m.
fcoeadm is segfaulting when trying to parse sysfs paths to rports for
traditional FC or non fcoe-utils/libfc FCoE.

I also changed search_rports to just ignore these instead of returning
an error, so that the target information for all fcoe-utils managed
storage can be displayed instead of stopping at the first exception.

Signed-off-by: Chris Leech <cleech@redhat.com>
---
 fcoeadm_display.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Johannes Thumshirn - Oct. 21, 2016, 7:05 a.m.
On Thu, Oct 20, 2016 at 01:56:43PM -0700, Chris Leech wrote:
> fcoeadm is segfaulting when trying to parse sysfs paths to rports for
> traditional FC or non fcoe-utils/libfc FCoE.
> 
> I also changed search_rports to just ignore these instead of returning
> an error, so that the target information for all fcoe-utils managed
> storage can be displayed instead of stopping at the first exception.
> 
> Signed-off-by: Chris Leech <cleech@redhat.com>
> ---
>  fcoeadm_display.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fcoeadm_display.c b/fcoeadm_display.c
> index 7b95aa4..bf3831a 100644
> --- a/fcoeadm_display.c
> +++ b/fcoeadm_display.c
> @@ -660,17 +660,19 @@ static char *get_ifname_from_rport(char *rport)
>  
>  	err = asprintf(&path, "%s/%s", "/sys/class/fc_remote_ports", rport);
>  	if (err == -1)
> -		return false;
> +		return NULL;
>  
>  	ret = readlink(path, link, sizeof(link));
>  	free(path);
>  	if (ret == -1)
> -		return false;
> +		return NULL;
>  
>  	if (link[ret] != '\0')
>  		link[ret] = '\0';
>  
>  	offs = strstr(link, "/net/");
> +	if (!offs)
> +		return NULL;
>  
>  	offs = offs + 5;
>  
> @@ -778,7 +780,7 @@ static int search_rports(struct dirent *dp, void *arg)
>  	} else {
>  		ifname = get_ifname_from_rport(rport);
>  		if (!ifname)
> -			return -ENOMEM;
> +			return 0;
>  		allocated = true;
>  	}
>  
> -- 
> 2.7.4
> 

Hi Chris,

Looks good but I noticed we're leaking memory here (from the asprintf()
calls). I know it's my code that leaks it but do you mind fixing it (for
get_ifname_from_rport()) while you're at it?

Thanks,
	Johannes
Chris Leech - Oct. 21, 2016, 12:58 p.m.
On Fri, Oct 21, 2016 at 09:05:57AM +0200, Johannes Thumshirn wrote:
> Hi Chris,
> 
> Looks good but I noticed we're leaking memory here (from the asprintf()
> calls). I know it's my code that leaks it but do you mind fixing it (for
> get_ifname_from_rport()) while you're at it?

Sorry, but I'm not seeing it.  If asprintf allocates memory, it's used
as an argument to readlink and then freed.  What am I missing?

Chris
Johannes Thumshirn - Oct. 21, 2016, 1:23 p.m.
On Fri, Oct 21, 2016 at 05:58:10AM -0700, Chris Leech wrote:
> On Fri, Oct 21, 2016 at 09:05:57AM +0200, Johannes Thumshirn wrote:
> > Hi Chris,
> > 
> > Looks good but I noticed we're leaking memory here (from the asprintf()
> > calls). I know it's my code that leaks it but do you mind fixing it (for
> > get_ifname_from_rport()) while you're at it?
> 
> Sorry, but I'm not seeing it.  If asprintf allocates memory, it's used
> as an argument to readlink and then freed.  What am I missing?

You're right, I totally missed the call to free. Must've been too early in the
morning. And I reacll I did a valgrind run each test so I should have spotted
eventual leaks...

I'm sorry. Not a good week for me.

Anyways, thanks for the patch. I'll apply it and push to my github as I still
don't have commit access to open-fcoe.org's git :-/.

While we're at it, do you guys have any foce-utils patches in the queue? I'd
like to push out a new release soon (say after RHEL 7.3 and SLES12SP2 are out)?

Thanks,
	Johannes

Patch

diff --git a/fcoeadm_display.c b/fcoeadm_display.c
index 7b95aa4..bf3831a 100644
--- a/fcoeadm_display.c
+++ b/fcoeadm_display.c
@@ -660,17 +660,19 @@  static char *get_ifname_from_rport(char *rport)
 
 	err = asprintf(&path, "%s/%s", "/sys/class/fc_remote_ports", rport);
 	if (err == -1)
-		return false;
+		return NULL;
 
 	ret = readlink(path, link, sizeof(link));
 	free(path);
 	if (ret == -1)
-		return false;
+		return NULL;
 
 	if (link[ret] != '\0')
 		link[ret] = '\0';
 
 	offs = strstr(link, "/net/");
+	if (!offs)
+		return NULL;
 
 	offs = offs + 5;
 
@@ -778,7 +780,7 @@  static int search_rports(struct dirent *dp, void *arg)
 	} else {
 		ifname = get_ifname_from_rport(rport);
 		if (!ifname)
-			return -ENOMEM;
+			return 0;
 		allocated = true;
 	}