Ticket30007 refactor control response formatting #956

tlyu · 2019-04-15T19:49:13Z

overview:

split low level control reply formatting out of control_fmt.c
create abstractions to format the control protocol response syntax so that individual events or commands don't roll their own formatting
use Coccinelle to transform existing explicit formatting into calls to the new functions
some manual cleanup, both of whitespace quirks introduced by Coccinelle and some manual edits that it's not economical to write a Coccinelle script to accomplish

Use a table-based lookup to find the right command handler. This will serve as the basement for several future improvements, as we improve the API for parsing commands.

There _is_ an underlying logic to these commands, but it isn't wholly uniform, given years of tweaks and changes. Fortunately I think there is a superset that will work. This commit adds a parser for some of the most basic cases -- the ones currently handled by getargs_helper() and some of the object-taking ones. Soon will come initial tests; then I'll start using the parser. After that, I'll expand the parser to handle the other cases that come up in the controller protocol.

This is preliminary work for fixing 29984; no behavior has changed.

The first line break in particular was mishandled: it was discarded if no arguments came before it, which made it impossible to distinguish arguments from the first line of the body. To solve this, we need to allocate a copy of the command rather than using NUL to separate it, since we might have "COMMAND\n" as our input. Fixes ticket 29984.

(This should be all of the command that work nicely with positional arguments only.) Some of these commands should probably treat extra arguments as incorrect, but for now I'm trying to be careful not to break any existing users.

The two options are mutually exclusive, since otherwise an entry like "Foo" would be ambiguous. We want to have the ability to treat entries like this as keys, though, since some controller commands interpret them as flags.

This should let us handle all (or nearly all) of the remaining commands.

Here we get to throw away a LOT of unused code, since most of the old parsing was redundant with kvline.

This command does not fit perfectly with the others, since its second argument is optional and may contain equal signs. Still, it's probably better to squeeze it into the new metaformat, since doing so allows us to remove several pieces of the old command-parsing machinery.

This function decodes something different from the usual c-escaped format. It is only used in controller authorization.

These two presented their own challenge, because of their use of QString, and their distinguished handling of quoted versus non-quoted values.

Now that the legacy handlers are gone, we can simplify the structures and macros here.

Split the core reply formatting code out of control_fmt.c into control_proto.c. The remaining code in control_format.c deals with specific subsystems and will eventually move to join those subsystems.

Refer to control-spec.txt grammar productions in comments in control_proto.c for clarity.

Create a set of abstractions for controller commands and events to output replies to the control channel. The control protocol has a relatively consistent SMTP-like structure, so it's helpful when code that implements control commands and events doesn't explicitly format everything on its own.

Manually fix up some reply-generating code that the Coccinelle scripts won't match. Some more complicated ones remain -- these are mostly ones that accumulate data to send, and then call connection_buf_add() or connection_write_str_to_buf() directly.

Clean up some minor formatting quirks after the Coccinelle run.

tlyu · 2019-04-15T19:51:31Z

This includes the commits of #940. The manual merge commit is because GitHub doesn't seem willing to make a trial merge commit automatically for this branch.

coveralls · 2019-04-15T20:40:34Z

Pull Request Test Coverage Report for Build 4742

123 of 583 (21.1%) changed or added relevant lines in 9 files are covered.
105 unchanged lines in 6 files lost coverage.
Overall coverage increased (+0.1%) to 62.389%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/core/mainloop/connection.c	0	1	0.0%
src/lib/encoding/kvline.c	20	21	95.24%
src/lib/encoding/confline.c	0	3	0.0%
src/feature/control/control_getinfo.c	0	9	0.0%
src/feature/control/control.c	0	27	0.0%
src/lib/encoding/qstring.c	0	32	0.0%
src/feature/control/control_auth.c	0	47	0.0%
src/feature/control/control_proto.c	42	100	42.0%
src/feature/control/control_cmd.c	61	343	17.78%

Files with Coverage Reduction	New Missed Lines	%
src/feature/control/control.c	1	10.89%
src/feature/control/control_auth.c	3	12.09%
src/feature/dirauth/shared_random.c	3	85.64%
src/feature/control/control_getinfo.c	4	27.75%
src/feature/dirauth/dirvote.c	9	64.74%
src/feature/control/control_cmd.c	85	15.7%

