Bug 10620 - Bind 9.10 requires DLZ_DLOPEN_VERSION 3
Summary: Bind 9.10 requires DLZ_DLOPEN_VERSION 3
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: DNS server (internal) (show other bugs)
Version: 4.1.7
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 10695
  Show dependency treegraph
 
Reported: 2014-05-19 19:56 UTC by DJ Lucas
Modified: 2014-11-25 19:25 UTC (History)
6 users (show)

See Also:


Attachments
DNS patches to add support for BIND 9.10.x (19.62 KB, patch)
2014-10-20 05:58 UTC, Amitay Isaacs
no flags Details
patches for samba-4.2 (19.92 KB, patch)
2014-10-28 04:04 UTC, Amitay Isaacs
no flags Details
patches for samba-4.1 (17.30 KB, patch)
2014-10-28 04:05 UTC, Amitay Isaacs
no flags Details
Fix for bind 9.10 - add trailing . (3.43 KB, patch)
2014-11-03 03:48 UTC, Amitay Isaacs
no flags Details
Fix for bind 9.10 - add trailing . (7.52 KB, patch)
2014-11-05 06:51 UTC, Amitay Isaacs
no flags Details
Fix for bind 9.10 - add trailing . (7.69 KB, patch)
2014-11-20 10:24 UTC, Amitay Isaacs
kukks: review+
Details
patches for samba-4.1 (25.45 KB, patch)
2014-11-21 07:12 UTC, Amitay Isaacs
kukks: review+
Details
patches for samba-4.2 (28.07 KB, patch)
2014-11-21 07:13 UTC, Amitay Isaacs
kukks: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description DJ Lucas 2014-05-19 19:56:18 UTC
Upgrade to BIND 9.10 results in error: dlz_dlopen: /usr/lib/samba/bind9/dlz_bind9_9.so: incorrect driver API version 2, requires 3
Comment 1 DVader 2014-09-10 14:23:39 UTC
Any news about this when it will resolved ?
Comment 2 Kai Blin 2014-10-13 11:08:30 UTC
Reassigning to Amitay
Comment 3 Amitay Isaacs 2014-10-20 05:58:56 UTC
Created attachment 10356 [details]
DNS patches to add support for BIND 9.10.x
Comment 4 Amitay Isaacs 2014-10-20 06:00:48 UTC
Can someone test these patches with BIND 9.10.x?

The patches are for samba master.
Comment 5 Amitay Isaacs 2014-10-28 04:04:58 UTC
Created attachment 10368 [details]
patches for samba-4.2
Comment 6 Amitay Isaacs 2014-10-28 04:05:31 UTC
Created attachment 10369 [details]
patches for samba-4.1
Comment 7 Guenter Kukkukk 2014-11-01 03:41:51 UTC
As i already discussed with Amitay on IRC:

at least the the bind dlz_open driver exported functions
   "putrr" and 
   "putnamedrr"
are now more picky regarding the terminating dot at the end of (domain) names.

ATM those functions don't take care of the trailing dot!

Now for example i always get e.g.
 "li4771-131.addlz.kukkukk.com.addlz.kukkukk.com."
Note the double addition of the domain name!
After some minutes this "appending" also makes it into the samba DB storage:

samba_dlz: added rdataset _msdcs.addlz.kukkukk.com '_msdcs.addlz.kukkukk.com.  3600    IN      SOA     li4771-131.addlz.kukkukk.com. hostmaster.addlz.kukkukk.com._msdcs.addlz.kukkukk.com._msdcs.addlz.kukkukk.com._msdcs.addlz.kukkukk.com._msdcs.addlz.kukkukk.com._msdcs.addlz.kukkukk.com._msdcs.addlz.kukkukk.com._msdcs.addlz.kukkukk.com. 10 900 600 86400 3600'

... until name length errors occur.

So the current samba DLZ driver for bind-9.10.x is unusable and needs some additions.

I already prepared a patch, which seemed to fix all new problems.

But then i noticed that both
   rndc dumpdb -all
   dig AXFR zonename
still show _some_ double appending of the zone names ....

So i'm still investigating. My debugging atm shows that the DLZ driver with
my patch is now returning all names correctly to bind.

So it could also be a bind problem (with the special upper DLZ driver).

I keep you informed about my findings.

Cheers,  Günter
Comment 8 Amitay Isaacs 2014-11-03 03:48:29 UTC
Created attachment 10395 [details]
Fix for bind 9.10 - add trailing .
Comment 9 Amitay Isaacs 2014-11-03 03:49:41 UTC
Hi Günter,

Can you test with the attached patch?  I haven't found any problems in my testing.

Amitay.
Comment 10 Guenter Kukkukk 2014-11-04 00:18:46 UTC
Hi Amitay,

i've tested your patch against
  bind-9.10.1
  bind-9.9.5   (to check for a regression)
and all is working as expected.

