Opened 8 years ago

Closed 8 years ago

#1389 closed defect (fixed)

xfrout should allow a message with size of 65535

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20111206
Component: xfrout Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: AXFR-out
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 6.05 Internal?: no

Description

If I understand the code correctly, b10-xfrout split a response
into multiple messages if the size of the message would be >= 65535.
Actually 65535 should be allowed (which is the possible maximum).

While it's not a big deal in practice, it should better be fixed.

Subtickets

Change History (13)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 2

comment:2 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20111206

comment:3 Changed 8 years ago by jinmei

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

comment:4 follow-up: Changed 8 years ago by jinmei

trac1389 is ready for review.

On closer look, I found that the code has (at least potentially)
more substantial bugs: it didn't take into account the size of
the question section, and TSIG len could be counted twice in some
cases. Still, it's less likely to have caused real harm due to
name compression, but there was possibility to create some bogus
responses. (I believe) I've fixed all these problems as well as
the original issue of 'off-by-one' bug.

I also made a couple of unrelated small fixes/cleanups (the first and
last commits).

This is the proposed changelog entry:

333.	[bug]		jinmei
	b10-xfrout could potentially create an overflow response message
	(exceeding the 64KB max) or could create unnecessarily small
	messages.  The former was actually unlikely to happen due to the
	effect of name compression, and the latter was marginal and at least
	shouldn't cause an interoperability problem, but these were still
	potential problems and should be fixed.
	(Trac #1389, git TBD)

comment:5 Changed 8 years ago by jinmei

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

comment:6 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:7 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

On closer look, I found that the code has (at least potentially)
more substantial bugs: it didn't take into account the size of
the question section, and TSIG len could be counted twice in some
cases. Still, it's less likely to have caused real harm due to
name compression, but there was possibility to create some bogus
responses. (I believe) I've fixed all these problems as well as
the original issue of 'off-by-one' bug.

I also made a couple of unrelated small fixes/cleanups (the first and
last commits).

From a design point of view, I believe this is still suboptimal, due to the name compression. If we compress the names, more RRs could fit in. If we repeatedly tried to add an RR and render the message until it overflows, we would get the biggest one possible, but it doesn't look very optimal. Maybe if there was some kind of iterative message rendering. Anyway, that is out of the scope of this ticket. Should I create a new one or did we already talked about this?

The code looks sane, but the tests don't pass for me. But I'm not sure if it is related to this ticket:

======================================================================
FAIL: test_axfr_normal_session (__main__.TestXfroutSessionWithSQLite3)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/bin/xfrout/tests/xfrout_test.py", line 1061, in test_axfr_normal_session
    self.check_axfr_stream(response)
  File "/home/vorner/work/bind10/src/bin/xfrout/tests/xfrout_test.py", line 1055, in check_axfr_stream
    self.assertTrue(rrsets_equal(expected_rr, actual_rr))
AssertionError: False is not true

======================================================================
FAIL: test_ixfr_to_axfr (__main__.TestXfroutSessionWithSQLite3)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/bin/xfrout/tests/xfrout_test.py", line 1072, in test_ixfr_to_axfr
    self.check_axfr_stream(response)
  File "/home/vorner/work/bind10/src/bin/xfrout/tests/xfrout_test.py", line 1055, in check_axfr_stream
    self.assertTrue(rrsets_equal(expected_rr, actual_rr))
AssertionError: False is not true

----------------------------------------------------------------------
333.	[bug]		jinmei
	b10-xfrout could potentially create an overflow response message
	(exceeding the 64KB max) or could create unnecessarily small
	messages.  The former was actually unlikely to happen due to the
	effect of name compression, and the latter was marginal and at least
	shouldn't cause an interoperability problem, but these were still
	potential problems and should be fixed.
	(Trac #1389, git TBD)

Shouldn't it be „potential problems and were fixed“? Because this would look like as the problem is still there and we are asking someone to fix it for us.

Thanks

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

Replying to vorner:

Thanks for the review.

From a design point of view, I believe this is still suboptimal, due to the name compression. If we compress the names, more RRs could fit in. If we repeatedly tried to add an RR and render the message until it overflows, we would get the biggest one possible, but it doesn't look very optimal. Maybe if there was some kind of iterative message rendering. Anyway, that is out of the scope of this ticket. Should I create a new one or did we already talked about this?

We can talk about it. One quick note is that this is what BIND 9
currently does, so we basically follow existing practice. I know it's
suboptimal, but I'm not sure whether there's a reasonable way to make
it optimize or whether it's even worth solving in the first place.
Again, however, I have no problem with discussing this separately,
probably on the bind10-dev list. In any case let's leave it out of
scope for this ticket.

The code looks sane, but the tests don't pass for me. But I'm not sure if it is related to this ticket:

Ah, okay. I suspect it's a portability issue depending on the
implementation detail of the SQLite3 "select" statement. If my guess
is correct, commit 144549c should solve the problem.

333.	[bug]		jinmei
	b10-xfrout could potentially create an overflow response message
	(exceeding the 64KB max) or could create unnecessarily small
	messages.  The former was actually unlikely to happen due to the
	effect of name compression, and the latter was marginal and at least
	shouldn't cause an interoperability problem, but these were still
	potential problems and should be fixed.
	(Trac #1389, git TBD)

Shouldn't it be „potential problems and were fixed“? Because this would look like as the problem is still there and we are asking someone to fix it for us.

Okay, will change it on merge.

comment:9 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:10 in reply to: ↑ 8 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei
  • Total Hours changed from 0 to 1.25

Hello

Replying to jinmei:

We can talk about it. One quick note is that this is what BIND 9
currently does, so we basically follow existing practice. I know it's

Well, the fact that other implementation does that doesn't make it right. Anyway, moving to the ML.

The code looks sane, but the tests don't pass for me. But I'm not sure if it is related to this ticket:

Ah, okay. I suspect it's a portability issue depending on the
implementation detail of the SQLite3 "select" statement. If my guess
is correct, commit 144549c should solve the problem.

Yes, it fixes it. Please merge.

Thanks

comment:11 Changed 8 years ago by shane

  • Feature Depending on Ticket set to AXFR-out

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

Replying to vorner:

Hello

We can talk about it. One quick note is that this is what BIND 9
currently does, so we basically follow existing practice. I know it's

Well, the fact that other implementation does that doesn't make it right.

I didn't mean that but just tried to note what the current BIND 10
does may not necessarily be a result of mere laziness (I didn't write
that part of implementation of xfrout, so I actually don't know). I'd
also point out that other implementations often do something specific
as a result of practical compromise. It's not very wise not to try to
learn from that by just dogmatically saying it's suboptimal.

But, of course, in some other cases it's possible that the behavior of
existing implementation is really wrong or doesn't make sense today
while it might at the time it was written. That's perfectly valid to
behave differently with the understanding the rationale of others and
with considerations on what really makes sense now on top of it.

So, as we both agreed, the right next step is to discuss this at
bind10-dev.

Yes, it fixes it. Please merge.

Okay, thanks, merge done. Closing ticket.

comment:13 Changed 8 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 1.25 to 6.05
Note: See TracTickets for help on using tickets.