Opened 6 years ago
Closed 4 years ago
#6480 closed defect (fixed)
double connection_free() in dns_resolve()
Reported by: | arma | Owned by: | |
---|---|---|---|
Priority: | Medium | Milestone: | Tor: 0.2.3.x-final |
Component: | Core Tor/Tor | Version: | |
Severity: | Keywords: | tor-relay 022-backport | |
Cc: | Actual Points: | ||
Parent ID: | Points: | ||
Reviewer: | Sponsor: |
Description
If dns_resolve()'s call to dns_resolve_impl() returns -1, it ends up running
dns_cancel_pending_resolve(exitconn->_base.address); if (!exitconn->_base.marked_for_close) { connection_free(TO_CONN(exitconn));
But dns_cancel_pending_resolve() runs
while (resolve->pending_connections) { ... if (!pendconn->_base.marked_for_close) connection_free(TO_CONN(pendconn));
So we would end up calling connection_free() on it twice. But we don't in practice, since the first connection_free() scribbles 0xCC on it, which sets marked_for_close to true, so we don't free it the second time! Cue Nick's circus music.
Our friendly irc person says "fix not so easy btw, connection_free() still need to call if no it was attached to pending resolve list."
Child Tickets
Change History (10)
comment:1 Changed 6 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 6 years ago by
Adapted that patch, and added some more comments, in branch "bug6480" in my public repo. Personally I'd like to stick it in 0.2.3, but if you think it's risky, we can try it out in 0.2.4.x for a bit first.
comment:3 Changed 6 years ago by
Based on a conversation on IRC, I now think that maybe this should be "major" and in 0.2.3 with a possible backport to 0.2.2. It *is* a remote freed-memory read. If somebody's running Tor under valgrind, or with a particularly aggressive/paranoid malloc implementation, this could turn into a remote crash for them.
comment:4 Changed 6 years ago by
Milestone: | Tor: 0.2.4.x-final → Tor: 0.2.3.x-final |
---|
comment:5 Changed 6 years ago by
I think putting it in 0.2.3 is fine.
I don't think backporting to 0.2.2 will be worth our while. I want 0.2.2 to be solidly frozen except for super critical things.
comment:6 Changed 6 years ago by
Milestone: | Tor: 0.2.3.x-final → Tor: 0.2.2.x-final |
---|
Okay. Merged "bug6480_squashed" to maint-0.2.3. Moving this ticket to 0.2.2, just in case.
comment:7 Changed 6 years ago by
Keywords: | tor-relay added |
---|
comment:8 Changed 6 years ago by
Component: | Tor Relay → Tor |
---|
comment:9 Changed 5 years ago by
Keywords: | 022-backport added |
---|
comment:10 Changed 4 years ago by
Milestone: | Tor: 0.2.2.x-final → Tor: 0.2.3.x-final |
---|---|
Resolution: | → fixed |
Status: | needs_review → closed |
We're dropping 0.2.2 servers from the Tor network. Nothing gets backported to 0.2.2 for server-side at this point.
Our friendly irc person suggests this patch:
which looks pretty straightforward.
I assigned the ticket to 0.2.4 originally since it isn't occurring in practice. We might want to move that to 0.2.3 if we become confident of the diagnosis and fix.