How to diagnose memory leak in ASN.1 structure?

[gingerbeard-man]

Using OpenSSL 3.0.10 I have defined a new CRL Entry Extension (based on BSI TR-03145-5 Appendix B). For every instance of the extension that is created (and later discarded) Valgrind reports a leak of 72 bytes in 3 blocks. E.g. for 4 CRL entries this adds up to 288 bytes:

==30819== HEAP SUMMARY:
==30819==     in use at exit: 288 bytes in 12 blocks
==30819==   total heap usage: 13,005 allocs, 12,993 frees, 892,165 bytes allocated
==30819== 
==30819== 288 bytes in 12 blocks are definitely lost in loss record 1 of 1
==30819==    at 0x4C2A2AF: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==30819==    by 0x508C5D3: CRYPTO_malloc (mem.c:190)
==30819==    by 0x508C5FE: CRYPTO_zalloc (mem.c:197)
==30819==    by 0x4F1622D: ASN1_STRING_type_new (asn1_lib.c:350)
==30819==    by 0x4F255FB: asn1_primitive_new (tasn_new.c:317)
==30819==    by 0x4F24E52: asn1_item_embed_new (tasn_new.c:93)
==30819==    by 0x4F253AD: asn1_template_new (tasn_new.c:242)
==30819==    by 0x4F2509C: asn1_item_embed_new (tasn_new.c:151)
==30819==    by 0x4F24D03: ASN1_item_ex_new (tasn_new.c:57)
==30819==    by 0x4F24C35: ASN1_item_new (tasn_new.c:32)
==30819==    by 0x404C0C: REVOCATION_DETAILS_new (tse_asn1.c:26)
==30819==    by 0x40646F: tse_crl_main (tse_crl.c:514)
==30819== 
==30819== LEAK SUMMARY:
==30819==    definitely lost: 288 bytes in 12 blocks
==30819==    indirectly lost: 0 bytes in 0 blocks
==30819==      possibly lost: 0 bytes in 0 blocks
==30819==    still reachable: 0 bytes in 0 blocks
==30819==         suppressed: 0 bytes in 0 blocks

I am at a loss as to which part is not being freed correctly and why. I suspect some silly mistake in my use of the ASN1 macros.

The structure is declared in a header file

/*
 *   RevocationDetails         ::=   SEQUENCE {
 *
 *         version                   Version,
 *         revokingParty             RevokingParty,
 *         revocationReason          RevocationReason,
 *         signatureCounter          SignatureCounter    OPTIONAL,
 *         additionalInfo            AdditionalInfo      OPTIONAL
 *   }
 *
 *   Version                   ::=   INTEGER (1)
 *
 *   SignatureCounter          ::=   INTEGER
 *
 *   AdditionalInfo            ::=   PrintableString
 */
typedef struct RevocationDetails_st {
    ASN1_INTEGER         *version;
    ASN1_ENUMERATED      *revokingParty;
    ASN1_ENUMERATED      *revocationReason;
    ASN1_INTEGER         *signatureCounter;
    ASN1_PRINTABLESTRING *additionalInfo;
} REVOCATION_DETAILS;

DECLARE_ASN1_FUNCTIONS(REVOCATION_DETAILS)

and defined in a body together with the extension method

ASN1_SEQUENCE(REVOCATION_DETAILS) = { 
    ASN1_SIMPLE(REVOCATION_DETAILS, version, ASN1_INTEGER),
    ASN1_SIMPLE(REVOCATION_DETAILS, revokingParty, ASN1_ENUMERATED),
    ASN1_SIMPLE(REVOCATION_DETAILS, revocationReason, ASN1_ENUMERATED),
    ASN1_OPT(REVOCATION_DETAILS, signatureCounter, ASN1_INTEGER),
    ASN1_OPT(REVOCATION_DETAILS, additionalInfo, ASN1_PRINTABLESTRING),
} ASN1_SEQUENCE_END(REVOCATION_DETAILS)

IMPLEMENT_ASN1_FUNCTIONS(REVOCATION_DETAILS)

static int i2r_REVOCATION_DETAILS(const X509V3_EXT_METHOD *method, void *in, BIO *bp, int ind);

X509V3_EXT_METHOD v3_ext_tss = {
    NID_undef,                          /* .ext_nid = */
    0,                                  /* .ext_flags = */
    ASN1_ITEM_ref(REVOCATION_DETAILS),  /* .it = */
    NULL, NULL, NULL, NULL,
    NULL,                               /* .i2s = */
    NULL,                               /* .s2i = */
    NULL,                               /* .i2v = */
    NULL,                               /* .v2i = */
    i2r_REVOCATION_DETAILS,             /* .i2r = */
    NULL,                               /* .r2i = */
    NULL 
};

The main application creates an OID to add the extension method (error handling omitted):

tss_nid = OBJ_create("0.4.0.127.0.7.3.10.1.1.1", "id-TSS-CRL-entry-extension", "TSS revocation details");
v3_ext_tss.ext_nid = tss_nid;
X509V3_EXT_add(&v3_ext_tss);

During CRL creation, for every entry the extension is created, added and then freed (error handling omitted):

r = X509_REVOKED_new();
...
rd = REVOCATION_DETAILS_new();

rd->version = ASN1_INTEGER_new();
ASN1_INTEGER_set(rd->version, 1);

rd->revokingParty = ASN1_ENUMERATED_new();
ASN1_ENUMERATED_set(rd->revokingParty, rev_party);

rd->revocationReason = ASN1_ENUMERATED_new();
ASN1_ENUMERATED_set(rd->revocationReason, rev_reason);

rd->signatureCounter = ASN1_INTEGER_new();
ASN1_INTEGER_set(rd->signatureCounter, sig_count);

rd->additionalInfo = ASN1_PRINTABLESTRING_new();
ASN1_STRING_set(rd->additionalInfo, add_info, strlen(add_info));

X509_REVOKED_add1_ext_i2d(r, tss_nid, rd, 0, 0);

REVOCATION_DETAILS_free(rd);

X509_CRL_add0_revoked(crl, r);

Explicitly freeing and NULLing all the individual components before REVOCATION_DETAILS_free() does not make any difference. There remains a constant 72 bytes per REVOCATION_DETAILS object that is not freed.

[slontis]

Try printing out the fields of the object right after REVOCATION_DETAILS_new() is called..
I suspect rd->additionalInfo is not NULL.
If they are not NULL and then you are then doing rd->additionalInfo = ASN1_PRINTABLESTRING_new()
you will end up with floating memory.

[gingerbeard-man]

Thanks! Indeed some of the fields are not NULL. The string rd->additionalInfo is NULL, as is rd->signatureCounter, but I guess that is because I declared them optional. My problem was with rd->version and the two ENUMERATED which are already allocated and ready for use. That also fits the 3 blocks reported by Valgrind.

I must try to remember which existing application I based my code on, as it may very well have a similar leak. OpenSSL itself does not seem to have any places where a TYPE_new() is followed directly by setting the fields, but after following several levels of indirection I am now convinced that regular non-optional fields are allocated as part of the surrounding structure.