Totals
Change from base Build 4732:	0.1%
Covered Lines:	46265
Relevant Lines:	74156

💛 - Coveralls

tlyu · 2019-04-26T19:15:17Z

Superseded by #984, which is a rebased version.

nmathewson and others added 30 commits Apr 12, 2019

Improve handling of controller commands

d1f5957

Use a table-based lookup to find the right command handler. This will serve as the basement for several future improvements, as we improve the API for parsing commands.

Add assertions for correct input to handle_control_command.

f3bd024

Define two more commands as wipe-after-parse.

e9ca904

Extract the argument-splitting part of control.c's parser

a502017

This is preliminary work for fixing 29984; no behavior has changed.

Unit tests for current control-command parser logic

86c2b1b

Use parsing code for the simpler controller commands.

e3fcbff

(This should be all of the command that work nicely with positional arguments only.) Some of these commands should probably treat extra arguments as incorrect, but for now I'm trying to be careful not to break any existing users.

kvline: handle empty alues as well as empty keys

bb2062d

The two options are mutually exclusive, since otherwise an entry like "Foo" would be ambiguous. We want to have the ability to treat entries like this as keys, though, since some controller commands interpret them as flags.

Add fuzzing support for several more groups of kvlines flags

be8c400

Allow kvlines in control commands.

00500d4

Add kvline support to controller command parser.

a118e9c

This should let us handle all (or nearly all) of the remaining commands.

Add a case-insensitive variant to config_line_find()

612f73d

Update more controller commands, now that we have kvline support

5d3dcd6

Use new parser logic for SETCONF/RESETCONF code.

3eed319

Here we get to throw away a LOT of unused code, since most of the old parsing was redundant with kvline.

Move and rename decode_escaped_string()

23b7dc8

This function decodes something different from the usual c-escaped format. It is only used in controller authorization.

Add rudimentary qstring support to kvline.c

4e87743

Port the authenticate and authchallenge commands to the new parser

3761716

These two presented their own challenge, because of their use of QString, and their distinguished handling of quoted versus non-quoted values.

Simplify handler logic in control_cmd.c

6255656

Now that the legacy handlers are gone, we can simplify the structures and macros here.

expand CMD_FL_WIPE to wipe the parsed arguments too

aca3103

changes file for ticket 30091 (controller parsing refactor)

957b89f

Correct file name in doxygen comment

fbdc64a

Split reply formatting out of control_fmt.c

8bcb406

Split the core reply formatting code out of control_fmt.c into control_proto.c. The remaining code in control_format.c deals with specific subsystems and will eventually move to join those subsystems.

Add clarifying comments to control_proto.c

09b5675

Refer to control-spec.txt grammar productions in comments in control_proto.c for clarity.

Manually fix some control replies

7f79a19

Manually fix up some reply-generating code that the Coccinelle scripts won't match. Some more complicated ones remain -- these are mostly ones that accumulate data to send, and then call connection_buf_add() or connection_write_str_to_buf() directly.

Coccinelle scripts for control.c refactor [ci skip]

4ac2ab3

Run Coccinelle for control.c refactor [ci skip]

db1dd68

Clean up formatting after Coccinelle

1a773fd

Clean up some minor formatting quirks after the Coccinelle run.

Merge branch 'ticket30007' into tmp20190415

350b51f

tlyu mentioned this pull request Apr 26, 2019

Ticket30007 v2 refactor control response formatting #984

Merged

tlyu closed this Apr 26, 2019

torproject / tor Public

Ticket30007 refactor control response formatting #956

Ticket30007 refactor control response formatting #956

tlyu commented Apr 15, 2019

tlyu commented Apr 15, 2019

coveralls commented Apr 15, 2019

tlyu commented Apr 26, 2019

torproject / tor Public

Ticket30007 refactor control response formatting #956

Ticket30007 refactor control response formatting #956

Conversation

tlyu commented Apr 15, 2019

tlyu commented Apr 15, 2019

coveralls commented Apr 15, 2019

Pull Request Test Coverage Report for Build 4742

💛 - Coveralls

tlyu commented Apr 26, 2019