[PATCH] Fix incorrect sign-extension of pointer in 32-bit acl __to_entry

[PATCH] Fix incorrect sign-extension of pointer in 32-bit acl __to_entry

[David Allsopp]

[-- Attachment #1: Type: text/plain, Size: 298 bytes --]

I have some code where the acl_t returned by get_file_acl is allocated at
0x80038248. As a result the acl_entry_t generated by acl_get_entry has an
"index" of -1, since the pointer was sign-extended to 64-bits.

My fix is trivial and simply casts the pointer to uintptr_t first.

All best,


David

[-- Attachment #2: 0001-Fix-invalid-acl_entry_t-on-32-bit-Cygwin.patch --]
[-- Type: application/octet-stream, Size: 1386 bytes --]

From c9af9cc1ecba0c716577f7fe6b380437c817de2c Mon Sep 17 00:00:00 2001
From: David Allsopp <david.allsopp@metastack.com>
Date: Thu, 9 Jul 2020 20:17:03 +0100
Subject: [PATCH] Fix invalid acl_entry_t on 32-bit Cygwin

If the acl_t struct was at or above 0x80000000 then the pointer was
sign-extended to 0xffff_ffff_8000_0000 and so the index was lost.

Signed-off-by: David Allsopp <david.allsopp@metastack.com>
---
 winsup/cygwin/release/3.1.7  | 4 ++++
 winsup/cygwin/sec_posixacl.h | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)
 create mode 100644 winsup/cygwin/release/3.1.7

diff --git a/winsup/cygwin/release/3.1.7 b/winsup/cygwin/release/3.1.7
new file mode 100644
index 000000000..6ce316fc4
--- /dev/null
+++ b/winsup/cygwin/release/3.1.7
@@ -0,0 +1,4 @@
+Bug Fixes:
+----------
+
+- Fix acl_get_* functions in 32-bit Cygwin (pointer sign extension)
diff --git a/winsup/cygwin/sec_posixacl.h b/winsup/cygwin/sec_posixacl.h
index a3790a52b..0f9e7bde3 100644
--- a/winsup/cygwin/sec_posixacl.h
+++ b/winsup/cygwin/sec_posixacl.h
@@ -34,7 +34,7 @@ struct __acl_t
 inline acl_entry_t
 __to_entry (acl_t acl, uint16_t idx)
 {
-  return ((uint64_t) idx << 48) | (uint64_t) acl;
+  return ((uint64_t) idx << 48) | (uint64_t) ((uintptr_t) acl);
 }
 #define __to_permset(a,i)	((acl_permset_t)__to_entry((a),(i)))
 
-- 
2.19.2.windows.1

Re: [PATCH] Fix incorrect sign-extension of pointer in 32-bit acl __to_entry

[Corinna Vinschen]

On Jul  9 20:30, David Allsopp via Cygwin-patches wrote:
> I have some code where the acl_t returned by get_file_acl is allocated at
> 0x80038248. As a result the acl_entry_t generated by acl_get_entry has an
> "index" of -1, since the pointer was sign-extended to 64-bits.
> 
> My fix is trivial and simply casts the pointer to uintptr_t first.

Pushed.  I still don't quite understand what the compiler is thinking
there, sign-extending a pointer when casted to an unsigend int type,
but your patch works, so all is well, I guess.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

RE: [PATCH] Fix incorrect sign-extension of pointer in 32-bit acl __to_entry

[David Allsopp]

Corinna Vinschen wrote:
> On Jul  9 20:30, David Allsopp via Cygwin-patches wrote:
> > I have some code where the acl_t returned by get_file_acl is allocated
> > at 0x80038248. As a result the acl_entry_t generated by acl_get_entry
> > has an "index" of -1, since the pointer was sign-extended to 64-bits.
> >
> > My fix is trivial and simply casts the pointer to uintptr_t first.
> 
> Pushed.  I still don't quite understand what the compiler is thinking
> there, sign-extending a pointer when casted to an unsigend int type, but
> your patch works, so all is well, I guess.

Thank you - it is indeed hard to imagine when you'd ever want that behaviour!

Would it be possible to have a snapshot with it, just for continuous integration servers which need the fix, please?


David

Re: [PATCH] Fix incorrect sign-extension of pointer in 32-bit acl __to_entry

[Corinna Vinschen]

On Jul 10 15:22, David Allsopp via Cygwin-patches wrote:
> Corinna Vinschen wrote:
> > On Jul  9 20:30, David Allsopp via Cygwin-patches wrote:
> > > I have some code where the acl_t returned by get_file_acl is allocated
> > > at 0x80038248. As a result the acl_entry_t generated by acl_get_entry
> > > has an "index" of -1, since the pointer was sign-extended to 64-bits.
> > >
> > > My fix is trivial and simply casts the pointer to uintptr_t first.
> > 
> > Pushed.  I still don't quite understand what the compiler is thinking
> > there, sign-extending a pointer when casted to an unsigend int type, but
> > your patch works, so all is well, I guess.
> 
> Thank you - it is indeed hard to imagine when you'd ever want that behaviour!

I wonder if this is a bug in x86 gcc... Jon?

> Would it be possible to have a snapshot with it, just for continuous
> integration servers which need the fix, please?

Sure, done.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

RE: [PATCH] Fix incorrect sign-extension of pointer in 32-bit acl __to_entry

[David Allsopp]

Corinna Vinschen wrote:
> On Jul 10 15:22, David Allsopp via Cygwin-patches wrote:
> > Corinna Vinschen wrote:
> > > On Jul  9 20:30, David Allsopp via Cygwin-patches wrote:
> > > > I have some code where the acl_t returned by get_file_acl is
> > > > allocated at 0x80038248. As a result the acl_entry_t generated by
> > > > acl_get_entry has an "index" of -1, since the pointer was sign-
> extended to 64-bits.
> > > >
> > > > My fix is trivial and simply casts the pointer to uintptr_t first.
> > >
> > > Pushed.  I still don't quite understand what the compiler is
> > > thinking there, sign-extending a pointer when casted to an unsigend
> > > int type, but your patch works, so all is well, I guess.
> >
> > Thank you - it is indeed hard to imagine when you'd ever want that
> behaviour!
> 
> I wonder if this is a bug in x86 gcc... Jon?

I put it to our C gurus in the OCaml team - one (who has also written a formally verified C compiler which on purpose 0-extends in this case) observed that GCC does the same for ARM32 and another (who has occasionally delighted in abusing language memory models to concoct highly bizarre, but legal, abuses of undefined behaviour) found https://gcc.gnu.org/onlinedocs/gcc/Arrays-and-pointers-implementation.html... although the Git history for GCC makes it fairly clear that this behaviour is retrospectively documented[1][2]!


David


[1] https://github.com/gcc-mirror/gcc/commit/cbf4c36fa373
[2] https://github.com/gcc-mirror/gcc/commit/58f4de4f271c

Re: [PATCH] Fix incorrect sign-extension of pointer in 32-bit acl __to_entry

[Corinna Vinschen]

On Jul 10 18:31, David Allsopp via Cygwin-patches wrote:
> Corinna Vinschen wrote:
> > On Jul 10 15:22, David Allsopp via Cygwin-patches wrote:
> > > Corinna Vinschen wrote:
> > > > On Jul  9 20:30, David Allsopp via Cygwin-patches wrote:
> > > > > I have some code where the acl_t returned by get_file_acl is
> > > > > allocated at 0x80038248. As a result the acl_entry_t generated by
> > > > > acl_get_entry has an "index" of -1, since the pointer was sign-
> > extended to 64-bits.
> > > > >
> > > > > My fix is trivial and simply casts the pointer to uintptr_t first.
> > > >
> > > > Pushed.  I still don't quite understand what the compiler is
> > > > thinking there, sign-extending a pointer when casted to an unsigend
> > > > int type, but your patch works, so all is well, I guess.
> > >
> > > Thank you - it is indeed hard to imagine when you'd ever want that
> > behaviour!
> > 
> > I wonder if this is a bug in x86 gcc... Jon?
> 
> I put it to our C gurus in the OCaml team - one (who has also written
> a formally verified C compiler which on purpose 0-extends in this
> case) observed that GCC does the same for ARM32 and another (who has
> occasionally delighted in abusing language memory models to concoct
> highly bizarre, but legal, abuses of undefined behaviour) found
> https://gcc.gnu.org/onlinedocs/gcc/Arrays-and-pointers-implementation.html...

"Bizarre" is the right description here :)


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer