Opened 5 years ago

Last modified 7 months ago

#9390 needs_revision enhancement

Warn if you're being a public relay but have too-low file descriptor limit

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-relay, easy, dos, resources, logging, 033-triage-20180320, 033-removed-20180320
Cc: Actual Points:
Parent ID: Points:
Reviewer: teor Sponsor:

Description

Inspired by discussion on #3030: if we're a public relay, we should check if ulimit -n is too low (under 8192 I'm thinking, based on debian's init script), and warn (and recommend using a package) if so.

This is to help people who run Tor from tarball and don't realize all the fixes you have to do manually to do it right.

Child Tickets

Change History (31)

comment:1 Changed 5 years ago by nickm

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

comment:2 Changed 2 years ago by teor

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

Milestone renamed

comment:3 Changed 22 months 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:4 Changed 17 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:5 Changed 17 months ago by nickm

Keywords: easy dos resources logging added
Severity: Normal

comment:6 Changed 10 months ago by aruna1234

As far as I have understood the bug,a new function has to be created which would check whether the relay is a public relay and if it is then check whether the limit is under 8192. If yes then warn the user about the same. Let me know if I understood something wrong.
The only thing that I am not sure of is where will the new function be added?

comment:7 in reply to:  6 Changed 10 months ago by teor

Replying to aruna1234:

As far as I have understood the bug,a new function has to be created which would check whether the relay is a public relay

Use public_server_mode(get_options()) to check if tor is a relay.

and if it is then check whether the limit is under 8192. If yes then warn the user about the same. Let me know if I understood something wrong.
The only thing that I am not sure of is where will the new function be added?

You could add code at the end of set_max_file_descriptors() that checks limit.

Changed 10 months ago by aruna1234

comment:8 Changed 10 months ago by teor

Status: newneeds_revision

I'm sorry, this code isn't at the end of set_max_file_descriptors(), and it does not check if limit is less than 8192.

Changed 10 months ago by aruna1234

comment:9 Changed 10 months ago by aruna1234

Added the check and the code at the end of set_max_file_descriptors().

comment:10 Changed 10 months ago by teor

I'm sorry, this code isn't at the end of the function. And does it compile? It looks like it has an extra closing brace.

comment:11 Changed 10 months ago by teor

Hi,

This patch makes the function always return -1 on relays.

if(public_server_mode(get_options())){
  ...
  return -1;
}

That's not what we want, please delete the extra "return -1" line.

comment:12 Changed 10 months ago by teor

Thanks! This looks ok.

Would you mind attaching a single patch with all the changes in it?

comment:13 Changed 10 months ago by aruna1234

sure!

comment:14 Changed 10 months ago by teor

Thanks for this patch.

There is one remaining bug in this patch: we can't have set_max_file_descriptors() return -1 if there aren't enough file descriptors, because that causes options_act_reversible() to fail. We just want to warn the relay operator.

Here are the other things that someone will need to do before we merge it:

  • change the spacing to conform to our coding style:
    • two spaces indent,
    • spaces after "if" and before "{"
  • replace 8192 with a "#define" constant. For an example, look at how "ULIMIT_BUFFER" is defined, used, and commented
  • change the log message so it tells the operator what they can do to fix the issue ("raise the file descriptor limit, or install tor using a package manager")
  • add a comment that explains why we are doing the check

comment:15 Changed 10 months ago by teor

I'm sorry, I can't apply this branch to master using "git am".

This is what I see:

$ git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
$ git am 0001-Bug-9390-Check-for-public-relay-added.patch
Applying: Bug-9390 Check for public relay added
error: sha1 information is lacking or useless (src/common/compat.c).
error: could not build fake ancestor
Patch failed at 0001 Bug-9390 Check for public relay added

Please provide a patch that applies cleanly to master.

comment:16 Changed 10 months ago by aruna1234

Hey, I have pushed it to my branch:https://github.com/ArunaMaurya221B/tor/commit/47b08bab6a548a70f5148e4a91cfaee52bebb391.

Does this solve the issue?

comment:17 Changed 10 months ago by teor

Please make a separate branch that only has the changes for this ticket on it,
Please run "make check" on this branch before submitting it, and fix any errors.
In particular, this branch does not pass "make check-spaces".

comment:19 Changed 10 months ago by teor

Thanks for submitting this patch!
It looks like it's more complicated than we expected.
(That's ok, it's not your fault, sometimes things end up being harder once we try them.)

This patch doesn't compile, because the public_server_mode() and get_options() functions come from src/or, and this file is in src/common.

We need to:

  • add an argument to set_max_file_descriptors() that tells us whether this tor instance is a relay. In Tor, we use the int type for boolean arguments.
  • use the new argument in set_max_file_descriptors() to check if we want to log the new warning.
  • pass the result of calling public_server_mode(options) to set_max_file_descriptors() every time we call it in config.c.

I changed the macro name and warning message so they are consistent with other messages in my branch Bug-9390 at https://github.com/teor2345/tor.git

Please merge my Bug-9390 into your Bug-9390, then add another commit to your Bug-9390 with these changes.

You'll need to use commands like:

git checkout Bug-9390
git remote add teor https://github.com/teor2345/tor.git
git fetch teor
git merge teor/Bug-9390

And then make your changes, commit them, and push your Bug-9390 branch to github.

Thanks for sticking with us through this process!

comment:20 Changed 10 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.3.x-final
Reviewer: teor

comment:21 Changed 10 months ago by aruna1234

In order to check whether tor instance is a relay- public_server_mode(options()) is used? But it can't be added as an argument.But calling public_server_mode() is gonna again create an issue. How can I put a function call which returns a value in a declaration?

int set_max_file_descriptors(arg1, arg2, int value = public_server_mode(options()))?

comment:22 Changed 10 months ago by teor

You have to add an argument to the function, and then use public_server_mode(options) for that argument everywhere the function is called.

It's ok if you don't know enough C to do this, you can leave the ticket for someone else to do.

comment:23 Changed 7 months ago by nickm

Keywords: 033-triage-20180320 added

Marking all tickets reached by current round of 033 triage.

comment:24 Changed 7 months ago by nickm

Keywords: 033-removed-20180320 added

Mark all not-already-included tickets as pending review for removal from 0.3.3 milestone.

comment:25 Changed 7 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: unspecified

These tickets were marked as removed, and nobody has said that they can fix them. Let's remember to look at 033-removed-20180320 as we re-evaluate our triage process, to see whether we're triaging out unnecessarily, and to evaluate whether we're deferring anything unnecessarily. But for now, we can't do these: we need to fix the 033-must stuff now.

Note: See TracTickets for help on using tickets.