s390x ffi_closure_helper_SYSV

s390x ffi_closure_helper_SYSV

[Richard Plangger]

Hi,

I hit an issue with libffi that was included in CPython 2.7.10.
Here is the github issue:

https://github.com/atgreen/libffi/pull/216

Let me know what you think.

Cheers,
Richard

Re: s390x ffi_closure_helper_SYSV

[Ulrich Weigand]

Richard Plangger wrote:

> I hit an issue with libffi that was included in CPython 2.7.10.
> Here is the github issue:
> 
> https://github.com/atgreen/libffi/pull/216
> 
> Let me know what you think.

I think you're running into the special case with return values
that is described here in the libffi docs:

  In most situations, @samp{libffi} will handle promotion according to
  the ABI.  However, for historical reasons, there is a special case
  with return values that must be handled by your code.  In particular,
  for integral (not @code{struct}) types that are narrower than the
  system register size, the return value will be widened by
  @samp{libffi}.  @samp{libffi} provides a type, @code{ffi_arg}, that
  can be used as the return type.  For example, if the CIF was defined
  with a return type of @code{char}, @samp{libffi} will try to store a
  full @code{ffi_arg} into the return value.

I.e. when a return value is described by any of the small integral
type codes, code is expected to actually return a full ffi_arg.

While this is not explicitly mentioned in the closure docs, the
code makes the assumption that this is handled likewise for closure
return values.  This is also explicitly checked for in the libffi
test suite, e.g. testsuite/libffi.call/cls_sshort.c

While this is indeed somewhat surprising, I don't think we can
simply change the behavior at this point, as other existing
users may depend on current behavior.

In any case, this is a cross-platform issue (though probably
exacerbated on big-endian platforms).

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: s390x ffi_closure_helper_SYSV

[Richard Plangger]

Hi Ulrich,

thx for your reply. I agree that it makes sense to return a full ffi_arg
if the integral value is smaller than the machine register.

However, I still think there is something not right with the s390
implementation. Please have a look at this program:

https://gist.github.com/planrich/3fd72767812754d9104d

The program pollutes a fairly large junk of memory below the frame
pointer and then calls back to a python function (from c).
Because the ret_buffer variable (in ffi_closure_helper_SYSV) is not
initialized properly, the returned value is not the same on s390x as it
is on e.g. x86. `make` on my laptop (x86) returns without asserting, but
it does not on s390x. PPC was recently implemented on PyPy and there we
did also not hit this issue.

Cheers,
Richard

On 12/17/2015 04:30 PM, Ulrich Weigand wrote:
> Richard Plangger wrote:
> 
>> I hit an issue with libffi that was included in CPython 2.7.10.
>> Here is the github issue:
>>
>> https://github.com/atgreen/libffi/pull/216
>>
>> Let me know what you think.
> 
> I think you're running into the special case with return values
> that is described here in the libffi docs:
> 
>   In most situations, @samp{libffi} will handle promotion according to
>   the ABI.  However, for historical reasons, there is a special case
>   with return values that must be handled by your code.  In particular,
>   for integral (not @code{struct}) types that are narrower than the
>   system register size, the return value will be widened by
>   @samp{libffi}.  @samp{libffi} provides a type, @code{ffi_arg}, that
>   can be used as the return type.  For example, if the CIF was defined
>   with a return type of @code{char}, @samp{libffi} will try to store a
>   full @code{ffi_arg} into the return value.
> 
> I.e. when a return value is described by any of the small integral
> type codes, code is expected to actually return a full ffi_arg.
> 
> While this is not explicitly mentioned in the closure docs, the
> code makes the assumption that this is handled likewise for closure
> return values.  This is also explicitly checked for in the libffi
> test suite, e.g. testsuite/libffi.call/cls_sshort.c
> 
> While this is indeed somewhat surprising, I don't think we can
> simply change the behavior at this point, as other existing
> users may depend on current behavior.
> 
> In any case, this is a cross-platform issue (though probably
> exacerbated on big-endian platforms).
> 
> Bye,
> Ulrich
> 

Re: s390x ffi_closure_helper_SYSV

[Ulrich Weigand]

Hi Richard,

> thx for your reply. I agree that it makes sense to return a full ffi_arg
> if the integral value is smaller than the machine register.

