Re: (Desperate) Plea for multi-person code review

[Nick Mathewson <nickm@xxxxxxxxxxxxx>]

On Sat, Feb 20, 2010 at 6:27 PM, Mike Perry <mikeperry@xxxxxxxxxx> wrote:
> Ok, consensus-bw-weights4 should have fixes for these, with the
> exception of the XXX in find_start_of_next_routerstatus() that you
> mention. That XXX actually is for the old code, which seemed to
> preserve the \n for some reason there.
>
> Nick, do you think you could have a look at that function, and see
> what should be done there? It looks like it was possibly a bug or just
> bad behavior to me.

I think it's a bug, but a harmless one.  If we fix it, we need to make
sure that everything that calls find_start_of_next_routerstatus()
works fine if it gets the actual start of the routerstatus, not one
character before.  This means at least that we'd need to test parsing
consensuses and v2 networkstatus documents.

Or for stability we could convert the XXX into an explanatory comment,
but leave it alone otherwise.

> In general, I'd also like someone to verify that adding the new
> 'directory-footer' line and 'bandwidth-weights' line to the consensus
> won't break existing clients. It looks to me like the parsing code is
> written to handle the addition of arbitrary new lines in the
> consensus, but I'd like some confirmation of that. For example, is it
> possible that the last routerstatus document in the consensus might
> get silently rejected due to the parser finding the extra lines there?
> routerstatus_parse_entry_from_string() looks like it might be OK with
> that to me, but I'm not familiar with all of the bits of the parsing
> code.

According to the spec:

 When interpreting a Document, software MUST ignore any KeywordLine that
 starts with a keyword it doesn't recognize; future implementations MUST NOT
 require current clients to understand any KeywordLine not currently
 described.

All the parsing code goes through get_next_token(), which translates
unrecognized keywords into K_OPT, which is ignored everywhere.
Specifically, nothing in routerstatus_parse_entry_from_string does
anything to reject routerstatuses with extraneous entries.

--
Nick