Opened 2 years ago

Closed 2 years ago

Last modified 18 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 2 years ago.

Download all attachments as: .zip

Change History (8)

Changed 2 years ago by dgoulet

Attachment: debug.log.tgz added

comment:1 Changed 2 years ago by nickm

Keywords: regression added
Priority: criticalblocker

comment:2 Changed 2 years ago by nickm

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

comment:3 Changed 2 years ago by dgoulet

Status: newneeds_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 2 years ago by dgoulet

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

Branch: bug17041_027_02

comment:5 Changed 2 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

merged that one!

comment:6 Changed 2 years ago by dgoulet

Keywords: SponsorR removed
Sponsor: SponsorR

comment:7 Changed 18 months ago by nickm

Keywords: 2016-bug-retrospective added

Marking for bug retrospective based on Priority.

Note: See TracTickets for help on using tickets.