Opened 6 years ago

Closed 6 years ago

#3286 closed defect (fixed)

Message::fromWire cannot be called more than once per instance

Reported by: tmark Owned by: muks
Priority: low Milestone: bind10-1.2-release-freeze
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Once an INBOUND message has been populated using the fromWire method, subsequent calls to isc::dns::Message::fromWire result in unpredictable behavior. Typically an exception is thrown but which one can vary from one run to the next and it is not clear as the root cause. If fromWire is not intended to be used more than once in the lifetime of a Message instance this should at least be documented. Even better would be a a safety check in fromWire to throw an exception if fromWire is called more than once.

This behavior is simple to reproduce. Simple instantiate a Message and call fromWire twice with a valid packet buffer. The first call will succeed, the second will fail.

Subtickets

Change History (10)

comment:1 Changed 6 years ago by muks

I'm not sure if we need to support more than one fromWire() call on a Message object anywhere.

The Message class API doc says:

    /// \brief (Re)build a \c Message object from wire-format data.

which implies that it should be possible to call it more than once. But reading the implementation, it seems that fromWire() does not initially clear/reinitialize some member variables at the start, so multiple calls may fail.

comment:2 Changed 6 years ago by muks

Stephen says Message should support calling fromWire() more than once.

comment:3 Changed 6 years ago by muks

  • Milestone changed from New Tasks to bind10-1.2-release-freeze
  • Status changed from new to reviewing

Up for review. I don't think it requires a ChangeLog entry as it is not a user visible change.

comment:4 Changed 6 years ago by kean

  • Owner changed from UnAssigned to kean

comment:5 Changed 6 years ago by kean

  • Owner changed from kean to muks

If you configure with --enable-experimental-resolver this change causes make check to fail:

[ RUN      ] RecursiveQueryTest.forwarderSend
../../../../../bind10/src/lib/resolve/tests/recursive_query_unittest.cc:676: Failure
Expected: m.fromWire(ibuf) doesn't throw an exception.
  Actual: it throws.
../../../../../bind10/src/lib/resolve/tests/recursive_query_unittest.cc:680: Failure
Value of: q2->getName()
  Actual: .
Expected: q.getName()
Which is: example.com.

Did you check with that option?

comment:6 follow-up: Changed 6 years ago by kean

It might be a good idea to get in the habit of always using that option when checking changes made to libdns.

Last edited 6 years ago by kean (previous) (diff)

comment:7 in reply to: ↑ 6 Changed 6 years ago by muks

Replying to kean:

It might be a good idea to get in the habit of always using that option when checking changes made to libdns.

I agree. We should build the resolver too during development. --enable-experimental-resolver is in the build script, but I missed running make check outside src/lib/dns/.

Last edited 6 years ago by muks (previous) (diff)

comment:8 Changed 6 years ago by muks

  • Owner changed from muks to kean

Back to review. The explicit parseHeader() call before fromWire() caused the InputBuffer to seek past position 0.

comment:9 Changed 6 years ago by kean

  • Owner changed from kean to muks

Looks fine now. Please merge and close.

comment:10 Changed 6 years ago by muks

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

Merged to master branch in commit 1f0e3dc6f4de89e71282eb500a5841ff85693764:

* 040a4ae [3286] Make fromWire() always start reading from position 0 in the passed buffer
* e19305e [3286] Clear Message before parsing fromWire()

Resolving as fixed. Thank you for the reviews Kean.

Note: See TracTickets for help on using tickets.