NetBSD Problem Report #56448

From www@netbsd.org  Fri Oct  8 19:47:31 2021
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id DFC431A921F
	for <gnats-bugs@gnats.NetBSD.org>; Fri,  8 Oct 2021 19:47:31 +0000 (UTC)
Message-Id: <20211008194730.3C5F21A9239@mollari.NetBSD.org>
Date: Fri,  8 Oct 2021 19:47:30 +0000 (UTC)
From: solduck@gmail.com
Reply-To: solduck@gmail.com
To: gnats-bugs@NetBSD.org
Subject: Inetd Refactoring and IP rate limiting improvement
X-Send-Pr-Version: www-1.0

>Number:         56448
>Category:       bin
>Synopsis:       Inetd Refactoring and IP rate limiting improvement
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Fri Oct 08 19:50:00 +0000 2021
>Last-Modified:  Tue Oct 12 19:10:01 +0000 2021
>Originator:     Solomon Ritzow
>Release:        NetBSD-current
>Organization:
>Environment:
>Description:
I have implemented the changes proposed for usr.sbin/inetd by Christos 
Zoulas at https://github.com/ritzow/src/pull/1#issuecomment-907763557. 
I have also implemented various minor improvements as well as larger 
changes to the IP rate limiting code (which was added to inetd about a 
month ago) to reduce memory usage.

Here's some other info about the changes my university senior project
group implemented which Christos Zoulas committed a month ago:
http://cvsweb.netbsd.org/bsdweb.cgi/src/usr.sbin/inetd/inetd.c?rev=1.127&content-type=text/x-cvsweb-markup&only_with_tag=MAIN

The changes I've made to NetBSD-current can be viewed at
https://github.com/ritzow/src/pull/2/files
and downloaded from the following URLs:
https://raw.githubusercontent.com/ritzow/src/inetd-changes-2/usr.sbin/inetd/Makefile
https://raw.githubusercontent.com/ritzow/src/inetd-changes-2/usr.sbin/inetd/inetd.8
https://raw.githubusercontent.com/ritzow/src/inetd-changes-2/usr.sbin/inetd/inetd.c
https://raw.githubusercontent.com/ritzow/src/inetd-changes-2/usr.sbin/inetd/inetd.h
https://raw.githubusercontent.com/ritzow/src/inetd-changes-2/usr.sbin/inetd/parse.c
https://raw.githubusercontent.com/ritzow/src/inetd-changes-2/usr.sbin/inetd/ratelimit.c

Comparing the current inetd.c with the new parse.c or the new inetd.c 
should produce a pretty clear side-by-side diff. Same with inetd.h. 
To compare the new ratelimit.c code, it would be better to copy the old 
rate limit code at the bottom of inetd.c (the first function starting 
with rl_) into a new file and compare with ratelimit.c.

Rate limiting code has been moved to ratelimit.c. I renamed 
clear_ip_list to rl_clear_ip_list and broke the code up into more 
functions. I have also made the per-IP rate limiting allocation more 
efficient. IP addresses are now stored in their network format instead 
of a string from getnameinfo (see inetd.h struct rl_ip_node). malloc 
calls use only the space needed by the structure by using offsetof on 
union members (I suppose this can be a bit dangerous if not done 
correctly...). Per-IP rate limiting still supports textual comparison 
using getnameinfo for address families other than AF_INET and AF_INET6, but I 
don't think there are any that are actually compatible or used by inetd (I 
haven't tested UNIX sockets with a remote bound to another file, but I did test 
using IPv6 with the textual format by commenting out the IPv6 specific 
code, and it works properly). Still potentially handy for the future. 
The IP node list (se_rl_ip_list) now uses the <sys/queue.h> SLIST macros 
instead of a custom list. I've broken rl_process up into helper functions 
for each type of rate limiting and created a separate function for 
address stringification, for use with printouts from the -d flag. I 
tried to reduce stack memory use by moving printing code involving
string buffers into separate functions. I haven't tested rl_ipv6_eq on
a 32-bit system.

