#25943 closed defect (fixed)

Investigate and fix tortls/x509_cert_free test failure on win32 CI builds

Reported by: isis Owned by: saper
Priority: Medium Milestone:
Component: Core Tor/Tor Version: Tor: 0.3.3.5-rc
Severity: Normal Keywords: tor-ci tor-windows tor-testing
Cc: Actual Points:
Parent ID: #25549 Points: 1
Reviewer: Sponsor:

Description

From #25549, the tortls/x509_cert_free test is failing with:

tortls/x509_cert_free: [forking] 
  [x509_cert_free FAILED]

Child Tickets

Change History (13)

comment:1 Changed 14 months ago by catalyst

Parent ID: #25549

comment:2 Changed 14 months ago by saper

Kind of unrelated, but what is the following needed for?

cert->cert = tor_malloc_zero(sizeof(X509));

comment:3 Changed 14 months ago by nickm

It looks like it's trying to make some object to fill in the "cert" pointer, but it's doing it in a bogus way: It should be creating its X509 object using X509_new, since they will later be freed with X509_free().

That could easily be the cause of this problem.

comment:4 Changed 14 months ago by saper

https://github.com/torproject/tor/pull/103 submitted to remove this (which btw. fixes the issue)

comment:5 Changed 14 months ago by saper

There is no "cert" member in this structure.

comment:6 Changed 14 months ago by nickm

If there were no cert member, I think the code wouldn't compile. The cert member is declared here, as the first member of the structure (from tortls.h, line 84):

/** Structure that we use for a single certificate. */
struct tor_x509_cert_t {
  struct x509_st *cert;
  uint8_t *encoded;
  size_t encoded_len;
  unsigned pkey_digests_set : 1;
  common_digests_t cert_digests;
  common_digests_t pkey_digests;
};

That said, it seems we likely have the right diagnosis. What if you replace the tor_malloc_zero(sizeof(X509)); with X509_new()?

comment:7 Changed 14 months ago by saper

Status: newneeds_review

comment:8 Changed 14 months ago by saper

Owner: set to saper
Status: needs_reviewaccepted

comment:9 Changed 14 months ago by saper

Status: acceptedneeds_review

comment:10 Changed 14 months ago by saper

Status: needs_reviewneeds_revision

comment:11 Changed 14 months ago by saper

Looks like this is it. OpenSSL we are using for Windows seems to have stack checking enabled.

comment:12 Changed 14 months ago by saper

Status: needs_revisionneeds_review

comment:13 Changed 14 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Excellent! I've added a changes file and merged your patch.

Note: See TracTickets for help on using tickets.