[PATCH] Cygwin: get_posix_access: avoid negative subscript

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Cygwin: get_posix_access: avoid negative subscript

Ken Brown-6
Don't refer to lacl[pos] unless we know that pos >= 0.
---
 winsup/cygwin/sec_acl.cc | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/sec_acl.cc b/winsup/cygwin/sec_acl.cc
index 933bfa69d..67749d7b1 100644
--- a/winsup/cygwin/sec_acl.cc
+++ b/winsup/cygwin/sec_acl.cc
@@ -807,9 +807,9 @@ get_posix_access (PSECURITY_DESCRIPTOR psd,
   lacl[pos].a_id = ACL_UNDEFINED_ID;
   lacl[pos].a_perm = CYG_ACE_MASK_TO_POSIX (ace->Mask);
   aclsid[pos] = well_known_null_sid;
+  has_class_perm = true;
+  class_perm = lacl[pos].a_perm;
  }
-      has_class_perm = true;
-      class_perm = lacl[pos].a_perm;
     }
   if (ace->Header.AceFlags & SUB_CONTAINERS_AND_OBJECTS_INHERIT)
     {
@@ -820,9 +820,9 @@ get_posix_access (PSECURITY_DESCRIPTOR psd,
   lacl[pos].a_id = ACL_UNDEFINED_ID;
   lacl[pos].a_perm = CYG_ACE_MASK_TO_POSIX (ace->Mask);
   aclsid[pos] = well_known_null_sid;
+  has_def_class_perm = true;
+  def_class_perm = lacl[pos].a_perm;
  }
-      has_def_class_perm = true;
-      def_class_perm = lacl[pos].a_perm;
     }
  }
     }
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: get_posix_access: avoid negative subscript

Corinna Vinschen-2
Hi Ken,

On Aug 26 17:43, Ken Brown wrote:
> Don't refer to lacl[pos] unless we know that pos >= 0.

I'm not sure this is entirely right.  Moving the assignment to
class_perm/def_class_perm into the previous if makes sense, but the
bools has_class_perm and has_def_class_perm should be set no matter
what, to indicate that class perms had been specified.

Either way, does this solve a real-world problem?  If so, a pointer
or a short description would be nice.


Thanks,
Corinna


> ---
>  winsup/cygwin/sec_acl.cc | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/winsup/cygwin/sec_acl.cc b/winsup/cygwin/sec_acl.cc
> index 933bfa69d..67749d7b1 100644
> --- a/winsup/cygwin/sec_acl.cc
> +++ b/winsup/cygwin/sec_acl.cc
> @@ -807,9 +807,9 @@ get_posix_access (PSECURITY_DESCRIPTOR psd,
>    lacl[pos].a_id = ACL_UNDEFINED_ID;
>    lacl[pos].a_perm = CYG_ACE_MASK_TO_POSIX (ace->Mask);
>    aclsid[pos] = well_known_null_sid;
> +  has_class_perm = true;
> +  class_perm = lacl[pos].a_perm;
>   }
> -      has_class_perm = true;
> -      class_perm = lacl[pos].a_perm;
>      }
>    if (ace->Header.AceFlags & SUB_CONTAINERS_AND_OBJECTS_INHERIT)
>      {
> @@ -820,9 +820,9 @@ get_posix_access (PSECURITY_DESCRIPTOR psd,
>    lacl[pos].a_id = ACL_UNDEFINED_ID;
>    lacl[pos].a_perm = CYG_ACE_MASK_TO_POSIX (ace->Mask);
>    aclsid[pos] = well_known_null_sid;
> +  has_def_class_perm = true;
> +  def_class_perm = lacl[pos].a_perm;
>   }
> -      has_def_class_perm = true;
> -      def_class_perm = lacl[pos].a_perm;
>      }
>   }
>      }
> --
> 2.21.0
--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: get_posix_access: avoid negative subscript

Ken Brown-6
On 8/27/2019 4:13 AM, Corinna Vinschen wrote:
> On Aug 26 17:43, Ken Brown wrote:
>> Don't refer to lacl[pos] unless we know that pos >= 0.
>
> I'm not sure this is entirely right.  Moving the assignment to
> class_perm/def_class_perm into the previous if makes sense, but the
> bools has_class_perm and has_def_class_perm should be set no matter
> what, to indicate that class perms had been specified.

I don't think has_class_perm should be set if class_perm isn't set; that would
cause a problem at sec_acl.cc:1169.  For has_def_class_perm it doesn't seem to
matter.  Unless I'm missing something, has_def_class_perm is not used when
new_style is true.

> Either way, does this solve a real-world problem?  If so, a pointer
> or a short description would be nice.

No, I just happened to notice it while studying the ACL code.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: get_posix_access: avoid negative subscript

Corinna Vinschen-2
On Aug 27 20:00, Ken Brown wrote:

> On 8/27/2019 4:13 AM, Corinna Vinschen wrote:
> > On Aug 26 17:43, Ken Brown wrote:
> >> Don't refer to lacl[pos] unless we know that pos >= 0.
> >
> > I'm not sure this is entirely right.  Moving the assignment to
> > class_perm/def_class_perm into the previous if makes sense, but the
> > bools has_class_perm and has_def_class_perm should be set no matter
> > what, to indicate that class perms had been specified.
>
> I don't think has_class_perm should be set if class_perm isn't set; that would
> cause a problem at sec_acl.cc:1169.  For has_def_class_perm it doesn't seem to
I see what you mean.  class_perm defaults to 0 so the group perms might
be off.

> matter.  Unless I'm missing something, has_def_class_perm is not used when
> new_style is true.
>
> > Either way, does this solve a real-world problem?  If so, a pointer
> > or a short description would be nice.
>
> No, I just happened to notice it while studying the ACL code.

Ok, thanks, please push.


Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment