Opened 4 years ago

Closed 4 years ago

Last modified 14 months ago

#17417 closed defect (fixed)

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

Reported by: yancm Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Starting in the last 2-5 days, I started getting failed self-test on cert_new as shown:

tortls/create_certificate: [forking] OK
tortls/cert_new: [forking] [Lost connection!]

[cert_new FAILED]

just built 'tor -V' shows:

Oct 23 20:41:00.966 [notice] Tor v0.2.8.0-alpha-dev (git-8acaac4622c67ce8)
running on NetBSD with Libevent 2.1.5-beta, OpenSSL 1.1.0-dev and Zlib 1.2.3.

*NOTE* : I'm building against HEAD on OpenSSL 1.1.0-dev ... in case the bug is an OpenSSL regression...

Child Tickets

Change History (10)

comment:1 Changed 4 years ago by rl1987

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]

comment:2 Changed 4 years ago by rl1987

Status: newneeds_review

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

Last edited 4 years ago by rl1987 (previous) (diff)

comment:3 Changed 4 years ago by yancm

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

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

comment:4 Changed 4 years ago by rl1987

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

comment:6 Changed 4 years ago by yancm

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...

comment:7 Changed 4 years ago by rl1987

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.

comment:8 Changed 4 years ago by 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?

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?

comment:9 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged rl1987/bug17417_take2 -- thanks!

comment:10 in reply to:  8 Changed 14 months ago by rl1987

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.

Note: See TracTickets for help on using tickets.