Revert changes from bug4312 and thereafter until 0.2.4.x

changed milestone to %Tor: 0.2.3.x-final in legacy/trac

added component::core tor/tor in Legacy / Trac milestone::Tor: 0.2.3.x-final in Legacy / Trac priority::medium in Legacy / Trac resolution::fixed in Legacy / Trac status::closed in Legacy / Trac tor-relay in Legacy / Trac type::defect in Legacy / Trac labels

Here's how I made the branches. To find commits which should maybe be reverted, I looked for things that changed tortls.c, connection_or.c, or compat_libevent.ch:

[528]$ git log --no-merges --reverse --format='%h %s' tor-0.2.3.8-alpha..master src/common/tortls.c src/or/connection_or.c src/common/compat_libevent.[ch] > revertable
[529]$ cp revertable revert
[530]$ vim revert

I then edited the file "revert" to have only the commits that we wanted to remove. Those are:

69a821e Refactor the SSL_set_info_callback() callbacks.
4fd79f9 Detect renegotiation when it actually happens.
ecd239e Detect and deny excess renegotiations attempts.
340809d Get rid of tor_tls_block_renegotiation().
e2b3527 Also handle needless renegotiations in SSL_write().
e097bff Fix issues pointed out by nickm.
406ae1b Use callback-driven approach to block renegotiations.
f77f9bd appease check-spaces
7920ea5 Refactor tor_event_base_once to do what we actually want
633071e Avoid a double-mark in connection_or_close_connection_cb
e8dde3a Fix some wide lines in tortls.c
9a88c0c use event_free() wrapper; fix bug 4582
617617e Don't schedule excess_renegotiations_callback unless it's set
40a87c4 indent; add comment
aba25a6 Make pending libevent actions cancelable
e27a26d Set renegotiation callbacks immediately on tls inititation

Note that we are NOT reverting the following commits even though they affect those files:

b42ff65 Use random bytes as our certificate serial numbers.
7b02d1a Clarify function documentation.
f89c619 Use the preferred address and port when initiating a connection.
529820f Use correct address family where necessary for bridges on IPv6.
2376a6a Merge node_get_{prim,pref,pref_ipv6}_addr with their _orport counterparts.
682a85f Don't just tell the controller "foo" on id mismatch

I then created the set of reverts automatically using:

% git checkout master
% git checkout -b revert_4312
% git revert --no-edit `cut -f 1  -d ' ' revert`

I then made a new set of commits to reapply the old changes using:

% git checkout -b reapply_4312
% git cherry-pick --no-edit `cut -f 1  -d ' ' revert`

So if I missed anything, let me know: it'll be easy to regenerate two new branches that have the right set of commits.

Trac:
Status: new to needs_review

Also, if we do this, we should make a new ticket for "merge the 4312 code again", and make all remaining bugs that affected that code child tickets of that ticket, targeting 0.2.4.x. We should triage the TLS/v2-handshake bugs carefully to see which fall into which category.

Looking at the list of to-be-reverted commits, do we need to revert f77f9bd or 40a87c4? We will also need to rip the relevant text out of the Changelog.

Apart from comment:3 it "Looks good!".

I did a diff between master and your branch and I couldn't see anything related to legacy/trac#4312 (moved) et al. forgotten in. I also studied git log master a couple of times to find any possible forgotten commits. Finally, I did some v2 handshakes with a maint-0.2.2 client and it seemed to work.

Let's also wait and see what effects this branch will have on gabelmoo.

gabelmoo lost the fast flag mid-day yesterday. Whatever my analysis was wrt asn't attempt to fix 4626 and this branch here can just go down the toilet, because I have no feel for how gabelmoo should behave without that flag. Preliminary results seem to indicate that even on 0.2.3.8-alpha, I'm sending way fewer bytes than I'm receiving. Maybe people only publish to me now, and I don't give out many consensuses/descriptors anymore because I'm not Fast.

Even going back to 0.2.2.34 doesn't mean I'm pushing more bytes. I feel like the change in flags threw us(at least me) off massively.

Even so, I think it might be helpful to revert so we can put out an 0.2.3.9-alpha that we do not know has problems.

wanoskarnet has pointed out a variety of problems in the reneg rate limiting stuff, and having a workable alpha release is holding up putting out a stable release.

I don't care whether we decide to delay it until 0.2.4 or just later in the 0.2.3 cycle. But I want to put out another alpha and expect it to mostly work. :)

So, IIUIC, with that branch (asn/bug4626_callback_conditions_theory), stuff does work. The thing that doesn't work is to actually limit renegotiations, but that is just as bad as it was before, so we don't actually lose anything. Of course we can also revert the branch, but maybe we get that revert wrong. hrm.

I'm running asn's b6c18f81f71 on moria1 and as far as I can tell it's working fine.

This is probably the wrong ticket to mention this, but bug4626_callback_conditions_theory (aka b6c18f81f71) should not be considered The Commit That Fixes Everything, if we want an -alpha any time soon.

There are still bugs lurking in the code, like for example:

<wanoskarnet> ugh. ssl3_read_bytes(). seems like if SSL_ST_OK after calling handshake then it going to get record. if any one record there then it returns number of bytes n
ot a error. most times it's impossible because non-blocking io, but non impossible. SSL_read() can return non error if reneg for some extremal cases.

I'm just mentioning this here, because I see Roger still considering b6c18f81f71.

Roger, how did revert_4312 work out for moria1?

Replying to asn:

This is probably the wrong ticket to mention this, but bug4626_callback_conditions_theory (aka b6c18f81f71) should not be considered The Commit That Fixes Everything, if we want an -alpha any time soon.

Ok. I would like an alpha rsn.

Roger, how did revert_4312 work out for moria1?

Fine. I just switched moria1 to it.

I agree that somebody should help me clean out the changelog stanzas (or just do it) once we revert.

Merged revert_4312, and removed the 4312 stuff from the changelog.

Now I am going to make a new "restore/revise 4312" ticket to revert to restore_4312, bug4626_callback_conditions_theory, etc.

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

f77f9bd and 40a87c4 were also reverted.

f77f9bd was "appease check-spaces" and got reverted by 53f535aeb863204470379b2da4631770fa10b13f. It was a commit by Sebastian that made check-spaces happy.

40a87c4 was "indent; add comment" and got reverted by 75134c6c86e54c10fd9e11c4345aadcdabc0f8fb. It was a commit by Nick that improved the new certificate serial number generation code.

Trac:
Keywords: N/A deleted, tor-relay added

Trac:
Component: Tor Relay to Tor

closed

mentioned in issue legacy/trac#4668 (moved)

mentioned in issue legacy/trac#4672 (moved)

moved from legacy/trac#4662 (moved)

mentioned in issue #4672 (closed)

added Bug label and removed 1 deleted label

added Relay label and removed 1 deleted label

Revert changes from bug4312 and thereafter until 0.2.4.x

Child items ...

Activity