Project

General

Profile

Actions

Feature #4324

open

FaceUri: per-scheme parsing

Added by Junxiao Shi over 6 years ago. Updated 3 months ago.

Status:
In Progress
Priority:
Normal
Category:
Network
Target version:
-
Start date:
Due date:
% Done:

40%

Estimated time:

Description

Currently, FaceUri parses a string with a set of regular expressions, but performs canonization with per-scheme logic. This causes FaceUri to incorrectly accept invalid input in several occasions.
This issue is to refactor FaceUri to have per-scheme parsing logic, and reject invalid input during parsing instead of during canonization.


Related issues 4 (3 open1 closed)

Related to ndn-cxx - Bug #4474: FaceUri throws "Malformed URI" for IPv6 UDP multicast addressClosedDavide Pesavento01/24/2018

Actions
Blocks ndn-cxx - Bug #3896: FaceUri::parse doesn't reject some invalid unix:// URIsNew

Actions
Blocks ndn-cxx - Bug #3897: FaceUri::canonize doesn't always validate the port numberNew

Actions
Blocks ndn-cxx - Bug #4100: FaceUri::parse doesn't reject IPv4 addresses with v6 scheme (and vice versa)New

Actions
Actions #1

Updated by Junxiao Shi over 6 years ago

  • Blocks Bug #3896: FaceUri::parse doesn't reject some invalid unix:// URIs added
Actions #2

Updated by Junxiao Shi over 6 years ago

  • Blocks Bug #3897: FaceUri::canonize doesn't always validate the port number added
Actions #3

Updated by Junxiao Shi over 6 years ago

  • Blocks Bug #4100: FaceUri::parse doesn't reject IPv4 addresses with v6 scheme (and vice versa) added
Actions #4

Updated by Davide Pesavento over 6 years ago

I've been working on a rewritten FaceUri parser in a local branch, as a low priority task. The actual parsing (splitting the input string into URI components) is shared across all schemes, and follows RFC 3986 as closely as possible. After the parsing there is a validation step, which is scheme-specific.

Actions #5

Updated by Davide Pesavento over 6 years ago

  • Status changed from New to In Progress
  • Assignee set to Davide Pesavento
  • % Done changed from 0 to 40
Actions #6

Updated by Junxiao Shi over 6 years ago

I reviewed https://gerrit.named-data.net/4281 patchset-2.
I like that the parsing steps is cleaner.
I dislike that "validate" functions and "canonize" classes are separate. They should be together. A "scheme handler" (formerly "canonize provider") handles one or more schemes, and does both validation and canonization for these schemes. Each has its own file such as src/net/face-uri-udp.*, and registers itself using a macro similar to NFD's strategy. Shared functions, such as IP address validation, can be placed in base classes.

Actions #7

Updated by Davide Pesavento over 6 years ago

Junxiao Shi wrote:

I dislike that "validate" functions and "canonize" classes are separate. They should be together.

I haven't touched canonization at all (yet).
Several checks performed by canonization are now redundant with the new parsing algorithm. I plan to remove this duplication. However, a larger refactoring such as the one you propose should be deferred to a separate commit, to make reviewing easier.

Each has its own file such as src/net/face-uri-udp.*, and registers itself using a macro [...]

I'm mildly against splitting into multiple files. The per-scheme logic is a relatively small amount of code.

Actions #8

Updated by Junxiao Shi over 6 years ago

a larger refactoring such as the one you propose should be deferred to a separate commit, to make reviewing easier.

In contrast, the end result of this refactoring is mostly new code, so I'd rather review them as new code.

I'm mildly against splitting into multiple files. The per-scheme logic is a relatively small amount of code.

CanonizeProvider classes, altogether, are too long. Small files are easier to navigate.

Actions #9

Updated by Junxiao Shi about 6 years ago

  • Related to Bug #4474: FaceUri throws "Malformed URI" for IPv6 UDP multicast address added
Actions #10

Updated by Davide Pesavento about 4 years ago

  • Target version changed from v0.7 to 0.8.0
Actions #11

Updated by Davide Pesavento almost 3 years ago

  • Target version changed from 0.8.0 to 0.9.0
Actions #12

Updated by Davide Pesavento 3 months ago

  • Target version deleted (0.9.0)
Actions

Also available in: Atom PDF