Opened 23 months ago

Closed 23 months ago

Last modified 16 months ago

#17041 closed defect (fixed)

Memory corruption in the HS client

Reported by: dgoulet Owned by:
Priority: Immediate Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-hs, regression, 2016-bug-retrospective
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor: SponsorR

Description

This is in git master and hasn't been released.

Here is how the bug is triggered. You download a descriptor of a valid HS. Then restart that HS (thus making the current descriptor obsolete) and retry right away to download the descriptor for that HS. The tor client stops with a segfault in malloc() (you sometime need couple of tries to trigger the issue).

Now I believe this is a memory corruption of some sort since during the git bisect, I was able to trigger bad free() and other segfaults with tor_memcmp() in some other non related functions with the same usecase. Bisect gave me this commit as the first bad commit:

commit ab9a0e340728abd96128da726f67b4ccca10ba52
Author: David Goulet <dgoulet@ev0ke.net>
Date:   Thu Jun 18 16:09:18 2015 -0400

    Add rend failure cache
[...]

That precise commit introduces a memory corruption somewhere somehow, I can't find it for now so I'm filling this ticket. Attached is a debug log (3.3M) of the issue being triggered. It's also quite easy to run tor in gdb and catch the issue.

Child Tickets

Attachments (1)

debug.log.tgz (406.2 KB) - added by dgoulet 23 months ago.

Download all attachments as: .zip

Change History (8)

Changed 23 months ago by dgoulet

comment:1 Changed 23 months ago by nickm

  • Keywords regression added
  • Priority changed from critical to blocker

comment:2 Changed 23 months ago by nickm

can you reproduce this under valgrind, or with dmalloc, or some similar tool?

comment:3 Changed 23 months ago by dgoulet

  • Status changed from new to needs_review

Here is the fix with some extra goodies in the patch. Ouff... that one took me a while to figure out! :P

Branch: bug17041_027_01

comment:4 Changed 23 months ago by dgoulet

To nickm's request, here is a branch with *only* the bugfix.

Branch: bug17041_027_02

comment:5 Changed 23 months ago by nickm

  • Resolution set to fixed
  • Status changed from needs_review to closed

merged that one!

comment:6 Changed 22 months ago by dgoulet

  • Keywords SponsorR removed
  • Sponsor set to SponsorR

comment:7 Changed 16 months ago by nickm

  • Keywords 2016-bug-retrospective added

Marking for bug retrospective based on Priority.

Note: See TracTickets for help on using tickets.