Bug #43050 for bind9-public: [PATCH] pre-release DynDB nits

Tue, 16 Aug 2016 07:38:17 -0400 Petr Spacek <pspacek@redhat.com> - Ticket created

CC:	"Evan Hunt" <each@isc.org>
Subject:	pre-release DynDB nits
Date:	Tue, 16 Aug 2016 13:38:11 +0200
To:	bind-bugs@isc.org
From:	"Petr Spacek" <pspacek@redhat.com>

Hello Evan and others, attempt to port bind-dyndb-ldap to dyndb API shipped with BIND 9.11 uncovered couple of nits in current dyndb code. I believe that it is worth fixing them before the final release: 1) Docs in the example driver were outdated. A patch is attached. 2) Function dns_dyndb_register_t register_func is given the text between {} in dyndb section. Following snippet is causing problems: dyndb "name" "library.so" { custom_structure { blah 1; nah 2; }; }; When register_func is called, all inner curly braces are gone. This makes impossible to re-use map parsers from libraries. I peeked at the isc_tokentype_btext handling in isc_lex_gettoken() but I'm sure what is the right approach to fix this. Ideally, I would like to send the complete text into the plugin and let plugin to deal with it (possibly by calling ISC parser with custom grammar definition). It is possible to escape all the curly braces using \{ \} but that is quite ugly for any non-trivial structures. Would it be possible to convince the parser not to remove {} from the isc_tokentype_btext? -- Petr Spacek @ Red Hat

0001-update-comments-in-dyndb-example-driver.patch

Message body is not shown because sender requested not to inline it.

Tue, 16 Aug 2016 10:26:03 -0400 Petr Spacek <pspacek@redhat.com> - Correspondence added

CC:	"Evan Hunt" <each@isc.org>
Subject:	Re: [ISC-Bugs #43050] AutoReply: pre-release DynDB nits
Date:	Tue, 16 Aug 2016 16:25:56 +0200
To:	bind9-bugs@isc.org
From:	"Petr Spacek" <pspacek@redhat.com>

On 16.8.2016 13:38, BIND9 Bugs via RT wrote: > Hello Evan and others, > > attempt to port bind-dyndb-ldap to dyndb API shipped with BIND 9.11 uncovered > couple of nits in current dyndb code. > > I believe that it is worth fixing them before the final release: > > 1) Docs in the example driver were outdated. A patch is attached. > > 2) Function dns_dyndb_register_t register_func is given the text between {} in > dyndb section. > > Following snippet is causing problems: > dyndb "name" "library.so" { > custom_structure { > blah 1; > nah 2; > }; > }; > > When register_func is called, all inner curly braces are gone. > > This makes impossible to re-use map parsers from libraries. I peeked at the > isc_tokentype_btext handling in isc_lex_gettoken() but I'm sure what is the > right approach to fix this. > > Ideally, I would like to send the complete text into the plugin and let plugin > to deal with it (possibly by calling ISC parser with custom grammar definition). > > It is possible to escape all the curly braces using \{ \} but that is quite > ugly for any non-trivial structures. > > Would it be possible to convince the parser not to remove {} from the > isc_tokentype_btext? I just found third problem: 3) bind-dyndb-ldap is using header file errno2result.h from isc/unix, which is not installed by BIND's 'make install'. Would it be possible to add this header to the list of installed headers? I can see that the function in it has isc__ prefix. Would it be possible to make it public? Quick & dirty port of bind-dyndb-ldap to BIND 9.11 is available on: https://github.com/pspacek/bind-dyndb-ldap/tree/v11 It is not finished by any means. At least the configuration is awkward at the moment and I suspect that there are be some race conditions somewhere in event processing on plugin startup/reload/shutdown. I will look into these later this week. -- Petr Spacek @ Red Hat

Wed, 17 Aug 2016 00:13:32 -0400 Mark Andrews <marka@isc.org> - Correspondence added

On Tue Aug 16 01:38:17 2016, pspacek@redhat.com wrote: > 1) Docs in the example driver were outdated. A patch is attached. committed.

Wed, 17 Aug 2016 00:13:33 -0400 The RT System itself - Status changed from 'new' to 'open'

Wed, 17 Aug 2016 02:19:51 -0400 Evan Hunt <Evan_Hunt@isc.org> - Given to Mark Andrews <marka@isc.org>

