Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#658 closed defect (complete)

Check qid of responses

Reported by: jelte Owned by: jelte
Priority: medium Milestone: R-Team-Sprint-20110316
Component: resolver Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 5.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

ticket #583 sets a random qid on all outgoing queries, but it is not actually checked yet.

As part of this, I propose we also move the creation and rendering out of io_fetch, and pass it a buffer only. (or rather, abstract them out and up to lib/resolve somewhere).

That would also be a step towards having a demuxer. But it is not absolutely necessary (we can also just remember the qid and retry the receive if it doesn't match with io_fetch for now).

Subtickets

Change History (15)

comment:1 Changed 9 years ago by stephen

The update should also ensure that the source address of the UDP response packet is receives is the same as the address it sent the query to.

comment:2 Changed 9 years ago by shane

  • Milestone changed from R-Team-Task-Backlog to R-Team-Sprint-20110308

comment:3 Changed 9 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:4 Changed 9 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Ready for review, though there are a few TODO's, mostly related to the deferred decision where to actually put the check (if we do it in something like a demuxer we may habe put the data in a buffer already, and don't need the copied readUint16 for instace, and if we do need that copy, we should move the implementation to a util/compat lib).

  • added writeUint8At to buffer (needed for a test, but seemed useful in general)

does *not* check the address at this point, since we already do a receiveFrom, the only thing we could check would be that we would check whether the address we entered was equal to the address we entered, which seems a bit superfluous :)

did add 2 tests; one to see it does ignore bad qid responses, and one to see that once it has ignored it, it will in fact accept a second (correct) response.

comment:5 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:6 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

src/lib/dns/buffer_unittest.cc
Should add a EXPECT_THROW test to check that the exception is thrown if the position specified is invalid.

src/lib/asiolink/io_fetch.cc
The recently merged ticket #499 includes "asiolink_utilities", which provides a readUint16 function.

I think it would be more elegant to put the QID check in a separate method which does the check and prints the error message (instead of an explicit test inlie and a "break" to get out of the loop), i.e.

do {
   do {
      asyncReceive
   } while (!receiveComplete)
} while (!qidMatches)

We do need the address check. The endpoint passed to open() and asyncSend() is is the address to where the data is being sent. The endpoint returned from asyncReceive() is the address from which the response was received. (These may be different when UDP is used.) At present, IOFetch is incorrect in that it uses the same variable for both: it should use a separate one for the returned address.

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

Replying to stephen:

src/lib/dns/buffer_unittest.cc
Should add a EXPECT_THROW test to check that the exception is thrown if the position specified is invalid.

ack, done

src/lib/asiolink/io_fetch.cc
The recently merged ticket #499 includes "asiolink_utilities", which provides a readUint16 function.

ah, i'll use that one when i merge (but see below)

I think it would be more elegant to put the QID check in a separate method which does the check and prints the error message (instead of an explicit test inlie and a "break" to get out of the loop), i.e.

do {
   do {
      asyncReceive
   } while (!receiveComplete)
} while (!qidMatches)

ok. In fact, the address check could go in there as well, so i've made it responseOK()

We do need the address check. The endpoint passed to open() and asyncSend() is is the address to where the data is being sent. The endpoint returned from asyncReceive() is the address from which the response was received. (These may be different when UDP is used.) At present, IOFetch is incorrect in that it uses the same variable for both: it should use a separate one for the returned address.

right. Changed remote to remote_snd and added a remote_rcv (which is initially a copy of remote_snd)

For the merge i'll also change that shared_ptr for remote_rcv to a scoped_ptr. But I just quickly looked, and 499 changed exactly that much that nearly every change here ended up a conflict. So over lunch i will contemplate 'replaying' this branch manually on top of the current master (in a new branch). You may choose whether to review this current change, the merge result, or both :)

comment:8 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

Ok, 'redoing' it on a new branch was less work than merging master back (nearly every line was a conflict);

I've pushed a branch trac658_new that is the original branch on top of the big changes in master, and it addresses the review comments. Most of the actual new code changes are getting the (new/changed) RecurseQuery? tests to work (since the code now checks qid we have to get that back in our tcp answer data).

I also added an operator==() and operator!=() to IOAddress (+tests), and responseOK() checks that.

comment:9 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

src/lib/asiolink/io_fetch.h
IOFetchData constructor: remote_rcv should be initialized in the same way as remote_snd. Because both are shared pointers, by initializing via the copy constructor, both point to the same underlying IOEndpoint.

src/lib/asiolink/io_fetch.h
responseOK: remote_snd and remote_rcv are shared pointers, so they should be dereferenced before checking for equality: the check should be:

*data.remote_snd == *data.remote_rcv

operator(): Note: when you merge, there are changes around the call to open.

I also added an operator==() and operator!=() to IOAddress (+tests), and
responseOK() checks that.

The changes don't appear to be in the commit (and besides, remote_xxx are pointers to IOEndpoints, not IOAddresses).

comment:10 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

comment:11 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

OK, ignore my last comments, I've reviewed trac658_new now ;-)

src/lib/asiolink/tests/io_fetch_unittest.c
udpReceiveHandler: should extend Doxygen description of argument list with new arguments. (However, there appears to be only one case where they are used - in UdpSendReceive - and both are set to false. Are they needed?)

IOFetchTest::operator(): where the expected result is checked, two lines are commented out.

With the addition of the QID check, it does occur to me that a missing test is to check that the I/O times out if the remote TCP server does not send back enough data.

src/lib/resolve/tests/recursive_query_unittest_2.cc
checkReceivedPacket now returns the QID, although all calls ignore the returned value.

comment:12 in reply to: ↑ 11 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

Replying to stephen:

OK, ignore my last comments, I've reviewed trac658_new now ;-)

src/lib/asiolink/tests/io_fetch_unittest.c
udpReceiveHandler: should extend Doxygen description of argument list with new arguments. (However, there appears to be only one case where they are used - in UdpSendReceive - and both are set to false. Are they needed?)

oops, both the descriptions and the tests that used them did not make the transition :/

added them both. (in fact i moved the original udpsendreceivetest to be a that takes those two methods, and added two tests, one that checks if there is a timeout if the qid doesn't match, and one that checks if there is success if the first answer does not match but the second one does, this actually made me find a bug; fetchdata->received should be cleared before doing a (second) async_receive()

IOFetchTest::operator(): where the expected result is checked, two lines are commented out.

they should have been removed, done.

With the addition of the QID check, it does occur to me that a missing test is to check that the I/O times out if the remote TCP server does not send back enough data.

i can see if i can add a test for that, returning this ticket now anyway

src/lib/resolve/tests/recursive_query_unittest_2.cc
checkReceivedPacket now returns the QID, although all calls ignore the returned value.

oh right, that makes some of the code two lines shorter, done, thanks :)

comment:13 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

src/lib/asiolink/tests/io_fetch_unittest.cc[[BR]
The new code contains tabs.

In tcpSendData, where short-send is activated, it might be useful in the future if the debug message were updated to note that fact and record the actual amount sent.

---
I don't need to see it again, please merge after making the suggested changes.

comment:14 Changed 9 years ago by jelte

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

comment:15 Changed 9 years ago by jelte

  • Estimated Difficulty changed from 0.0 to 5
Note: See TracTickets for help on using tickets.