Opened 10 years ago

Closed 10 years ago

#116 closed defect (complete)

review: NSEC RDATA "from wire" fixes and tests

Reported by: jinmei Owned by: each
Priority: high Milestone: 03. 1st Incremental Release
Component: libdns++ 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 review r1611. I've added more detailed tests for variuos kinds of malformed wire input and fixed bugs of the implementations that could make these thest fail.

Subtickets

Change History (5)

comment:1 Changed 10 years ago by jinmei

  • Priority changed from major to critical

upgrading it to "critical" as this bug is remotely exploitable.

comment:2 Changed 10 years ago by shane

  • Owner changed from shane to each
  • Status changed from new to assigned

comment:3 follow-up: Changed 10 years ago by shane

  • Owner changed from each to jinmei

I decided I would go ahead and have a look at this. I'm not sure if this code has already been merged or not, but I figured it couldn't hurt to review it. I'm certainly not the best person for this, but I'll have a look.

Typo here:

116 isc_throw(DNSMessageFORMERR, "NSEC RDATA from wire: "
117 "incomplete bit map filed");

Should be "field".

A question about the new tests. One is this:

81 Bitmap length is 0, which is invalid.
82 EXPECT_THROW(rdataFactoryFromFile(RRType::NSEC(), RRClass::IN(),
83 "testdata/rdata_nsec_fromWire6"),
84 DNSMessageFORMERR);

However in toText() we see this:

assert(len >= 0 && len <= 32);

If the length cannot be 0, then the assertion should be:

assert(len > 0 && len <= 32);

Further, do we consider it good practice to use assert()? Will this raise an exception or simply end the program?

Hopefully this is useful. :)

comment:4 in reply to: ↑ 3 Changed 10 years ago by jinmei

  • Owner changed from jinmei to each
  • Status changed from assigned to reviewing

Replying to shane:

I decided I would go ahead and have a look at this. I'm not sure if this code has already been merged or not, but I figured it couldn't hurt to review it. I'm certainly not the best person for this, but I'll have a look.

Thanks for the check, and, actually I think you are the best possible reviewer as the original code was written by you (although you might think you're not just for the same reason:-).

The code is already in trunk (it was done with confusion about how we use trunk/branches wrt review, etc), but hasn't been "reviewed".

Typo here:

[snip]

If the length cannot be 0, then the assertion should be:

Both are correct, thanks. Fixed in the trunk.

Further, do we consider it good practice to use assert()?

Good question...a non documented, probably not so consistent policy of mine is that I use assert() when an invariant isn't met or other internal error, while I generally use an exception for an unexpected/invalid input or run time system error (memory shortage, failure of opening a file, etc). In this specific, case, since the bitmap is private, is populated only via the constructors, and the constructors validate the source of bitmaps, the bitmaps must be valid once an NSEC object is successfully constructed. If this assumption fails it means there should be an internal bug in the class implementation or something very catastrophic should happen (such as memory corruption, the application overrides the memory region for the object with bogus data, etc).

We could still raise an exception even for such a case. For example, we could use a std::logic_error for internal bug.

That's certainly one possible approach, but I didn't do that for the following reasons:

  • even if we use an exception, I believe we cannot safely recover from such a serious error anyway, and the end result is the same: exit.
  • if we exit anyway, IMO it's better to let the program crash (hopefully with dumping a core) as soon as the error is detected. Rraising an exception may trigger a different exception, which will hide the real cause of the bug. Or worse, sometimes we are lazy and write catch-all catch clause just as an easy way of exception handling. Even though we should discourage such a style in any event, it should be prudent not to assume we always do the right thing, especially in a critical situation like this.

Admittedly, however, I'm probably not so consistent on when to use assert, and the policy wouldn't be clear.

If we can agree with how/when to use exceptions/assertions, we should probably document it as part of coding guideline.

Will this raise an exception or simply end the program?

No, if the assertion fails the program will just abort itself.

Hopefully this is useful. :)

It is. Thanks.

Do you have any other comment for this ticket, or can we close it?

comment:5 Changed 10 years ago by shane

  • Resolution set to complete
  • Status changed from reviewing to closed

No other comments for this ticket. (Handling exceptions/assertions issue on the mailing list.)

Note: See TracTickets for help on using tickets.