On 2024-11-22 00:55 +01, Kirill A. Korinsky <kirill@korins.ky> wrote:
> On Tue, 29 Oct 2024 22:51:30 +0100,
> Kirill A. Korinsky <kirill@korins.ky> wrote:
>> 
>> On Sat, 14 Sep 2024 20:18:14 +0200,
>> Florian Obser <florian@openbsd.org> wrote:
>> > 
>> > I had hopped I'd find the time to review this, but it's been two weeks
>> > and it didn't happen. It's getting too close to release so it won't make
>> > it for 7.6.
>> > I put it on my todo list to look at it mid October at the earliest. Feel
>> > free to prod me if it doesn't happen then.
>> >
>>
>> as asked here the friendly ping which includes the diff.
>>

your timing was not optimal end of October...

>
> I'd like to remind about this diff.
>
> Ok?
>
> Index: sbin/unwind/frontend.c
> ===================================================================
> RCS file: /home/cvs/src/sbin/unwind/frontend.c,v
> diff -u -p -r1.89 frontend.c
> --- sbin/unwind/frontend.c	21 Nov 2024 13:35:20 -0000	1.89
> +++ sbin/unwind/frontend.c	21 Nov 2024 22:05:20 -0000
> @@ -118,6 +118,8 @@ TAILQ_HEAD(, pending_query)	 pending_que
>  struct bl_node {
>  	RB_ENTRY(bl_node)	 entry;
>  	char			*domain;
> +	int			 len;
> +	int			 wildcard;
>  };
>  
>  __dead void		 frontend_shutdown(void);
> @@ -171,6 +173,19 @@ RB_GENERATE(bl_tree, bl_node, entry, bl_
>  struct dns64_prefix	*dns64_prefixes;
>  int			 dns64_prefix_count;
>  
> +static void

Please provide a prototype and remove static, most functions don't use
static in these daemons. They probably should, but they currently don't.
And then move the function to the end of the file, the boring stuff is
at the end...

> +reverse(char *begin, char *end)
> +{
> +	char	t;
> +
> +	while (begin < --end) {
> +		t = *begin;
> +		*begin = *end;
> +		*end = t;
> +		++begin;
> +	}
> +}
> +
>  void
>  frontend_sig_handler(int sig, short event, void *bula)
>  {
> @@ -741,7 +756,7 @@ handle_query(struct pending_query *pq)
>  {
>  	struct query_imsg	 query_imsg;
>  	struct bl_node		 find;
> -	int			 rcode;
> +	int			 rcode, matched;
>  	char			*str;
>  	char			 dname[LDNS_MAX_DOMAINLEN + 1];
>  	char			 qclass_buf[16];
> @@ -798,8 +813,13 @@ handle_query(struct pending_query *pq)
>  	log_debug("%s: %s %s %s ?", ip_port((struct sockaddr *)&pq->from),
>  	    dname, qclass_buf, qtype_buf);
>  
> +	find.len = strlen(dname);
> +	find.wildcard = 0;
> +	reverse(dname, dname + find.len);
>  	find.domain = dname;
> -	if (RB_FIND(bl_tree, &bl_head, &find) != NULL) {
> +	matched = (RB_FIND(bl_tree, &bl_head, &find) != NULL);
> +	reverse(dname, dname + find.len);
> +	if (matched) {

Please wrap all of this in if (RB_EMPTY(bl_tree)) so that you only pay
for the cost of strlen and 2x reverse if you are using the feature.

>  		if (frontend_conf->blocklist_log)
>  			log_info("blocking %s", dname);
>  		error_answer(pq, LDNS_RCODE_REFUSED);
> @@ -1549,14 +1569,20 @@ parse_blocklist(int fd)
>  			if (linelen >= 2 && line[linelen - 2] != '.')
>  				line[linelen - 1] = '.';
>  			else
> -				line[linelen - 1] = '\0';
> +				line[linelen-- - 1] = '\0';
>  		}
>  
> +		if (line[0] == '#')
> +		    continue;
> +
>  		bl_node = malloc(sizeof *bl_node);
>  		if (bl_node == NULL)
>  			fatal("%s: malloc", __func__);
>  		if ((bl_node->domain = strdup(line)) == NULL)
>  			fatal("%s: strdup", __func__);
> +		reverse(bl_node->domain, bl_node->domain + linelen);
> +		bl_node->len = linelen;
> +		bl_node->wildcard = line[0] == '.';
>  		if (RB_INSERT(bl_tree, &bl_head, bl_node) != NULL) {
>  			log_warnx("duplicate blocked domain \"%s\"", line);
>  			free(bl_node->domain);
> @@ -1571,7 +1597,12 @@ parse_blocklist(int fd)
>  
>  int
>  bl_cmp(struct bl_node *e1, struct bl_node *e2) {
> -	return (strcasecmp(e1->domain, e2->domain));
> +	if (e1->wildcard == e2->wildcard)
> +		return (strcasecmp(e1->domain, e2->domain));

this is a very weird case:
$ dig .example.com
dig: '.example.com' is not a legal name (empty label)

I wonder what asr is doing with this. But meh, I guess you still need to
handle it. So this is "fine".

> +	else if (e1->wildcard)
> +		return (strncasecmp(e1->domain, e2->domain, e1->len));
> +	else /* e2->wildcard */
> +		return (strncasecmp(e1->domain, e2->domain, e2->len));
>  }
>  
>  void
> Index: sbin/unwind/unwind.conf.5
> ===================================================================
> RCS file: /home/cvs/src/sbin/unwind/unwind.conf.5,v
> diff -u -p -r1.34 unwind.conf.5
> --- sbin/unwind/unwind.conf.5	30 Jun 2024 16:10:26 -0000	1.34
> +++ sbin/unwind/unwind.conf.5	6 Sep 2024 20:40:53 -0000
> @@ -69,9 +69,10 @@ If a domain from this list is queried,
>  .Nm unwind
>  answers with a return code of
>  .Dv REFUSED .
> -With
>  .Cm log
> -blocked queries are logged.
> +blocked queries are logged. The list supports limited wildcard

new sentence, new line.

> +syntax: domains starting with . (dot) are treated as any subdomains
> +on that zone.
>  .It Ic forwarder Brq Ar address Oo Ic port Ar number Oc Oo Oo Ic authentication name Ar name Oc Ic DoT Oc ...
>  A list of addresses of DNS name servers to forward queries to.
>  .Ic port
>
>
> -- 
> wbr, Kirill
>

no objections from me with all the things fixed.

-- 
In my defence, I have been left unsupervised.