Wed, 17 Aug 2016 03:44:02 -0400 Evan Hunt <Evan_Hunt@isc.org> - Queue changed from #6 to #7

Wed, 17 Aug 2016 04:56:03 -0400 Mark Andrews <marka@isc.org> - Given to Evan Hunt <Evan_Hunt@isc.org>

Wed, 17 Aug 2016 10:11:32 -0400 Petr Spacek <pspacek@redhat.com> - Correspondence added

Subject:	Re: [ISC-Bugs #43050] pre-release DynDB nits
Date:	Wed, 17 Aug 2016 16:10:11 +0200
To:	bind9-bugs@isc.org, "Evan Hunt" <each@isc.org>
From:	"Petr Spacek" <pspacek@redhat.com>

I just found another nit in the example driver: The string db_name passed to dyndb_init is freed when configuration parsing is finished. The driver has to copy the value instead of copying the pointer. Patch is attached. -- Petr Spacek @ Red Hat

0002-fix-dyndb-example-driver-to-correctly-copy-db_name.patch

Message body is not shown because sender requested not to inline it.

Wed, 17 Aug 2016 14:41:45 -0400 Evan Hunt <Evan_Hunt@isc.org> - Correspondence added

4445. [cleanup] isc_errno_toresult() can now be used to call the formerly private function isc__errno2result(). [RT #43050] 9.11.0, 9.9.10, 9.10.5 4444. [bug] Fixed some issues related to dyndb: A bug caused braces to be omitted when passing configuration text from named.conf to a dyndb driver, and there was a use-after-free in the sample dyndb driver. [RT #43050] 9.11.0 Petr, you should be able to pull this code from our public source repository (source.isc.org) as soon as it syncs. Or I can send you a patch if you prefer.

Wed, 17 Aug 2016 14:41:46 -0400 Evan Hunt <Evan_Hunt@isc.org> - Status changed from 'open' to 'resolved'

Thu, 18 Aug 2016 07:23:49 -0400 Petr Spacek <pspacek@redhat.com> - Correspondence added

CC:	"Evan Hunt" <each@isc.org>
Subject:	Re: [ISC-Bugs #43050] pre-release DynDB nits
Date:	Thu, 18 Aug 2016 13:23:43 +0200
To:	bind9-review@isc.org
From:	"Petr Spacek" <pspacek@redhat.com>

On 17.8.2016 20:41, Evan Hunt via RT wrote: > 4445. [cleanup] isc_errno_toresult() can now be used to call the > formerly private function isc__errno2result(). > [RT #43050] > > 9.11.0, 9.9.10, 9.10.5 > > 4444. [bug] Fixed some issues related to dyndb: A bug caused > braces to be omitted when passing configuration text > from named.conf to a dyndb driver, and there was a > use-after-free in the sample dyndb driver. [RT #43050] > > 9.11.0 > > Petr, you should be able to pull this code from our public source repository > (source.isc.org) as soon as it syncs. Or I can send you a patch if you prefer. I fetched 3390d74e33385337631b19e68760025e0ca5d6ec from Git and confirm that it fixes the problem in parser. The new header isc/errno.h works fine if I install it manually. It does not get installed using 'make install' - I think that this deserves one more patch. Thank you! -- Petr Spacek @ Red Hat

Thu, 18 Aug 2016 07:23:50 -0400 The RT System itself - Status changed from 'resolved' to 'open'

Thu, 18 Aug 2016 08:28:38 -0400 Mark Andrews <marka@isc.org> - Correspondence added

On Thu Aug 18 01:23:49 2016, pspacek@redhat.com wrote: > On 17.8.2016 20:41, Evan Hunt via RT wrote: > > 4445. [cleanup] isc_errno_toresult() can now be used to call > > the > > formerly private function isc__errno2result(). > > [RT #43050] > > > > 9.11.0, 9.9.10, 9.10.5 > > > > 4444. [bug] Fixed some issues related to dyndb: A bug > > caused > > braces to be omitted when passing configuration > > text > > from named.conf to a dyndb driver, and there > > was a > > use-after-free in the sample dyndb driver. [RT > > #43050] > > > > 9.11.0 > > > > Petr, you should be able to pull this code from our public source > > repository > > (source.isc.org) as soon as it syncs. Or I can send you a patch if > > you prefer. > > I fetched 3390d74e33385337631b19e68760025e0ca5d6ec from Git and > confirm that > it fixes the problem in parser. > > The new header isc/errno.h works fine if I install it manually. It > does not > get installed using 'make install' - I think that this deserves one > more patch. addressed > > Thank you!

Thu, 18 Aug 2016 08:28:40 -0400 Mark Andrews <marka@isc.org> - Status changed from 'open' to 'resolved'

Fri, 19 Aug 2016 10:23:13 -0400 Petr Spacek <pspacek@redhat.com> - Correspondence added

CC:	"Evan Hunt" <each@isc.org>
Subject:	Re: [ISC-Bugs #43050] pre-release DynDB nits
Date:	Fri, 19 Aug 2016 16:23:06 +0200
To:	bind9-review@isc.org
From:	"Petr Spacek" <pspacek@redhat.com>

On 18.8.2016 14:28, Mark Andrews via RT wrote: > On Thu Aug 18 01:23:49 2016, pspacek@redhat.com wrote: >> On 17.8.2016 20:41, Evan Hunt via RT wrote: >> I fetched 3390d74e33385337631b19e68760025e0ca5d6ec from Git and >> confirm that >> it fixes the problem in parser. >> >> The new header isc/errno.h works fine if I install it manually. It >> does not >> get installed using 'make install' - I think that this deserves one >> more patch. > > addressed I confirm it works for me. Now the plugin should be build-able with BIND 9.11rc1. Now I started to play with parser a little bit more and I wonder if there is a way to pass file + line information from "parent" config object to the plugin so the error reporting could give meaningful results. bin/named/server.c:configure_dyndb() is now calling cfg_obj_asstring() before the data are given to the plugin so all auxiliary information from cfg_obj_t are lost. Would it help to pass plain cfg_obj_t instead of string to plugins? (The plugin can call cfg_obj_asstring() if desired.) I'm not sure... Is there a way to take cfg_type_bracketed_text and feed it into parser again while supplying own cfg_type_t? I mean, is it possible to call the parser and tell it to re-interpret cfg_type_bracketed_text as some other cfg_type_t? That would improve user experience and general debugging as well because plugins would be able to print nice error messages using cfg_obj_log() etc. Wondering a bit more about this, there is an alternative: Plugin API could contain new symbol (provided by the plugin) which would contain cfg_type_t for plugin's top-level config. This symbol could be used instead of cfg_type_bracketed_text by the named's parser and resulting cfg_obj could be just passed to the plugin (e.g. attached to dctx). As a side-effect it would enable users to use "include" in plugin config (I guess). What do you think? What are viable alternatives? Have a nice weekend! -- Petr Spacek @ Red Hat

Fri, 19 Aug 2016 10:23:14 -0400 The RT System itself - Status changed from 'resolved' to 'open'

Fri, 19 Aug 2016 15:19:49 -0400 Evan Hunt <each@isc.org> - Correspondence added

CC:	bind9-review@isc.org
Subject:	Re: [ISC-Bugs #43050] pre-release DynDB nits
Date:	Fri, 19 Aug 2016 19:19:48 +0000
To:	"Petr Spacek" <pspacek@redhat.com>
From:	"Evan Hunt" <each@isc.org>

> I'm not sure... Is there a way to take cfg_type_bracketed_text and feed it > into parser again while supplying own cfg_type_t? I mean, is it possible to > call the parser and tell it to re-interpret cfg_type_bracketed_text as some > other cfg_type_t? The bracketed text isn't parsed as such in the first place; the lexer basically just reads it as a string token, except for counting internal braces so they're balanced. Perhaps we could associate the line number value with it, though, and pass that to the plugin, so that when the plugin parsed its input it would have an accurate line number to report when errors were encountered. (If the plugin is parsing its input using libisccfg, then we may need to update libisccfg so you can set an initial line number. I can't recall whether such a mechanism already exists.) > Wondering a bit more about this, there is an alternative: > Plugin API could contain new symbol (provided by the plugin) which would > contain cfg_type_t for plugin's top-level config. This symbol could be used > instead of cfg_type_bracketed_text by the named's parser and resulting cfg_obj > could be just passed to the plugin (e.g. attached to dctx). > > As a side-effect it would enable users to use "include" in plugin config (I > guess). > > What do you think? What are viable alternatives? Seems like overkil, to be honest.

Mon, 22 Aug 2016 19:07:44 -0400 Evan Hunt <Evan_Hunt@isc.org> - Correspondence added

> Perhaps we could associate the line number value with it, though, and > pass that to the plugin, so that when the plugin parsed its input it would > have an accurate line number to report when errors were encountered. Turns out this is already doable -- you can get the line number of the bracketed text object by using cfg_obj_line(obj). I think it's only necessary to add a 'lineno' parameter to dns_dyndb_load(). > (If the plugin is parsing its input using libisccfg, then we may need > to update libisccfg so you can set an initial line number. I can't recall > whether such a mechanism already exists.) This mechanism, however, does not yet exist. I'd implement it as a lexer function, isc_lex_setsourceline(), and call that on parser->lexer before calling cfg_parse_buffer().

Mon, 22 Aug 2016 19:07:46 -0400 Evan Hunt <Evan_Hunt@isc.org> - Status changed from 'open' to 'resolved'

Wed, 24 Aug 2016 06:42:15 -0400 Petr Spacek <pspacek@redhat.com> - Correspondence added

Subject:	Re: [ISC-Bugs #43050] pre-release DynDB nits
Date:	Wed, 24 Aug 2016 12:41:40 +0200
To:	bind9-review@isc.org
From:	"Petr Spacek" <pspacek@redhat.com>

On 23.8.2016 01:07, Evan Hunt via RT wrote: > >> Perhaps we could associate the line number value with it, though, and >> pass that to the plugin, so that when the plugin parsed its input it would >> have an accurate line number to report when errors were encountered. > > Turns out this is already doable -- you can get the line number of the > bracketed text object by using cfg_obj_line(obj). I think it's only necessary > to add a 'lineno' parameter to dns_dyndb_load(). > >> (If the plugin is parsing its input using libisccfg, then we may need >> to update libisccfg so you can set an initial line number. I can't recall >> whether such a mechanism already exists.) > > This mechanism, however, does not yet exist. > > I'd implement it as a lexer function, isc_lex_setsourceline(), and call > that on parser->lexer before calling cfg_parse_buffer(). It sounds good to me. Will it set name of the file as well? It gets handy when "include" is used all over named.conf. -- Petr Spacek @ Red Hat

Wed, 24 Aug 2016 06:42:16 -0400 The RT System itself - Status changed from 'resolved' to 'open'

Thu, 25 Aug 2016 01:07:34 -0400 Evan Hunt <Evan_Hunt@isc.org> - Untaken

Thu, 25 Aug 2016 21:04:02 -0400 Mark Andrews <marka@isc.org> - Given to Evan Hunt <Evan_Hunt@isc.org>

Thu, 25 Aug 2016 21:11:56 -0400 Evan Hunt <Evan_Hunt@isc.org> - Correspondence added

Hi Petr, I've added this feature in the 9.11 branch now; see commit 02fb764681. 4455. [cleanup] Allow dyndb modules to correctly log the filename and line number when processing configuration text from named.conf. [RT #43050] 9.11.0

Thu, 25 Aug 2016 21:11:57 -0400 Evan Hunt <Evan_Hunt@isc.org> - Status changed from 'open' to 'resolved'

Tue, 30 Aug 2016 11:30:46 -0400 Petr Spacek <pspacek@redhat.com> - Correspondence added

CC:	"Evan Hunt" <each@isc.org>
Subject:	Re: [ISC-Bugs #43050] pre-release DynDB nits
Date:	Tue, 30 Aug 2016 17:30:37 +0200
To:	bind9-review@isc.org
From:	"Petr Spacek" <pspacek@redhat.com>

On 26.8.2016 03:11, Evan Hunt via RT wrote: > Hi Petr, > > I've added this feature in the 9.11 branch now; see commit 02fb764681. > > 4455. [cleanup] Allow dyndb modules to correctly log the filename > and line number when processing configuration text > from named.conf. [RT #43050] > > 9.11.0 I've tried to write a minimal program which is using the parser and functions you provided to set file & name of the buffer in question. For some reason it does not work and I'm not sure if it is caused by mistake I made or what is going on. I'm attaching minimal program to reproduce the crash. Compile it using: $ gcc -ggdb -lisc -lisccfg -ldns test.c The source contains in-line comments to make the line where I see problems. How should I fix that? Thank you for your help! -- Petr Spacek @ Red Hat

test.c

Message body is not shown because sender requested not to inline it.

Tue, 30 Aug 2016 11:30:47 -0400 The RT System itself - Status changed from 'resolved' to 'open'

Tue, 30 Aug 2016 14:47:54 -0400 Evan Hunt <each@isc.org> - Correspondence added

CC:
Subject:	Re: [ISC-Bugs #43050] pre-release DynDB nits
Date:	Tue, 30 Aug 2016 18:47:54 +0000
To:	"Petr Spacek via RT" <bind9-review@isc.org>
From:	"Evan Hunt" <each@isc.org>

> For some reason it does not work and I'm not sure if it is caused by > mistake I made or what is going on. I'm attaching minimal program to > reproduce the crash. My fault, I left a piece out, darn it. I forgot that cfg_parse_buffer() calls isc_lex_openbuffer() too. We need to be able to have it skip that step, so you can open the buffer and set things up before you start parsing. I'm afraid this bug is going to persist into 9.11.0, but it can be fixed in 9.11.1, and I'll be able to give you a patch for it in the meantime. As a temporary measure you can also kluge around it at the driver level by yanking some code out of libisccfg and having a local buffer parser. See attached.

dyntest.c

Message body is not shown because sender requested not to inline it.

Wed, 31 Aug 2016 06:33:39 -0400 Petr Spacek <pspacek@redhat.com> - Correspondence added

Subject:	Re: [ISC-Bugs #43050] pre-release DynDB nits
Date:	Wed, 31 Aug 2016 12:33:33 +0200
To:	bind9-review@isc.org
From:	"Petr Spacek" <pspacek@redhat.com>

On 30.8.2016 20:47, Evan Hunt via RT wrote: >> For some reason it does not work and I'm not sure if it is caused by >> mistake I made or what is going on. I'm attaching minimal program to >> reproduce the crash. > > My fault, I left a piece out, darn it. I forgot that cfg_parse_buffer() > calls isc_lex_openbuffer() too. We need to be able to have it skip that > step, so you can open the buffer and set things up before you start > parsing. > > I'm afraid this bug is going to persist into 9.11.0, but it can be fixed in > 9.11.1, and I'll be able to give you a patch for it in the meantime. I'm fine with it as long as dyndb_init() does not change. (I do not see reason for such change, just saying.) > As a temporary measure you can also kluge around it at the driver level > by yanking some code out of libisccfg and having a local buffer parser. > See attached. Thank you! -- Petr Spacek @ Red Hat

Wed, 21 Sep 2016 12:16:28 -0400 Vicky Risk <vicky@isc.org> - Subject changed from 'pre-release DynDB nits' to '[PATCH] pre-release DynDB nits'

Wed, 21 Sep 2016 12:59:41 -0400 Evan Hunt <Evan_Hunt@isc.org> - Queue changed from #7 to #6

Thu, 22 Sep 2016 23:49:08 -0400 Evan Hunt <Evan_Hunt@isc.org> - Status changed from 'open' to 'resolved'

Mon, 26 Sep 2016 07:23:23 -0400 Stephen Morris <stephen@isc.org> - Priority P2 Normal added

Mon, 26 Sep 2016 07:23:23 -0400 Stephen Morris <stephen@isc.org> - Severity S2 Normal added

Mon, 26 Sep 2016 07:23:24 -0400 Stephen Morris <stephen@isc.org> - Area bug added

Mon, 26 Sep 2016 07:23:24 -0400 Stephen Morris <stephen@isc.org> - Component BIND Server added

Fri, 07 Jul 2017 19:30:21 -0400 Vicky Risk <vicky@isc.org> - Queue changed from #6 to bind9-public

Wed, 16 Aug 2017 07:54:18 -0400 Mark Andrews <marka@isc.org> - Version Fixed 9.11.0 added

Created:	Tue, 16 Aug 2016 07:38:17 -0400
Updated:	Wed, 16 Aug 2017 07:54:18 -0400
Closed:	Thu, 22 Sep 2016 23:49:08 -0400

Bug #43050 for bind9-public: [PATCH] pre-release DynDB nits

This bug tracker is no longer active.