Bug 9259 - rodc join fails complaining that it can't find a RWDC
rodc join fails complaining that it can't find a RWDC
Status: RESOLVED FIXED
Product: Samba 4.0
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB
4.0.0rc2
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-08 06:41 UTC by Matthieu Patou
Modified: 2012-10-15 10:01 UTC (History)
2 users (show)

See Also:


Attachments
Proposed patch to fix the problem (3.75 KB, patch)
2012-10-13 07:34 UTC, Matthieu Patou
kai: review-
mat: review? (jra)
metze: review+
obnox: review+
Details
Patch for v3-6-test (3.81 KB, patch)
2012-10-15 06:51 UTC, Stefan Metzmacher
obnox: review+
Details
Patch for v3-5-test/v3-4-test (3.84 KB, patch)
2012-10-15 06:52 UTC, Stefan Metzmacher
obnox: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matthieu Patou 2012-10-08 06:41:58 UTC
A bug in the DNS library can return under some circumstance a broken DNS record causing the whole answer to be ignored.
Comment 1 Matthieu Patou 2012-10-13 07:33:24 UTC
So the problem is due to label being not null terminated but our code doing the assumption that it is.
Comment 2 Matthieu Patou 2012-10-13 07:34:51 UTC
Created attachment 8060 [details]
Proposed patch to fix the problem

We force buffer to be always initialized with 0, in order to avoid garbage at the end of a label.
Comment 3 Kai Blin 2012-10-13 07:55:59 UTC
Comment on attachment 8060 [details]
Proposed patch to fix the problem


>It avoid bugs when one of the buffer is supposed to contain a string
>that is not null terminated (ie. label->label) and that we don't force
>the last byte to 0.

I think the correct fix is to force the last byte to 0. Unless there's some sort of padding after those invalid strings, we'll leak into other data in any case.
Comment 4 Matthieu Patou 2012-10-13 08:14:19 UTC
(In reply to comment #3)
> Comment on attachment 8060 [details]
> Proposed patch to fix the problem
> 
> 
> >It avoid bugs when one of the buffer is supposed to contain a string
> >that is not null terminated (ie. label->label) and that we don't force
> >the last byte to 0.
> 
> I think the correct fix is to force the last byte to 0. Unless there's some
> sort of padding after those invalid strings, we'll leak into other data in any
> case.

that was my initial solution but metze convinced me that should be more prudent with this code that not everybody understand and initializing buffer to 0 would avoid copying garbage.
I think this makes sense, if really needed I can also force the last byte to 0 to clearly indicate that we know that the string should be finished with a '\0'
Comment 5 Kai Blin 2012-10-13 08:19:13 UTC
All I'm saying is that you say the problem is that we're not terminating strings correctly, but you're not fixing that problem.
Comment 6 Matthieu Patou 2012-10-13 08:33:35 UTC
It is fixing
+	if (!(label->label = talloc_zero_array(label, char, len+1))) {
 		buf->error = ERROR_DNS_NO_MEMORY;
 		goto error;
 	}
Here is the buffer for the label, we allocate len + 1 bytes, len is obtained from the response (not by doing a strlen on label). so we are sure that we will copy at most len bytes from label and so the last byte will 0.
Comment 7 Michael Adam 2012-10-14 19:58:46 UTC
* It is good to use the talloc_zero* calls in all cases anyways.
* For the label case, this in particular fixes the non-zero-terminated
  string case as Matthieu explained.
* The other cases changed in this patch, don't seem to contain strings,
  so I don't see a problem with this patch.

So this fixes what is described in comment #1:
It ensures that the label string is always zero-terminated.
Comment 8 Michael Adam 2012-10-14 20:11:41 UTC
Comment on attachment 8060 [details]
Proposed patch to fix the problem

ACK from me, based on the previous comment. What do we do with this bug now?
Comment 9 Matthieu Patou 2012-10-14 21:11:47 UTC
Well this is maybe one of the most important bug because it can impact a lot of users.
I'm a bit enclined to force the pickup for rc3.

Karolin, what's your point of view
Comment 10 Stefan Metzmacher 2012-10-15 06:40:06 UTC
Comment on attachment 8060 [details]
Proposed patch to fix the problem

This fixes the bug
and given the complexity of this code it's better to always use talloc*zero*
instead of just fixing single byte.
Comment 11 Matthieu Patou 2012-10-15 06:44:39 UTC
Metze, should we pick it for rc3?
Comment 12 Stefan Metzmacher 2012-10-15 06:51:35 UTC
Created attachment 8073 [details]
Patch for v3-6-test
Comment 13 Stefan Metzmacher 2012-10-15 06:52:18 UTC
Created attachment 8074 [details]
Patch for v3-5-test/v3-4-test
Comment 14 Stefan Metzmacher 2012-10-15 06:52:48 UTC
Karolin, please pick this to v4-0-test.
Comment 15 Karolin Seeger 2012-10-15 09:56:36 UTC
Pushed to autobuild-v4-0-test.
Re-assigning to Michael to review the other patches.
Comment 16 Michael Adam 2012-10-15 09:59:14 UTC
assigning to Karolin for picking to 3.X versions.
Comment 17 Karolin Seeger 2012-10-15 10:01:58 UTC
Pushed to v3-6-test and v3-5-test.
Closing out bug report.

Thanks!