I would modify
-	if (str == NULL) {
+	if (str == NULL || *str == '\0') {
		return str;
	}

	len = strlen(str);
	if (str[len-1] != '.') {
		tmp = talloc_asprintf(mem_ctx, "%s.", str);

otherwise str[len-1] would access out-of-range memory in case
of an empty string.

It also worries me, that the return code of many talloc_*
functions is not checked for failure (as i did in my test patch).

I treat it as a bug, when the return code of a possibly failing
function is not checked...

Otherwise you can add my

Reviewed-by: Guenter Kukkukk <kukks@samba.org>

Cheers, Günter
Comment 11 Amitay Isaacs 2014-11-05 06:51:22 UTC
Created attachment 10408 [details]
Fix for bind 9.10 - add trailing .
Comment 12 Guenter Kukkukk 2014-11-05 23:43:35 UTC
Hi Amitay,

two notes:

1.) In my temp-patch i used:
 	 char *tmp;
+	 const char *str;

and then assigned the new fqdn-string to "str" instead of "tmp".

Your current version leads to warnings:
[2362/4226] Compiling source4/dns_server/dlz_bind9.c
../source4/dns_server/dlz_bind9.c: In function ‘b9_format’:
../source4/dns_server/dlz_bind9.c:176:7: warning: assignment discards ‘const’ qualifier from pointer target type [enabled by default]
   tmp = b9_format_fqdn(mem_ctx, rec->data.srv.nameTarget);
       ^
../source4/dns_server/dlz_bind9.c:189:7: warning: assignment discards ‘const’ qualifier from pointer target type [enabled by default]
   tmp = b9_format_fqdn(mem_ctx, rec->data.mx.nameTarget);
       ^
../source4/dns_server/dlz_bind9.c:228:7: warning: assignment discards ‘const’ qualifier from pointer target type [enabled by default]
   tmp = b9_format_fqdn(mem_ctx, rec->data.soa.rname);

So you should change that, too.
------------------

2.) Your patch contains trailing whitespace
See screencapture from emacs (the red blocks):
   http://picpaste.com/pics/emacs1-oEjdjD3L.1415229997.png

To *visually* notice those whitespace, with emacs i use:
;; set show trailing whitespace always on:
(setq-default show-trailing-whitespace t)

Also git can remove trailing whitespace:
git config --global apply.whitespace strip

Cheers, Günter
Comment 13 Amitay Isaacs 2014-11-20 10:24:37 UTC
Created attachment 10446 [details]
Fix for bind 9.10 - add trailing .
Comment 14 Amitay Isaacs 2014-11-20 10:29:21 UTC
Hi Günter,

I have updated the patch based on your suggestions.

Amitay.
Comment 15 eponymous 2014-11-20 12:57:43 UTC
Is the 4.1 patch likely to make it into the Samba 4.1.14 release scheduled for Dec 1st?
Comment 16 Guenter Kukkukk 2014-11-21 01:38:33 UTC
Hi Amitay,

now the patch looks ok.

Have done my usual tests and
  rndc dumpdb -all
  dig @server axfr zone
and also
make test TESTS=dlz_bind9
passes.

But you really should fix your editor to not produce trailing spaces.
See
http://picpaste.com/emacs2-3i40NQdI.png
the red blocks.

But will give my review answer.

Cheers, Günter
Comment 17 Guenter Kukkukk 2014-11-21 01:43:11 UTC
Hmm, it seems that kukks@samba.org does not work
to answer the review request. Cannot click that link.

Amitay,
remove that review request and use the following one:
linux@kukkukk.com

That one worked in the past.
Will ask Lars to have a look.

Cheers, Günter
Comment 18 Guenter Kukkukk 2014-11-21 01:46:13 UTC
sorry, forget the last post.  :-)

Cheers, Günter
Comment 19 Amitay Isaacs 2014-11-21 02:35:56 UTC
(In reply to Guenter Kukkukk from comment #16)

I do have trailing space highlighting.  However, most of those spaces have been there for a while.  I hope I have not added new ones. :-)
Comment 20 Amitay Isaacs 2014-11-21 07:12:46 UTC
Created attachment 10447 [details]
patches for samba-4.1
Comment 21 Amitay Isaacs 2014-11-21 07:13:19 UTC
Created attachment 10448 [details]
patches for samba-4.2
Comment 22 Amitay Isaacs 2014-11-21 14:08:51 UTC
Hi Karonlin,

Patches for 4.1 and 4.2 attached.

Amitay.
Comment 23 Karolin Seeger 2014-11-24 20:16:37 UTC
(In reply to Amitay Isaacs from comment #22)
Pushed to autobuild-v4-[1|2]-test.
Comment 24 Karolin Seeger 2014-11-25 19:25:12 UTC
(In reply to Karolin Seeger from comment #23)
Pushed to both branches.
Closing out bug report.

Thanks!