It's not a matter of what makes sense, it's a matter of what libffi
*expects the user-provided callback to do*.  If the *callback* doesn't
fill in a full ffi_arg, then libffi will not operate correctly.

> The program pollutes a fairly large junk of memory below the frame
> pointer and then calls back to a python function (from c).
> Because the ret_buffer variable (in ffi_closure_helper_SYSV) is not
> initialized properly, the returned value is not the same on s390x as it
> is on e.g. x86.

The point is that if the user-callback were to fill in a full ffi_arg,
then ret_buffer would be completely filled.  If ret_buffer isn't fully
written, then that's a bug in the callback PyPy provides to libffi.

> `make` on my laptop (x86) returns without asserting, but
> it does not on s390x. PPC was recently implemented on PyPy and there we
> did also not hit this issue.

Is this on little-endian or big-endian PowerPC?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: s390x ffi_closure_helper_SYSV

[Richard Plangger]

Hi,

>> The program pollutes a fairly large junk of memory below the frame
>> pointer and then calls back to a python function (from c).
>> Because the ret_buffer variable (in ffi_closure_helper_SYSV) is not
>> initialized properly, the returned value is not the same on s390x as it
>> is on e.g. x86.
> 
> The point is that if the user-callback were to fill in a full ffi_arg,
> then ret_buffer would be completely filled.  If ret_buffer isn't fully
> written, then that's a bug in the callback PyPy provides to libffi.

Because the return value (on s390x) contains contents from the stack, it
looked malicious to me.

>> `make` on my laptop (x86) returns without asserting, but
>> it does not on s390x. PPC was recently implemented on PyPy and there we
>> did also not hit this issue.
> 
> Is this on little-endian or big-endian PowerPC?

PyPy implements both.

Cheers,
Richard

Re: s390x ffi_closure_helper_SYSV

[Richard Plangger]

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

Hi and happy new year,

I have further investigated this issue. Sorry to be so persistent, but I
still think something is not quite right here. To show my point: here is
a comparison between x86_64 (I assume this is most tested platform for
libffi) and s390x.

In short: on x86_64 the closure helper behaves differently processing
the return value. Here are some gdb steps in the x86_64 linux
implementation:

https://gist.github.com/planrich/fd25c31213ba565116a9

In a nutshell: I broke at the assembly position at
https://github.com/python/cpython/blob/master/Modules/_ctypes/libffi/src/x86/unix64.S#L269
of my small ctypes sample program.
(https://gist.github.com/planrich/3fd72767812754d9104d)

As far as I can tell (on x86_64) ffi_closure_unix64_inner is the
equivalent to ffi_closure_helper_SYSV on s390x.

If the above is correct then:
movzx  eax,WORD PTR [rsp-0x18] zero extends the 16 bit value to a full
64bit value.
That is what my initial patch is all about, s390x does not do this
zero/sign extension just after invoking the user closure.

> The point is that if the user-callback were to fill in a full ffi_arg,
> then ret_buffer would be completely filled.  If ret_buffer isn't fully
> written, then that's a bug in the callback PyPy provides to libffi.

The closure return value (which is written on the stack location of
ret_buffer on s390x) is filled in ctypes here:
https://github.com/python/cpython/blob/master/Modules/_ctypes/cfield.c#L551
This would mean that ctypes only writes 16 bits into ret_buffer? I have
also debugged it, and it does only store 16 bits.

If I'm wrong, could someone please point out the issue with my sample
program?

Cheers,
Richard

P.S. I have looked at the PPC asm implementation as well, there it is
also zero/sign extended to the machine register size.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: s390x ffi_closure_helper_SYSV

[Tom Tromey]

>>>>> "Ulrich" == Ulrich Weigand <uweigand-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org> writes:

Ulrich> While this is not explicitly mentioned in the closure docs, the
Ulrich> code makes the assumption that this is handled likewise for closure
Ulrich> return values.  This is also explicitly checked for in the libffi
Ulrich> test suite, e.g. testsuite/libffi.call/cls_sshort.c

Actually the closure docs are actively wrong right now.
I didn't realize this same widening behavior applied to closures (in
fact I thought I asked about this and got the answer I documented, but I
can't find the email, so it was probably just my error).

I sent a github PR to correct the closure documentation.

Tom