Opened 6 months ago

Closed 4 months ago

Last modified 6 weeks ago

#27244 closed enhancement (implemented)

Use mmap to hold our cached consensus, when we even need it stored in RAM at all.

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 035-roadmap-master, 035-triaged-in-20180711
Cc: Actual Points: 1
Parent ID: #27243 Points:
Reviewer: ahf Sponsor: Sponsor8

Description


Child Tickets

TicketTypeStatusOwnerSummary
#27629enhancementclosedadd len argument to consensus parsing functions

Change History (23)

comment:1 Changed 6 months ago by nickm

Summary: Use mmap to hold our cached consensus, when we even need in RAM at all.Use mmap to hold our cached consensus, when we even need it stored in RAM at all.

comment:2 Changed 6 months ago by nickm

Hm, it looks like this might be the same as #27247, or redundant with it. Maybe we should look again once #27247 is done.

comment:3 Changed 6 months ago by nickm

Turns out not to be redundant with #27247. Clients no longer need to keep a cached_dir_t, and that ticket stops them -- but when they load it, they still load it with read_file_to_string().

There's a slight difficulty in moving to mmap, however. When we mmap() a file, it is not a NUL-terminated string, but rather a non-terminated pile of bytes with an associated size_t.

There are three cases when a client needs a consensus text:

  1. It's parsing it from disk
  2. It's applying a consensus diff.
  3. It's writing it to the controller

Case 3 is easy enough, since the client doesn't need to examine the bytes at all.

But for case 1 and case 2, our networkstatus parsing code and our consensus diff code both assume that the mapped object is NUL-terminated.

So before we can do this, we'll need to carefully revise a bunch of functions.

comment:4 Changed 6 months ago by nickm

(For the non-client case, see #27563.)

comment:5 in reply to:  3 ; Changed 5 months ago by cyberpunks

Replying to nickm:

So before we can do this, we'll need to carefully revise a bunch of functions.

Carefully revised a bunch of functions in #27629.

Also, there's no way this is making the 0.3.5 milestone, is there? Same with parent ticket.

Last edited 5 months ago by cyberpunks (previous) (diff)

comment:6 in reply to:  5 Changed 5 months ago by teor

Replying to cyberpunks:

Replying to nickm:

So before we can do this, we'll need to carefully revise a bunch of functions.

Carefully revised a bunch of functions in #27629.

Thanks! See my review in that ticket.

Also, there's no way this is making the 0.3.5 milestone, is there? Same with parent ticket.

We're focusing on sponsored work until the feature freeze on Friday. This ticket is sponsored work, so it might get in to 0.3.5.

Please be patient with us during the next few weeks. We appreciate your contributions, but we don't have enough people to review everything this week. We'll review your branches as soon as we get time.

If you'd like to speed up reviews on your code, you can:

  • run Travis or Appveyor CI on your branches, and link to the CI in your tickets
  • make sure there are unit tests for any code that you add or modify

comment:7 Changed 5 months ago by teor

Type: defectenhancement

comment:8 Changed 5 months ago by nickm

Status: newneeds_review

Oh! I hadn't seen #27629. Maybe we can use some of this too?

I just did a branch to implement this ticket called networkstatus_mmap, with PR at https://github.com/torproject/tor/pull/316 . Now I'll have a look at #27629

I think it might be necessary to defer this to 0.3.6 in any case: historically, bugs in our parsing code have been really gross to discover.

comment:9 in reply to:  8 Changed 5 months ago by cyberpunks

Replying to nickm:

I just did a branch to implement this ticket called networkstatus_mmap, with PR at https://github.com/torproject/tor/pull/316 .

This will read past the end of the mmap into invalid memory. It's missing some changes that are in the branch parselen1-on-ticket27247 (rebased on #27247 obviously).

Conversely, that branch was missing another mmap usage added in yours. (Added now.)

It looks like there are unrelated test fixups in the Stop memcpy'ing uncompressed consensuses when making diffs commit?

Github also seems to be wrongly showing the commit Replace "read consensus from disk" with "map consensus from disk". as being before Revise networkstatus parsing code to use lengths, when tpo shows the opposite.

Last edited 5 months ago by cyberpunks (previous) (diff)

comment:10 Changed 5 months ago by teor

Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final

I spoke to nickm, and we don't think it's a good idea to change parsing just before a feature freeze. Let's merge this ticket in 0.3.6 instead.

comment:11 Changed 4 months ago by dgoulet

Reviewer: ahf

comment:12 Changed 4 months ago by ahf

Status: needs_reviewmerge_ready

Very nice. Looking forward to hear from the iOS people if this helps with their memory reduction goals.

I added one very minor optional comment that could be added, but otherwise I think the patches looks good.

comment:13 Changed 4 months ago by ahf

This will read past the end of the mmap into invalid memory. It's missing some changes that are in the branch parselen1-on-ticket27247 (rebased on #27247 obviously).

Could you elaborate a bit on what and where the issue is Nick's patches?

comment:14 in reply to:  13 Changed 4 months ago by cyberpunks

Replying to ahf:

Could you elaborate

networkstatus_set_current_consensus() passes s to a few functions (dirserv_set_cached_consensus_networkstatus, consdiffmgr_add_consensus) which don't have length arguments. They still assume the string is null-terminated and will read past the end of the mmap.

This branch doesn't have that issue.
https://gitgud.io/onionk/tor/compare/master...consparsemmap

comment:15 Changed 4 months ago by ahf

Status: merge_readyneeds_revision

Good catch! Let's get those issues fixed in Nick's branch then before we get it in.

comment:16 Changed 4 months ago by nickm

okay, I've fixed those, plus one more (write_str_to_file). Thanks!

comment:17 Changed 4 months ago by nickm

Status: needs_revisionneeds_review

comment:18 in reply to:  16 Changed 4 months ago by cyberpunks

Replying to nickm:

okay, I've fixed those

There's also merge conflicts after all the refactoring. The consparsemmap branch is already rebased.

comment:19 Changed 4 months ago by ahf

Status: needs_reviewmerge_ready

I think these patches looks good. Let's get them merged.

@cyberpunks: Thanks a lot for the work you have put into this ticket (via #27629). I think your code looks good and I would have accepted this if there wasn't two implementations that was practically the same.

Let me try to explain a little bit why I ended up picking up the patches in PR #316 from Nick: When I review code I start out by looking at the "accumulated diff" (I ignore all the individual commits, but just focus on what was there before this set of changes vs. what is there after this set of changes). Once I'm happy with that, I walk over each individual patch to get an idea about the process on how you got there. In Tor, we very often use the "fixup" feature of Git for this very reason, so we can easily spot which things have been changed since last time we reviewed it. I looked at your changes and Nick changes and Nick's changes was easier (for me) to follow based on the *commits* themselves. I'm very happy you helped identifying the issues with the missing strlen() calls in Nick's changes, which was indeed bugs in the code. I hope this won't discourage you from contributing in the future, since your changes were in fact very good.

comment:20 Changed 4 months ago by nickm

okay, ahf. I've done a merge PR as https://github.com/torproject/tor/pull/462 , and I'm fuzzing that again. If all is well with CI and fuzzing, I'll merge.

comment:21 Changed 4 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Stuff passed; merging!

comment:22 Changed 4 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:23 Changed 6 weeks ago by nickm

Actual Points: 1
Note: See TracTickets for help on using tickets.