#28755 closed enhancement (implemented)

Implement a K/V parser library

Reported by: dgoulet Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points: .3
Parent ID: #25502 Points:
Reviewer: ahf, dgoulet Sponsor:

Description

The key/value format K=V is used extensively in the Tor code and seems many places uses their own way to parse that.

This ticket is for implementing a library to parse such K/V construction. There is a world where we might require V to be quoted or not so I'll let this challenge to the implementer(s).

This ticket comes from the work done in #28179 that implements messages from the PT to the Tor process.

Child Tickets

TicketTypeStatusOwnerSummary
#28808defectclosednickmFuzzer to check for round-trip encoding/decoding on various types.

Change History (10)

comment:1 Changed 12 months ago by nickm

Actual Points: .3
Status: assignedneeds_review

Branch ticket28755 has my first take on this. PR at https://github.com/torproject/tor/pull/568 . More testing and/or fuzzing might be a good idea here.

comment:2 Changed 12 months ago by dgoulet

Reviewer: ahf, dgoulet

comment:3 Changed 12 months ago by teor

We could test this branch by moving all our K=V parsing over to it, and running the unit and integration tests.

comment:4 Changed 11 months ago by ahf

Looks very good for our use case. A fuzz function might be a good idea to get in.

comment:5 Changed 11 months ago by nickm

Good call. I found a couple of bugs while fuzzing.

comment:6 Changed 11 months ago by nickm

I've made a new ticket, "ticket28755_v2", including a fuzzer. PR at https://github.com/torproject/tor/pull/577

The initial implementation is a bit different, because the fuzzer found a bunch of problems, most of them related to eat_whitespace() not doing what I expected, and a few of them related to ambiguity involving weird standalone values like "=foo" or "".

The fuzzing corpus is in a "strops" branch in my private fuzzing-corpora repo; I'll merge that when you like this.

comment:7 Changed 11 months ago by dgoulet

Status: needs_reviewneeds_revision

Provided a review. Majority of comments are about leftovers in the code. The more important thing is about a test case I tried which failed.

(Oh also... I don't think this is related but Travis fails on Stem test and NSS... ?)

Last edited 11 months ago by dgoulet (previous) (diff)

comment:8 Changed 11 months ago by nickm

Status: needs_revisionneeds_review

Okay, I think the branch is better now. The leftovers are gone, it passes the tests you gave, and still passes the fuzzer.

comment:9 in reply to:  8 Changed 11 months ago by dgoulet

Status: needs_reviewmerge_ready

Replying to nickm:

Okay, I think the branch is better now. The leftovers are gone, it passes the tests you gave, and still passes the fuzzer.

lgtm;

But before merging, can you do a fixup that adds the test cases I added on the ticket that caught AB=?

comment:10 Changed 11 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Added those in a0a3694fd16660911756eb1d38c9c0efc4a35334.

Squashed and merged to master!

Note: See TracTickets for help on using tickets.