PerlFFI / FFI-Platypus

Write Perl bindings to non-Perl libraries with FFI. No XS required.
89 stars 23 forks source link

Heap cleanup segfaults #358

Closed xsawyerx closed 2 years ago

xsawyerx commented 2 years ago

All the C functions work fine, but there's a segfault at the end when the last function (OSSL_DECODER_from_bio) returns. gdb claims it comes from the heap cleanup function ffi_pl_heap_free happening here: https://github.com/PerlFFI/FFI-Platypus/blob/main/include/ffi_platypus_call.h#L973

use FFI::Platypus 1.00;

my $ffi = FFI::Platypus->new( 'api' => 2 );
$ffi->load_custom_type( '::PointerSizeBuffer' => 'buffer' );
$ffi->lib( 'libssl.so', 'libcrypto.so' );

$ffi->attach( 'BIO_new_mem_buf' => ['buffer'] => 'opaque' );
$ffi->attach(
    'OSSL_DECODER_CTX_new_for_pkey',
    [ 'opaque*', 'string', 'string', 'string', 'int', 'opaque', 'string' ],
    'opaque',
);
$ffi->attach( 'OSSL_DECODER_from_bio' => [ 'opaque', 'opaque' ] => 'int' );

# taken from OpenSSL::Crypt::RSA's test suite
my $stringBIO = "-----BEGIN RSA PRIVATE KEY-----\nMBsCAQACAU0CAQcCASsCAQcCAQsCAQECAQMCAQI=\n-----END RSA PRIVATE KEY-----\n";
my $bio       = BIO_new_mem_buf($stringBIO);

my $pkey;
my $dctx = OSSL_DECODER_CTX_new_for_pkey( \$pkey, 'PEM', undef, 'RSA', 0, undef, undef );
my $res = OSSL_DECODER_from_bio( $dctx, $bio );
plicease commented 2 years ago

Can you try to re-write this without ::PointerSizeBuffer? Looks like BIO_new_mem_buf takes an int (usually signed 32 bit) as its second argument and I think ::PointerSizeBuffer uses a size_t (usually unsigned 64 bit).

BIO *BIO_new_mem_buf(const void *buf, int len);

You can use FFI::Platypus::Buffer to convert a Perl scalar to a opaque, size pair.

If that does work, we can maybe update PointerSizeBuffer to work with different integer types in the future.

xsawyerx commented 2 years ago

If I try to use the conversion, I get another segfault, but from a different place:

use FFI::Platypus::Buffer qw( scalar_to_buffer );
my $bio       = BIO_new_mem_buf( scalar_to_buffer($stringBIO) );

Error from gdb:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7d3d47e in __GI___libc_free (mem=0x6e6f6973726576) at ./malloc/malloc.c:3368

I also tried casting to an opaque from a string which has a new, exciting error:

$ffi->attach( 'BIO_new_mem_buf' => ['opaque', 'int'] => 'opaque' );
...
my $bio       = BIO_new_mem_buf( $ffi->cast( 'string' => 'opaque', $stringBIO ), -1 );

And the error:

*** stack smashing detected ***: terminated

xsawyerx commented 2 years ago

If I store the PTR resulting from scalar_to_buffer() in a lexical variable before calling BIO_new_mem_buf(), it no longer crashes, but it doesn't seem like it has done its job, since I can immediately check the content of the BIO object from OpenSSL and it reports it has no content. The string wasn't stored from the BIO_new_mem_buf function.

timlegge commented 2 years ago

@xsawyerx I could be wrong but I would think:

my $stringBIO = "-----BEGIN RSA PRIVATE KEY-----\nMBsCAQACAU0CAQcCASsCAQcCAQsCAQECAQMCAQI=\n-----END RSA PRIVATE KEY-----\n";

should be:

my $stringBIO = "MBsCAQACAU0CAQcCASsCAQcCAQsCAQECAQMCAQI=";

xsawyerx commented 2 years ago

