[PATCH] nfs4-acl-tools : nfs4_setfacl' failed with unexpected messages if the format of the input file is incorrect.

[PATCH] nfs4-acl-tools : nfs4_setfacl' failed with unexpected messages if the format of the input file is incorrect.

[faizan husain]

On RHEL5.5 and above releases,
read ACL entries to set from file leads to a segmentation fault on pp64,
more over the same problem does not show up for x86_64 architecture.

here is what i tried.
mounted a file system over nfs4.

#rpm -qa | grep nfs4
nfs4-acl-tools-0.3.3-5.el6.ppc64

'nfs4_setfacl' failed with unexpected message:
#nfs4_setfacl -S file4 file1
*** glibc detected *** nfs4_setfacl: double free or corruption (out):
0x08007760 ***
expected out should be like
Scanning ACE string 'abcd' failed.
Failed while inserting ACE(s).


 From b8333732964780394518be74ab0b7c61a7e4be4f Mon Sep 17 00:00:00 2001
From: faizan <faizan.husain@xxxxxxx>
Date: Tue, 28 Jun 2011 16:06:47 +0530
Subject: [PATCH][BUILD]] FIX - 'nfs4_setfacl' failed with unexpected 
messages iff
the format of the input file is incorrect.


Signed-off-by: faizan <faizan.husain@xxxxxxxx>
---
  libnfs4acl/nfs4_ace_from_string.c |    1 -
  1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/libnfs4acl/nfs4_ace_from_string.c 
b/libnfs4acl/nfs4_ace_from_string.c
index 9d877fb..1cc220e 100644
--- a/libnfs4acl/nfs4_ace_from_string.c
+++ b/libnfs4acl/nfs4_ace_from_string.c
@@ -125,7 +125,6 @@ parse_alloc_fields(char *buf, char *fields[NUMFIELDS])

         return 0;
  out_free:
-       free_fields(fields);
         return -ENOMEM;
  }

--
1.7.1

Regards
Faizan

Re: [PATCH] nfs4-acl-tools : nfs4_setfacl' failed with unexpected messages if the format of the input file is incorrect.

[Jim Rees]

faizan husain wrote:

  On RHEL5.5 and above releases,
  read ACL entries to set from file leads to a segmentation fault on pp64,
  more over the same problem does not show up for x86_64 architecture.

...

  diff --git a/libnfs4acl/nfs4_ace_from_string.c
  b/libnfs4acl/nfs4_ace_from_string.c
  index 9d877fb..1cc220e 100644
  --- a/libnfs4acl/nfs4_ace_from_string.c
  +++ b/libnfs4acl/nfs4_ace_from_string.c
  @@ -125,7 +125,6 @@ parse_alloc_fields(char *buf, char *fields[NUMFIELDS])
  
          return 0;
   out_free:
  -       free_fields(fields);
          return -ENOMEM;
   }

If this fix is correct, shouldn't the name of the label be changed?  Better
yet eliminate the gotos and label.

However, I don't think the fix is correct.  I suspect you need a test for
strsep() returning NULL.

Re: [PATCH] nfs4-acl-tools : nfs4_setfacl' failed with unexpected messages if the format of the input file is incorrect.

[faizan husain]

On Wednesday 29 June 2011 05:48 PM, Jim Rees wrote:
> faizan husain wrote:
>
>    On RHEL5.5 and above releases,
>    read ACL entries to set from file leads to a segmentation fault on pp64,
>    more over the same problem does not show up for x86_64 architecture.
>
> ...
>
>    diff --git a/libnfs4acl/nfs4_ace_from_string.c
>    b/libnfs4acl/nfs4_ace_from_string.c
>    index 9d877fb..1cc220e 100644
>    --- a/libnfs4acl/nfs4_ace_from_string.c
>    +++ b/libnfs4acl/nfs4_ace_from_string.c
>    @@ -125,7 +125,6 @@ parse_alloc_fields(char *buf, char *fields[NUMFIELDS])
>
>            return 0;
>     out_free:
>    -       free_fields(fields);
>            return -ENOMEM;
>     }
>
> If this fix is correct, shouldn't the name of the label be changed?  Better
> yet eliminate the gotos and label.
>
> However, I don't think the fix is correct.  I suspect you need a test for
> strsep() returning NULL.
I have tried strsep() returning NULL but without any success,
have figured out why double free error was coming leading to 
segmentation fault.

problem was this part of code in parse_alloc_fields() function:
if (count != 3)
          goto out_free;
at this point memory is not allocated for fields leading to double free 
of memory once inside parse_alloc_fields() and again inside 
nfs4_ace_from_string().

instead we can change the  code:
if (count != 3)
     return -EINVAL; /*Invalid argument*/

This look to me as more foolproof solution.
what do you say?

