fail self-test for cert_new in v0.2.8.0-alpha-dev NetBSD 6_Stable

changed milestone to %Tor: 0.2.8.x-final

added component::core tor/tor milestone::Tor: 0.2.8.x-final priority::medium reporter::yancm resolution::fixed severity::normal status::closed type::defect version::tor unspecified labels

The above testcase crashes on Mac OS X 10.10.5 with current OpenSSL master (070c23325af4526c9a8532a60d63522c58d5554b) :

tortls/cert_new: [forking] 
============================================================ T= 1445682546
Tor 0.2.8.0-alpha-dev died: Caught signal 11
0   test                                0x00000001003a5d9d crash_handler + 45
1   libsystem_platform.dylib            0x00007fff917c0f1a _sigtramp + 26
2   test                                0x000000010043a249 CRYPTO_add_lock + 89
3   test                                0x0000000100507115 asn1_item_embed_free + 117
4   test                                0x0000000100507097 ASN1_item_free + 23
5   test                                0x00000001003ca9fe tor_x509_cert_free + 46
6   test                                0x00000001002adddd testcase_run_one + 845
7   test                                0x00000001002ae2d3 tinytest_main + 499
8   test                                0x00000001002ad75e main + 606
9   libdyld.dylib                       0x00007fff961e95c9 start + 1
[Lost connection!] 
  [cert_new FAILED]

I believe this is a double-free bug in the unit test. Quick fix: https://github.com/rl1987/tor/commit/10cc51b32f914b464882e2e1f7779155cb62201f

Trac:
Status: new to needs_review

The above change does allow all tests to pass OK for me.

It says "quick fix", is this fix undesirable?

Trac:
Username: yancm

I'm afraid it introduces a memory leak for older OpenSSL versions.

A better way to fix this: https://github.com/rl1987/tor/commit/e2f9c7d54f20219da5d75a58196f21659774cafa

OK, that "fixes" it too, but... just setting ret = NULL seems kluge-y? If the value of ret should be NULL when coming back from tor_x509_cert_free(), why not fix this in that function?

I apologize if these are impertinent comments, but thought I should speak up... no offense intended none to be felt from your reply...

Trac:
Username: yancm

Here's my reasoning. tor_x509_cert_new() allocates a tor_x509_cert_t object. tor_x509_cert_free() then frees it. At the end of the function, it tries to free it again. One of the ways to avoid double-freeing things is to set the pointer to NULL after first freeing the memory. Note that ret the pointer cannot be cleared inside tor_x509_cert_free() because tor_x509_cert_free() receives a new copy of that pointer and cannot do anything about the old copy in the testcase.

If you compile Tor with non-cutting-edge OpenSSL, this portion of code will be compiled in:

 #ifndef OPENSSL_OPAQUE
   cert = read_cert_from(validCertString);
   X509_CINF_free(cert->cert_info);
   cert->cert_info = NULL;
   ret = tor_x509_cert_new(cert);
   tt_assert(ret);
 #endif

Here, it creates a new tor_x509_cert_t object. If I refrained from freeing the memory before the new tor_x509_cert_t instance is created (like I did in my first patch), everyone would forget about the old instance and the memory would be leaked.

Now, if I free the previous tor_x509_cert_t instance and nullify the pointer, there will be no memory leak regardless the OPENSSL_OPAQUE macro. And there is no double-free crashbug because tor_x509_cert_free(NULL) is no-op.

Ah, ok... If I understand the short of it; with bleeding edge OpenSSL, ret is passed by value to tor_x509_cert_free() so ret cannot be set to NULL there? And later in the test subroutine, we free ret again?

It seems odd to me to call a "free" anything (memory/records/etc) without returning a success or fail code, or some other status that can be reacted to...then you know when you NULL ret it has indeed been successfully free'd first... maybe we do not care if we NULL ret even if it was not successfully free'd?

Trac:
Username: yancm

Merged rl1987/bug17417_take2 -- thanks!

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

Replying to yancm:

Ah, ok... If I understand the short of it; with bleeding edge OpenSSL, ret is passed by value to tor_x509_cert_free() so ret cannot be set to NULL there? And later in the test subroutine, we free ret again?

Yes.

It seems odd to me to call a "free" anything (memory/records/etc) without returning a success or fail code, or some other status that can be reacted to...then you know when you NULL ret it has indeed been successfully free'd first... maybe we do not care if we NULL ret even if it was not successfully free'd?

Well, we're done with this instance of the object and we're not going to need it so we're freeing it. The lifecycle is over. We shouldn't worry about free() failing as it shouldn't happen.

Trac:
Reviewer: N/A to N/A

closed

moved to tpo/core/tor#17417 (closed)

fail self-test for cert_new in v0.2.8.0-alpha-dev NetBSD 6_Stable

Child items ...

Activity