jnr / jnr-ffi

Java Abstracted Foreign Function Layer
Other
1.25k stars 155 forks source link

uid_t/gid_t should be UINT on sparcv9, not SLONG #92

Closed ahilananantha closed 7 years ago

ahilananantha commented 7 years ago

The incorrect type appears to be specified in:

src/main/java/jnr/ffi/provider/jffi/platform/sparcv9/solaris/TypeAliases.java

Looks like in some update to Solaris 10, Sun cleaned up the header file to report uid_t as uint32_t:

https://docs.oracle.com/cd/E19082-01/820-0543/gfraf/index.html

whereas before it was long on 32 bit and int on 64 bit.. so in any case always a 32 bit value.

I don't have a Solaris system to check on. But I looked at Illumos code. This:

https://github.com/illumos/illumos-gate/blob/master/usr/src/boot/include/unistd.h

has:

ifndef _GID_T_DECLARED

typedef __gid_t gid_t;

define _GID_T_DECLARED

endif

...

ifndef _UID_T_DECLARED

typedef __uid_t uid_t;

define _UID_T_DECLARED

endif

and __uid_t and gid_t are declared in:

https://github.com/illumos/illumos-gate/blob/master/usr/src/boot/sys/sys/_types.h

as:

typedef uint32_t __gid_t; typedef uint32_t __uid_t;

There's another header file with definitions here:

https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/sys/types.h

effectively the same:

ifndef _UID_T

define _UID_T

typedef unsigned int uid_t; / UID type /

endif / _UID_T /

typedef uid_t gid_t; / GID type /

headius commented 7 years ago

I suppose we can just change it? Perhaps you can try it and make sure tests all pass? I don't have access to a sparcv9 machine.

headius commented 7 years ago

Oh, and if it works...PR please :-)

ahilananantha commented 7 years ago

I don't have access to any sparcv9 machines either. I just stumbled upon this by accidental code inspection. I was trying to figure out what the largest portably safe target Java type for a uid was. I was pretty sure it was 32 bit integer and our existing code assumes that. But then I saw this outlier in the jnr-ffi code where it was 64 bit, was skeptical about it, and then started googling. So I'm entirely unaffected by this bug.

Finding a sparcv9 at this point would range between time consuming or impossible depending on who you know. Even for Oracle employee, access is restricted to people in that specific organization. Maybe this is too cavalier but... if this code was never tested on sparcv9 when it was checked in maybe it's no worse to not test the fix? :-)

ahilananantha commented 7 years ago

Some friendly folks in the Solaris unit have pointed out the definition of uid_t and gid_t isn't processor specific. Instead it's a Solaris thing. So there's only one correct "configuration" for all the Solaris ports. And you already have it configured correctly on Solaris x86 and Solaris x86_64:

$ grep uid_t i386/solaris/TypeAliases.java m.put(TypeAlias.uid_t, NativeType.UINT); $ grep uid_t x86_64/solaris/TypeAliases.java m.put(TypeAlias.uid_t, NativeType.UINT); $ grep gid_t i386/solaris/TypeAliases.java m.put(TypeAlias.gid_t, NativeType.UINT); $ grep gid_t x86_64/solaris/TypeAliases.java m.put(TypeAlias.gid_t, NativeType.UINT);

They've suggested testing that it's correctly configured on Solaris x86. Would that be convincing enough to allow copying that setting to the SPARC files? Also... how do we test something like this?

ahilananantha commented 7 years ago

And it seems you also have "sparc" like for v8 and earlier that in theory needs the same fix. No chance of finding one of those. :-P