Opened 10 years ago

Closed 10 years ago

#129 closed defect (fixed)

lib/cc: from string to map doesn't check the length limitation

Reported by: jinmei Owned by: jelte
Priority: medium Milestone:
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

Please confirm r1666 (see the commit log).

This fix is incomplete: since some of public methods of MapElement? (including a constructor) accept an arbitrary std::map object, we can still have a MapElement? with an invalid tag length. We could solve this by checking the length of all map elements passed, but I was not sure this was the right approach and didn't touch this part. For example, we might have to reconsider the interface so that we only allow abstract level operations (e.g. via MapElement? objects, not bare std::map).

Note that I've added an assert() check in MapElement::toWire(). So an evil application can make the library abort itself (data received via network can't do this).

I'm giving this ticket to Jelte, who I believe should be most familiar with this code.

I've also made some additional cleanups:

  • removed unused variables from from_stringstream_map()
  • merge encode_tag() to MapElement::toWire(). this change may be debatable, but I believe consolidating the code logic improves readability in this case since toWire() is not (yet) big and this is the only user of encode_tag().

Subtickets

Change History (3)

comment:1 follow-up: Changed 10 years ago by jelte

  • Status changed from new to accepted

r1666 Looks good.

I've added the check in create() for now, for consistency; r1668. Throws a TypeError? instead of a ParseError?, since that is closer. Still not entirely the right exception, but:

I think you are right and we should remove the direct std::map from the API (as well as std::vector in ListElement? btw). I also want to add 'empty' constructors (which turns out to be the most common case, at least in all my code), and i think this could be added nicely when I do that (ticket #124)

comment:2 in reply to: ↑ 1 Changed 10 years ago by jinmei

Replying to jelte:

r1666 Looks good.

I've added the check in create() for now, for consistency; r1668. Throws a TypeError? instead of a ParseError?, since that is closer.

Hmm, I thought the create() factories are intended to be no-throw functions. Is it okay to throw an exception?

Also, I'd introduce a constant variable with an appropriate name for the length limitation of 255. I thought about that in r1666, but at that time it was the only occurrence of this magic number, so I simply explained what this number is in a comment line. Now we have more than one, it makes more sense to have a named constant.

I think you are right and we should remove the direct std::map from the API (as well as std::vector in ListElement? btw). I also want to add 'empty' constructors (which turns out to be the most common case, at least in all my code), and i think this could be added nicely when I do that (ticket #124)

Yeah, I noticed that a constructor (and/or factory) with an empty element might actually be sufficient. In the case of map element, it would be safer if we limit the interface to this type of constructor and add/delete element interfaces (with validation).

comment:3 Changed 10 years ago by jelte

  • Resolution set to fixed
  • Status changed from accepted to closed

The remaining open issue in this ticket will be addressed by ticket #124 (or #172, whichever comes first)

Note: See TracTickets for help on using tickets.