Re: [PATCH] [BZ #19408] Fix linux personality syscall wrapper

["Dmitry V. Levin" <ldv at altlinux dot org>]

On Tue, Dec 29, 2015 at 04:42:18PM -0500, Mike Frysinger wrote:
> On 28 Dec 2015 04:09, Dmitry V. Levin wrote:
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/personality.c
> >
> > +  INTERNAL_SYSCALL_DECL (err);
> > +  long ret = INTERNAL_SYSCALL (personality, err, 1, persona);
> > +
> > +  /* Starting with kernel commit v2.6.29-6609-g11d06b2, personality syscall
> > +     never fails to set the personality.  However, due to architecture
> > +     limitations of 32-bit kernels, personality syscall still may return
> > +     an "error".  */
> > +  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (ret, err)))
> > +    ret = -INTERNAL_SYSCALL_ERRNO (ret, err);
> > +  return ret;
> > +}
> 
> INTERNAL_SYSCALL returns the raw value from the kernel, so why do you
> need to test it directly ?

While some architectures (including x86/x86_64) use negated errno semantics,
others (e.g. mips, nios2, powerpc, and sparc) use dedicated registers to return
an error condition.  In the latter case, the kernel syscall exit code negates
the value returned by sys_personality and returns it to userspace as an
"errno".  It has to be negated again to recover the value returned by
sys_personality.

> > +  if (personality (test_persona) != saved_persona ||
> > +      personality (0xffffffff) == -1 ||
> > +      personality (PER_LINUX) == -1 ||
> > +      personality (0xffffffff) != PER_LINUX)
> > +    rc = 1;
> 
> should this also verify errno is not changed ?

Like this?

  errno = 0xdefaced;
  if (personality (test_persona) != saved_persona ||
      personality (0xffffffff) == -1 ||
      personality (PER_LINUX) == -1 ||
      personality (0xffffffff) != PER_LINUX ||
      0xdefaced != errno)
    rc = 1;

> > +  (void) personality (saved_persona);
> 
> we don't have __wur on this func, so the (void) is kind of pointless

This is my way to show that in this case, unlike others,
the return code is irrelevant.  If you or somebody else don't like this style,
I have no problem removing the (void).


-- 
ldv

Attachment: pgpDmXqLIuZiC.pgp
Description: PGP signature