Signed-off-by: faizan <faizan.husain@.in.ibm.com>
---
  libnfs4acl/nfs4_ace_from_string.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/libnfs4acl/nfs4_ace_from_string.c 
b/libnfs4acl/nfs4_ace_from_string.c
index 9d877fb..6f1e200 100644
--- a/libnfs4acl/nfs4_ace_from_string.c
+++ b/libnfs4acl/nfs4_ace_from_string.c
@@ -107,7 +107,7 @@ parse_alloc_fields(char *buf, char *fields[NUMFIELDS])
                         count++;
         }
         if (count != 3)
-               goto out_free;
+               return -EINVAL;

         for (i = 0; i < NUMFIELDS; i++) {
                 field = strsep(&buf, ":");
--
1.7.1

Thanks
Faizan

Re: [PATCH] nfs4-acl-tools : nfs4_setfacl' failed with unexpected messages if the format of the input file is incorrect.

[Jim Rees]

faizan husain wrote:

  problem was this part of code in parse_alloc_fields() function:
  if (count != 3)
           goto out_free;
  at this point memory is not allocated for fields leading to double
  free of memory once inside parse_alloc_fields() and again inside
  nfs4_ace_from_string().
  
  instead we can change the  code:
  if (count != 3)
      return -EINVAL; /*Invalid argument*/
  
  This look to me as more foolproof solution.
  what do you say?

That looks correct.  It should return EINVAL here, and there is no need to
free.  But I don't see why it fixes your segfault.  fields[] should be all
NULL at this point, so free_fields shouldn't do anything.

The test in free_fields() is redundant, since free(NULL) doesn't do
anything.  But it could be made more foolproof by zeroing the array so you
can't get a double free:

void
free_fields(char *fields[NUMFIELDS])
{
	int i;

	for (i = 0; i < NUMFIELDS; i++) {
		free(fields[i]);
		fields[i] = NULL;
	}
}

Re: [PATCH] nfs4-acl-tools : nfs4_setfacl' failed with unexpected messages if the format of the input file is incorrect.

[faizan husain]

On Thursday 30 June 2011 05:21 PM, Jim Rees wrote:
> faizan husain wrote:
>
>    problem was this part of code in parse_alloc_fields() function:
>    if (count != 3)
>             goto out_free;
>    at this point memory is not allocated for fields leading to double
>    free of memory once inside parse_alloc_fields() and again inside
>    nfs4_ace_from_string().
>
>    instead we can change the  code:
>    if (count != 3)
>        return -EINVAL; /*Invalid argument*/
>
>    This look to me as more foolproof solution.
>    what do you say?
>
> That looks correct.  It should return EINVAL here, and there is no need to
> free.  But I don't see why it fixes your segfault.  fields[] should be all
> NULL at this point, so free_fields shouldn't do anything.
>
> The test in free_fields() is redundant, since free(NULL) doesn't do
> anything.  But it could be made more foolproof by zeroing the array so you
> can't get a double free:
>
> void
> free_fields(char *fields[NUMFIELDS])
> {
> 	int i;
>
> 	for (i = 0; i<  NUMFIELDS; i++) {
> 		free(fields[i]);
> 		fields[i] = NULL;
> 	}
> }
yeah that could be done to make it more foolproof.
here is the final patch:

 From 6cd5263027e3fa5cf18756aa9db108dcdb2367d5 Mon Sep 17 00:00:00 2001
From: faizan <faizan.husain@in.ibm.com>
Date: Thu, 30 Jun 2011 18:55:28 +0530
Subject: [PATCH][BUILD]] FIX - 'nfs4_setfacl' failed with unexpected 
messages if
the format of the input file is incorrect.


Signed-off-by: faizan <faizan.husain@in.ibm.com>
---
  libnfs4acl/nfs4_ace_from_string.c |    9 +++++----
  1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/libnfs4acl/nfs4_ace_from_string.c 
b/libnfs4acl/nfs4_ace_from_string.c
index 9d877fb..b74b1a9 100644
--- a/libnfs4acl/nfs4_ace_from_string.c
+++ b/libnfs4acl/nfs4_ace_from_string.c
@@ -86,9 +86,10 @@ free_fields(char *fields[NUMFIELDS])
  {
         int i;

-       for (i = 0; i < NUMFIELDS; i++)
-               if (fields[i] != NULL)
-                       free(fields[i]);
+       for (i = 0; i < NUMFIELDS; i++) {
+               free(fields[i]);
+               fields[i] = NULL;
+       }
  }

  int
@@ -107,7 +108,7 @@ parse_alloc_fields(char *buf, char *fields[NUMFIELDS])
                         count++;
         }
         if (count != 3)
-               goto out_free;
+               return -EINVAL;

         for (i = 0; i < NUMFIELDS; i++) {
                 field = strsep(&buf, ":");
--
1.7.1

Thanks
Faizan