The code for the positional syntax has also been moved to parse.c. 
Function try_biltin has been added to remove parse.c:parse_server's 
dependency on the biltin structure definition.

File inetd.h has been updated with the proper function prototypes, and 
the servtab structure has been update with the new IP node SLIST. I also
moved things around a bit. The way we (a peer and myself) 
formatted inetd.h previously was somewhat confusing. Function and global
variable prototypes are now organized by the source file they are 
defined in.

I also added a -f flag that I saw in another problem report 
(https://gnats.netbsd.org/12823) that I thought could be useful. It 
runs inetd in the foreground but without debug printouts or SO_DEBUG.
I'm not completely sure about the line "if (foreground) setsid()" that 
I changed from "if (debug) setsid()".
>How-To-Repeat:

>Fix:

>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56448 CVS commit: src/usr.sbin/inetd
Date: Tue, 12 Oct 2021 15:08:04 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Tue Oct 12 19:08:04 UTC 2021

 Modified Files:
 	src/usr.sbin/inetd: Makefile inetd.8 inetd.c inetd.h parse_v2.c
 Added Files:
 	src/usr.sbin/inetd: parse.c ratelimit.c

 Log Message:
 PR/56448: Solomon Ritzow: Various improvements.

 Rate limiting code has been moved to ratelimit.c. I renamed
 clear_ip_list to rl_clear_ip_list and broke the code up into more
 functions. I have also made the per-IP rate limiting allocation more
 efficient. IP addresses are now stored in their network format instead
 of a string from getnameinfo (see inetd.h struct rl_ip_node). malloc
 calls use only the space needed by the structure by using offsetof on
 union members (I suppose this can be a bit dangerous if not done
 correctly...). Per-IP rate limiting still supports textual comparison
 using getnameinfo for address families other than AF_INET and AF_INET6, but I
 don't think there are any that are actually compatible or used by inetd (I
 haven't tested UNIX sockets with a remote bound to another file, but I did test
 using IPv6 with the textual format by commenting out the IPv6 specific
 code, and it works properly). Still potentially handy for the future.
 The IP node list (se_rl_ip_list) now uses the <sys/queue.h> SLIST macros
 instead of a custom list. I've broken rl_process up into helper functions
 for each type of rate limiting and created a separate function for
 address stringification, for use with printouts from the -d flag. I
 tried to reduce stack memory use by moving printing code involving
 string buffers into separate functions. I haven't tested rl_ipv6_eq on
 a 32-bit system.

 The code for the positional syntax has also been moved to parse.c.
 Function try_biltin has been added to remove parse.c:parse_server's
 dependency on the biltin structure definition.

 File inetd.h has been updated with the proper function prototypes, and
 the servtab structure has been update with the new IP node SLIST. I also
 moved things around a bit. The way we (a peer and myself)
 formatted inetd.h previously was somewhat confusing. Function and global
 variable prototypes are now organized by the source file they are
 defined in.

 I also added a -f flag that I saw in another problem report
 (https://gnats.netbsd.org/12823) that I thought could be useful. It
 runs inetd in the foreground but without debug printouts or SO_DEBUG.
 I'm not completely sure about the line "if (foreground) setsid()" that
 I changed from "if (debug) setsid()".


 To generate a diff of this commit:
 cvs rdiff -u -r1.29 -r1.30 src/usr.sbin/inetd/Makefile
 cvs rdiff -u -r1.64 -r1.65 src/usr.sbin/inetd/inetd.8
 cvs rdiff -u -r1.136 -r1.137 src/usr.sbin/inetd/inetd.c
 cvs rdiff -u -r1.3 -r1.4 src/usr.sbin/inetd/inetd.h
 cvs rdiff -u -r0 -r1.1 src/usr.sbin/inetd/parse.c \
     src/usr.sbin/inetd/ratelimit.c
 cvs rdiff -u -r1.5 -r1.6 src/usr.sbin/inetd/parse_v2.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

Home	PR Database Search