Skip to content

Commit

Permalink
curtail ASN1 memory leaks
Browse files Browse the repository at this point in the history
this was two different leaks rolled into one, which i will explain below.
i also have to note the test still leaks *some* objects, but i don't have
the time to do full analysis on that

modules used to diagnose the leaks:
Net::Prometheus, Devel::Gladiator, Devel::FindRef, Devel::Cycle

openxpki#1 is the globals in the parser holding on to things

normally deciding when to clean these up could be a little tricky, but the
parsing appears to be contained entirely within the _new method, so we can
declare them local and have perl reset them at the end of scope

optimally Convert::ASN1 would take care of this so they're deleted after
they're needed; as well as instanced per object or creation of new parsers
guarded while one is currently active

openxpki#2 is PKCS10 creating the object, running ->find on it, which returns a
*shallow* copy with a reference to a config hash in the original object,
then puts said shallow copy into the config hash of the original object,
which creates a cycle

at first i thought weaken would be the more appropriate option here, but
that simply crashed on trying to parse certain certs, so a deep clone is
used and seems to fix the issue
  • Loading branch information
wchristian committed Aug 8, 2023
1 parent 83f3f1a commit 6b54539
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 3 deletions.
4 changes: 3 additions & 1 deletion lib/Crypt/PKCS10.pm
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use Convert::ASN1( qw/:tag :const/ );
use Encode ();
use MIME::Base64;
use Scalar::Util ();
use Clone 'clone';

our $VERSION = '2.005';

