Re: [tor-bugs] #9249 [Tor]: GSOC seccomp stage 2

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#9249: GSOC seccomp stage 2
-----------------------------+--------------------------------------------
     Reporter:  ctoader      |      Owner:  nickm
         Type:  enhancement  |     Status:  needs_review
     Priority:  normal       |  Milestone:
    Component:  Tor          |    Version:
   Resolution:               |   Keywords:  tor-relay gsoc seccomp sandbox
Actual Points:               |  Parent ID:  #5756
       Points:               |
-----------------------------+--------------------------------------------

Comment (by nickm):

 Notes:
   * Does MAX_PARAM_LEN really mean that we can't have a filename longer
 than 64 characters? That sounds poor. Those happen all the time in real
 life.
   * Using strncmp instead of strcmp (in various places in sandbox.c) seems
 likely to give a wrong result if one value is a prefix of the other.
 strcmp() should be safe instead.
   * Do we truly only use getaddrinfo in the code to look up our own
 address?
   * Use doxygen style for documenting structure elments too.
   * I'd still be more comfortable if the data structure for pfd_elem
 weren't so closely tied to the sandbox implementation. At the very least,
 it shouldn't be exposed in the header if it's implementation-specific.
   * Prefer TOR_SLIST or TOR_SINGLEQ to manually managing a linked list.
 (They're defined in src/ext/tor_queue.h)
   * All functions that take no arguments need to be declared as foo(void),
 not as foo().  ex: sandbox_cfg_new() in sandbox.h, sandbox_init_filter()
 in main.c
   * Use "int" for a boolean, not "char".  (There's almost never a reason
 to use char this way)
   * We should be supporting /dev/urandom, not /dev/random
   * When we call openssl's RAND_poll  periodically, does that call still
 work?
   * In the functions that take an variable of filenames, it seems fragile
 to have to include the count of the filenames.  What if instead we make
 them end with NULL.?

 Also:
 >> *It seems excessive to use a separate mmap() for every prot_strdup()
 call. Perhaps instead we should have a big mmap that fills up with
 strings, and which we mprotect when we're done. This could wait until
 later though.
 > I don't really see the upside to that other than reducing the number of
 calls from N to 1; the downside however would be having to manage that
 memory similar to malloc/calloc but having only one allocated segment (or
 maybe have another block allocated when a limit is reached..?); do you
 consider it is worth the added code complexity?

 Well, it does mean that we use an entire memory page for every interned
 string.  That seems inefficient.

 You don't need malloc/calloc here, since these strings are never freed.
 Instead, you could probably use something closer to what we have in
 memarea.c.  (For example, you could make an alternative constructor for
 memarea_t that takes a function which it uses to get a new chunk.)  This
 isn't a change you need to make yourself if you don't want to, though; i'm
 happy to hack it in later.

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9249#comment:8>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs