Opened 8 years ago

Closed 6 months ago

#3569 closed enhancement (implemented)

Refactor socks parsing

Reported by: nickm Owned by: rl1987
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-client, intro, trunnel, prop229, refactor, code-quality, 035-triaged-in-20180711
Cc: linus@… Actual Points:
Parent ID: Points: 4
Reviewer: nickm Sponsor:

Description (last modified by rl1987)

The function parse_socks and its interactions with the functions that call it have grown nigh-unmaintainably complex. Let's replace it with a simple, more linear function. Key points:

  • State should be kept explicitly. Let's forget this "if the socks version is set, we've parsed this much, ..." business.
  • The function should dispatch first on state, next on anything else.
  • We should think of a much better interface; the functions that call parse_socks have grown way too tricky.

Child Tickets

Change History (37)

comment:1 Changed 8 years ago by ln5

Cc: linus@… added

comment:2 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final
Type: defecttask

comment:3 Changed 7 years ago by nickm

This is something we can do "the next time we need to change those functions."

comment:4 Changed 6 years ago by nickm

Keywords: tor-client added

comment:5 Changed 6 years ago by nickm

Component: Tor ClientTor

comment:6 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

comment:7 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.???

comment:8 Changed 4 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.7.x-final

These may be worth looking at for 0.2.7.

comment:9 Changed 4 years ago by nickm

Status: newassigned

comment:10 Changed 4 years ago by nickm

Keywords: 027-triaged-1-in added

Marking some tickets as triaged-in for 0.2.7 based on early triage

comment:11 Changed 4 years ago by isabela

Keywords: SponsorS added
Points: medium
Version: Tor: 0.2.7

comment:12 Changed 3 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final

comment:13 Changed 3 years ago by nickm

Keywords: SponsorS removed
Sponsor: SponsorS

Bulk-replace SponsorS keyword with SponsorS sponsor field in Tor component.

comment:14 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.???

It is impossible that we will fix all 252 currently open 028 tickets before 028 releases. Time to move some out. This is my first pass through the "assigned" tickets with no owner, looking for things to move to ???.

If somebody thinks they can get these done before the 0.2.8 timeout, please assign it to yourself and move it back?

comment:15 Changed 3 years ago by nickm

Keywords: intro added

comment:16 Changed 3 years ago by nickm

Keywords: SponsorS-deferred added
Sponsor: SponsorS

Remove the SponsorS status from these items, which we already decided to defer from 0.2.9. add the SponsorS-deferred tag instead in case we ever want to remember which ones these were.

comment:17 Changed 2 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:18 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:19 Changed 20 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:20 Changed 20 months ago by nickm

Keywords: 027-triaged-in added

comment:21 Changed 20 months ago by nickm

Keywords: 027-triaged-in removed

comment:22 Changed 20 months ago by nickm

Keywords: 027-triaged-1-in removed

comment:23 Changed 20 months ago by nickm

Keywords: trunnel prop229 refactor code-quality added; SponsorS-deferred removed
Points: medium4
Severity: Normal
Status: assignednew

This is a good use for trunnel, and a good time to implement proposal 229.

comment:24 Changed 20 months ago by nickm

Closed #11138 as a duplicate of this.

comment:25 Changed 8 months ago by rl1987

Owner: set to rl1987
Status: newassigned

comment:26 Changed 8 months ago by rl1987

Status: assignedneeds_review

Reworked SOCKS server code with trunnel: https://github.com/torproject/tor/pull/121

Once we merge this, I could work on making our SOCKS server part more like a state machine.

comment:27 Changed 8 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.5.x-final
Type: taskenhancement
Version: Tor: 0.2.7

Assigning to 0.3.5 because 0.3.4 is closed to new features

comment:28 Changed 7 months ago by dgoulet

Reviewer: catalyst

comment:29 Changed 7 months ago by catalyst

Reviewer: catalystnickm

comment:30 Changed 7 months ago by rl1987

Patch was rebased to fix merge conflicts: https://github.com/torproject/tor/pull/180

comment:31 Changed 7 months ago by nickm

Status: needs_reviewneeds_revision

This branch is looking very nice. I left a few small suggestions and requests on the github pull request.

And as I noted on the review, please ignore any comments that you addressed later in your branch. There are a lot of times when I said "this needs documentation" and later you documented it.

comment:32 Changed 7 months ago by rl1987

Description: modified (diff)
Status: needs_revisionneeds_review

comment:33 Changed 7 months ago by rl1987

Pushed a bunch of fixup commits to the branch and made some clarifications to the code.

comment:34 Changed 6 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:35 Changed 6 months ago by nickm

Status: needs_reviewmerge_ready

Okay, I've been over all the changes and looked at the comments. I squashed the branch, and then merged it forward, and then added a fuzzer for it. While fuzzing, I found one memory leak, and squashed it. I think I like this branch now.

Please have a look at branch socks_trunnel4_squashed_merged in my public repository, and let me know if you like it too? I've given it a PR at https://github.com/torproject/tor/pull/229

comment:36 Changed 6 months ago by rl1987

Looks good! I think we can merge it now.

comment:37 Changed 6 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged to master!

Note: See TracTickets for help on using tickets.