@xsawyerx I could be wrong but I would think:

my $stringBIO = "-----BEGIN RSA PRIVATE KEY-----\nMBsCAQACAU0CAQcCASsCAQcCAQsCAQECAQMCAQI=\n-----END RSA PRIVATE KEY-----\n";

should be:

my $stringBIO = "MBsCAQACAU0CAQcCASsCAQcCAQsCAQECAQMCAQI=";

Thank you for the suggestion!

When I try this, the OpenSSL function for creating a decoder context object works, but the method for parsing the BIO buffer fails. I had also compared it to their rsa utility (with debugging) and to their demo app - they both read with the headers. So, I think we need those headers in. Even if this were the way to go, why would it have caused a segfault? It would just be reading from the buffer and fail to decode, no?

Interestingly, any XS I wrote that tries to run the code I wrote above fails, but pure C (with no XS) works. :/

timlegge commented 2 years ago

@xsawyerx I would also base64 decode the stringBIO before using it.

xsawyerx commented 2 years ago

I think that's what RSA's DER decoder does when it's told the input is in DER. The C code doesn't decode it before calling the functions I call, so I don't understand why my code should.

timlegge commented 2 years ago

Those were a shot in the dark based on how would I would think to use it but you're right it looks like the function references the format

The only other thing I can think of based on the documentation is that it states that $pkey must be initialized to null. It's possible that it is looking for null not just an undefined variable

xsawyerx commented 2 years ago

I was thinking the same. I'm not sure how to address this. Platypus' documentation states that any undef is essentially NULL. I was able to use some debugging statements to verify that the default incantation (my $pkey; function(\$pkey)) did send double pointer to NULL, but alas, it still failed. :/

The C functions do the right thing and create the EVP_PKEY object pointer, but Platypus' cleanup is crashing when unpacking the return values and heap cleanup.

plicease commented 2 years ago

I think I have solved this. OSSL_DECODER_CTX_new_for_pkey takes $pkey a pointer to a pointer, but doesn't actually write to that pointer until OSSL_DECODER_from_bio is called, by which time the double pointer has been converted into a Perl reference to a pointer so the passed in double pointer has been freed.

This works:

use strict;
use warnings;
use v5.24;
use FFI::Platypus 1.00;
use FFI::Platypus::Memory qw( malloc memset );
use Alien::OpenSSL;

my $ffi = FFI::Platypus->new( 'api' => 1 );
$ffi->lib( Alien::OpenSSL->dynamic_libs );

$ffi->attach( 'BIO_new_mem_buf' => ['string','int'] => 'opaque' );
$ffi->attach(
    'OSSL_DECODER_CTX_new_for_pkey',
    [ 'opaque', 'string', 'string', 'string', 'int', 'opaque', 'string' ],
    'opaque',
);
$ffi->attach( 'OSSL_DECODER_from_bio' => [ 'opaque', 'opaque' ] => 'int' );

# taken from OpenSSL::Crypt::RSA's test suite
my $stringBIO = "-----BEGIN RSA PRIVATE KEY-----\nMBsCAQACAU0CAQcCASsCAQcCAQsCAQECAQMCAQI=\n-----END RSA PRIVATE KEY-----\n";
my $bio       = BIO_new_mem_buf($stringBIO, -1);

# create the double pointer
my $pkey = malloc($ffi->sizeof('opaque'));
# set the inner pointer to NULL
memset($pkey, 0, $ffi->sizeof('opaque'));

my $dctx = OSSL_DECODER_CTX_new_for_pkey( $pkey, 'PEM', undef, 'RSA', 0, undef, undef );
my $res = OSSL_DECODER_from_bio( $dctx, $bio );

# dereference the double pointer and get the value of the inner pointer.
$pkey_value = $ffi->cast('opaque', 'opaque*', $pkey)->$*;
say $pkey_value;
xsawyerx commented 2 years ago

Works! Thank you so much.