Expand Down Expand Up @@ -744,6 +745,7 @@ sub _new {

$self->{_bmpenc} = Encode::find_encoding('UCS2-BE');

local ( $Convert::ASN1::parser::yyval, @Convert::ASN1::parser::yyvs );
my $asn = Convert::ASN1->new;
$self->{_asn} = $asn;
$asn->prepare($schema) or die( "Internal error in " . __PACKAGE__ . ": " . $asn->error );
Expand Down Expand Up @@ -1162,7 +1164,7 @@ sub _init {
my $self = shift;
my( $node, $optional ) = @_;

my $parsed = $self->{_asn}->find($node);
my $parsed = clone $self->{_asn}->find($node);

unless( defined $parsed || $optional ) {
croak( "Missing node $node in ASN.1" );
Expand Down
16 changes: 14 additions & 2 deletions t/02_base.t
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ my @dirpath = (File::Spec->splitpath( $0 ))[0,1];

my $decoded;

plan tests => 12;
plan tests => 13;

# Basic functions test requires RSA

Expand Down Expand Up @@ -820,6 +820,18 @@ subtest 'API v0' => sub {
# More API v0 tests needed
};

if ( not eval { require Net::Prometheus } ) {
diag "Net::Prometheus not installed, skipping leak test";
ok 1;
}
else { # there are some leaks left, but i ran out of time on those
$decoded = undef;
is(
( Net::Prometheus::PerlCollector::count_heap(2) )[3]
->{"Convert::ASN1::parser"},
undef,
"no uncontrolled parser objects leak"
);
}

1;

51 changes: 51 additions & 0 deletions t/05_csr_parse_leak.t
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
#!/usr/bin/env perl
use strict;
use warnings;

use Test::More;
use Crypt::PKCS10;

run();

sub run {
Crypt::PKCS10->setAPIversion(1);

my $string = "-----BEGIN NEW CERTIFICATE REQUEST-----
MIIEcTCCA1kCAQAweDELMAkGA1UEBhMCREUxDzANBgNVBAgMBkJheWVybjEQMA4GA1UEBwwHS3JvbmFjaDEgMB4GA1UECgwXTHVjYXMtQ3JhbmFjaC1DYW1wdXMgS1UxCzAJBgNVBAsMAklUMRcwFQYDVQQDDA50aW1lLmxjYy1rYy5kZTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAMRlgtivD3ajS6Ww8AafhmfUeNwfoTEbzaihb2TPjYfbfHJ7aXKT5ZYXz9vsESQWdhqU8WZmVDkPRH5o6vQsG39F2ZtzPfYvB0BftT5Z5VoT2OUMuS6ecB9dP8B0uACmMyx3zc4pm5Kb70sqRazssmghZMlvBaBi4c1MB1EtoDVSWTI/QyEVN+Gam8qFNiJa38sGFepgn+NozTlOewqWPaOfUflQopkxMMxtJmvfRg22yk3a9aS66u1bTDW3517cFnbPYblh3UMtVSRJBgIJJ7FVo9LzVrDkfvYnNXPxx6aHJptijVlX+5viBFH2qAaaXom/tefpFRt5cbCxWNWCyG0CAwEAAaCCAbIwHAYKKwYBBAGCNw0CAzEOFgwxMC4wLjE3NzYzLjIwTAYJKwYBBAGCNxUUMT8wPQIBBQwWVElNRU1BU1RFUi5sY2NrYy5sb2NhbAwTTENDS0NcYWRtaW5pc3RyYXRvcgwLSW5ldE1nci5leGUwcgYKKwYBBAGCNw0CAjFkMGICAQEeWgBNAGkAYwByAG8AcwBvAGYAdAAgAFIAUwBBACAAUwBDAGgAYQBuAG4AZQBsACAAQwByAHkAcAB0AG8AZwByAGEAcABoAGkAYwAgAFAAcgBvAHYAaQBkAGUAcgMBADCBzwYJKoZIhvcNAQkOMYHBMIG+MA4GA1UdDwEB/wQEAwIE8DATBgNVHSUEDDAKBggrBgEFBQcDATB4BgkqhkiG9w0BCQ8EazBpMA4GCCqGSIb3DQMCAgIAgDAOBggqhkiG9w0DBAICAIAwCwYJYIZIAWUDBAEqMAsGCWCGSAFlAwQBLTALBglghkgBZQMEAQIwCwYJYIZIAWUDBAEFMAcGBSsOAwIHMAoGCCqGSIb3DQMHMB0GA1UdDgQWBBSVEDulP7zK1MhQyDj6QpRzQEZ/njANBgkqhkiG9w0BAQUFAAOCAQEAwAKgBQ/OsudTtQ9AiI6bC6Wb0K2uLMabgXvpa06ESZA3YO/Wd70IiAcMVdgdxhMHW+CW78c8YZ/E4JJw4Riu13JTuM3FuWBa/VH3D6TGN7kVGEBEGh98vaxK6RhlqQVdGMpqrOXtPTg5BRQUR4hP3h9fCXqYB59txyWxbG8fJ1YGALNJF95uiG+IE7/N4379hPT68/m4Bj3XLt1SmomtlqO8A+AemXWKZSacaP9KKggFnkxSC9BbjRi6V785URmwPgFwq20mpvIBQWphQKKBzeDoVRThwyI+RzwnX3XeJ8wDPWf/whlxkJfW3EYtGDcRkoMNykdirPE/JbN0MtIgoA==
-----END NEW CERTIFICATE REQUEST-----";

my $pkcs10 = Crypt::PKCS10->new($string);
ok( $pkcs10, "got a parse result" );
is( Crypt::PKCS10->error, undef, "no errors parsing the CSR" );

$pkcs10->commonName;
$pkcs10->countryName;
$pkcs10->stateOrProvinceName;
$pkcs10->organizationName;
$pkcs10->emailAddress;
$pkcs10->extensionPresent('subjectAltName');
$pkcs10->extensionValue('subjectAltName');
$pkcs10->commonName;
$pkcs10->csrRequest(1);
$pkcs10 = undef;

if ( not eval { require Net::Prometheus } ) {
diag "Net::Prometheus not installed, skipping leak test";
ok 1;
}
else { # there are some leaks left, but i ran out of time on those
note $_ for grep /UDAG|Convert::ASN1/, #
split /\n/,
Net::Prometheus->new->render( { perl_collector_detail => 2 } );
is(
( Net::Prometheus::PerlCollector::count_heap(2) )[3] #
{"Convert::ASN1::parser"},
undef,
"no leak"
);
}

done_testing;

return;
}

0 comments on commit 6b54539

Please sign in to comment.