Opened 8 years ago

Closed 8 years ago

#1784 closed defect (fixed)

b10-resolver incorrectly uses SyncUDPServer

Reported by: jinmei Owned by: jinmei
Priority: very high Milestone: Sprint-20120403
Component: resolver Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 9.5 Internal?: no

Description

That's the reason for the lettuce failure.
http://git.bind10.isc.org/~tester/builder//BIND10-lettuce/20120315040000-MacOS/logs/lettuce.out

I think there are several lessons we should learn from this incident,
but anyway I'll fix the bug first. And, as it's a quite critical
regression I'll do it within this sprint.

Subtickets

Change History (10)

comment:1 Changed 8 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:2 Changed 8 years ago by jinmei

trac1784 is ready for review.

The main fix is essentially a few lines of changes to the main code,
but it also contains fix to a number of issues in the original
#1600 implementation:

  • the original version almost totally lacked necessary unit tests. the almost only tests were for an important case and are for the part of code that is not really used by the main implementation.
  • the original version had almost no documentation for the added interface.

If at least either of an these were attempted, we may have been able
to notice this problem before merge. And, as a related issue:

  • the original version updated the part of the code that is not necessary for the purpose of this task; in fact, it updated the code that is not really used in any bind10 application, specifically, some mostly deprecated methods of DNSService. (I'm not sure we should keep it, but that's a separate question), and, these were not tested at all.

This is a related issue of missing tests or documentation. If we
diligently try to write tests and documentation, it would give us
an opportunity of reconsidering whether the addition is really
necessary. If we allow the developer to add something without
thinking about it much, that often leads to bugs like this.

Anyway, I've fixed all these issues: fix the main problem, added
detailed tests and documentation, and removed the dead addition.
I've also updated the interface a bit; that's in some sense a matter
of preference, but I saw the original one was quite confusing. Also,
its default was bad - the default must be something most general,
while the default of the previous version is for optimization. That's
another background cause of this bug.

For testing I've introduced an inheritance into the DNSService class.
That made the changes larger, but I believe it's a cleaner solution.
I guess we'll need something like MockDNSSerivce for other purposes,
too. Also, thanks to this abstraction we can now get rid of the
ugly "test_mode" global variable from portconfig (so I did that too).

Finally, I renamed asiodns/tests/io_service_unittest.cc to
dns_service_unittest.cc because that's more appropriate in terms of
what's tested in that file. That makes the diff even bigger, but
the essentially the only change is the lines below the introduction of
the helper TestLookup? class.

I don't think we need a changelog for this.

comment:3 Changed 8 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:4 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:5 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

my compiler is apparently not as smart as yours, and it needs the non-const static vars in dns_service_unittest to be defined outside of the class declaration.

cppcheck noted that data[] wasn't initialized, and, although it doesn't really matter what's in it, to make cppcheck happy, i just filled it with 1's. And another thing it caught is that in getSocketFD, res was dereferenced before its NULL check, so i've moved the code there around a bit.

Oh, and I got a linker error when linking against lib/libresolver, needed an LIBADD for that.

I have taken the liberty to address these issues myself, please check in commit

So the addServer calls are the thing you were wondering we should keep? I'd say remove them. (they even use the old dlog() calls, but have other problems as well)

Should we warn/error on unknown options in addServerUDPFromFD? (currently they are ignored)

No further comments, for the rest, the changes look OK

comment:6 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by jinmei

Replying to jelte:

Thanks for the prompt review.

my compiler is apparently not as smart as yours, and it needs the non-const static vars in dns_service_unittest to be defined outside of the class declaration.

cppcheck noted that data[] wasn't initialized, and, although it doesn't really matter what's in it, to make cppcheck happy, i just filled it with 1's. And another thing it caught is that in getSocketFD, res was dereferenced before its NULL check, so i've moved the code there around a bit.

Oh, and I got a linker error when linking against lib/libresolver, needed an LIBADD for that.

I have taken the liberty to address these issues myself, please check in commit

Thanks for the checks and corrections. These seem good. I've added
comments about the null check after getaddrinfo() because, technically
(per the API contract) the check should be redundant.

So the addServer calls are the thing you were wondering we should

keep? I'd say remove them. (they even use the old dlog() calls, but
have other problems as well)

Yeah I think we should remove them, even if we have vague expectation
that we may want to reuse them in future (YAGNI applies here). I
think it should go to a separate ticket (and guess it would be a
perfect item for "hardening").

Should we warn/error on unknown options in addServerUDPFromFD? (currently they are ignored)

Hmm, probably. Although it's quite unlikely to be misused (because it's
enum and you need to use a cast to decieve the compiler), it could still
be possible that an application built with a new version of header
(and using a new option flag) uses an old version of library binary.

So I've added the check.

comment:7 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:8 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

Thanks for the checks and corrections. These seem good. I've added
comments about the null check after getaddrinfo() because, technically
(per the API contract) the check should be redundant.

Yeah, my documentation wasn't explicitly clear about that. BTW, i *think* that if we don't reuse the error int and put the use of res-> directly in the if error check then cppcheck will swallow it. But that would mean repeating the error handling twice, not worth it imo :)

So the addServer calls are the thing you were wondering we should

keep? I'd say remove them. (they even use the old dlog() calls, but
have other problems as well)

Yeah I think we should remove them, even if we have vague expectation
that we may want to reuse them in future (YAGNI applies here). I
think it should go to a separate ticket (and guess it would be a
perfect item for "hardening").

ack, that code is a good example of deterioration by lack of use :)

Should we warn/error on unknown options in addServerUDPFromFD? (currently they are ignored)

Hmm, probably. Although it's quite unlikely to be misused (because it's
enum and you need to use a cast to decieve the compiler), it could still
be possible that an application built with a new version of header
(and using a new option flag) uses an old version of library binary.

So I've added the check.

ok cool

all good, please merge :)

comment:9 in reply to: ↑ 8 Changed 8 years ago by jinmei

Replying to jelte:

So the addServer calls are the thing you were wondering we should

keep? I'd say remove them. (they even use the old dlog() calls, but
have other problems as well)

Yeah I think we should remove them, even if we have vague expectation
that we may want to reuse them in future (YAGNI applies here). I
think it should go to a separate ticket (and guess it would be a
perfect item for "hardening").

Okay, I'll open a ticket for this.

all good, please merge :)

Thanks, merged, closing this ticket.

comment:10 Changed 8 years ago by jinmei

  • Estimated Difficulty changed from 0 to 4
  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 9.5

(giving an estimation of 4 at my discretion)

Note: See TracTickets for help on using tickets.