Re: Ntpd config file support
Mike Dean wrote: You mentioned earlier that code is never freed from memory. If you're this concerned with bloat, why don't you fix your problem of never deallocating the init code instead of worrying about some small feature like this? The Linux kernel does this; that's why you see the message about Freeing xxKB of memory late in the boot process. The freed memory is the memory that was used by the initialization code. This trick isn't limited to kernelspace. You can dynamically link libraries with init code, and you can unlink them at runtime as well. If you really want to reduce bloat, you can take a page from the kernel devs and free all that init code after the binary is up and running. If you really are concerned with bloat, that is. One important difference is that the kernel is loaded once and then runs for a long time, for different values of long. Busybox implements a large number of programs, so in most systems it is likely that at least one of those will be executed regularly. Therefor, those pages you call init will have to be loaded again and again, so there would be no advantage in memory usage. But you will have the disadvantages of the approach. It's more difficult to read and to maintain, as you have to write something like dlopen, dlsym, call init, dlclose instead of just init. For the same reason it also uses more memory, four function calls instead of one. The code in shared libraries tends to be larger because it has to be position independent. In addition, this would also increase the storage space required. The text and data segments are aligned to the page size. The shared library has it's own program header, relocation entries, names of the exported symbols. There are embedded devices running on 4MB flash that use busybox. Besides the work required for such an approach, the result would be actually worse. You would also either require a dynamic linker to be present on the target machine, and the target to support dynamic libraries and dlopen. Or you would have to maintain two versions of each call to an init function. What might be useful is labeling the functions and data structures as init_text and init_data, for the case where a device uses mostly long running processes. With help from the linker, these segments could be arranged together, and the kernel would just drop them when they are not used and memory is needed. It would not increase memory usage or storage requirements, and it doesn't have to be done all at once. And for targets that don't support it, it could just be left out, by defining the used macros as empty. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: Ntpd config file support
On Sat, Mar 22, 2014, at 1:57, Laurent Bercot wrote: On 21/03/2014 23:10, Cathey, Jim wrote: The only thing BB would need would be to isolate initialization into separate functions that would be grouped together by the linker. (And an associated link control file.) The usual demand-paged kernel will take care of the rest. Yes, that would definitely be the right approach. However, it conflicts with code organization, and thus, maintainability: currently, the code is sharded by functionality, which is sane and sound - but the linker would need the code to be sharded by type, init or non-init, which is exactly orthogonal to functionality. I'm not up to date with latest linkers, but unless you can annotate functions inside a single .c, it means that you now need to split every single functionality into at least two .c files. Slightly off tangent, but I remember reading a thesis on link time optimization for the 2.4 linux kernel. Among other things they managed to do was automatic recognition of init code, and moving such to its own section. It was quite interesting tech, no annotations needed; given the kernel already had some annotations, the tech merged the newly found init code to the annotated one. It could even detect parts that could not be annotated, because on some arch they would be called more than once - if your arch didn't, into the init section they went. So this could be done entirely in the linker, no busybox code needed. - Lauri -- http://www.fastmail.fm - A fast, anti-spam email service. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH] Ntpd config file support
Hello,
I got tired of hearing numbers thrown around without basis in a 4-5 day old
thread
when the subject was probably trivial, so I wrote a parser for ntp.conf
this morning.
This version falls back to /etc/ntp.conf when -p is not specified; if -p
is specified, ntp.conf is ignored. Only lines starting with server are
used.
Here (Debian Squeeze) the patch is +10 bytes without FEATURE_NTPD_CONF,
+250 bytes with it on. I could get rid of that +10 bytes easily; it's
just because I changed how it detects whether to error out.
HTH,
Isaac Dunham
From 3e0a8c764d58d1637fdcbaf24b6b4f77b2216402 Mon Sep 17 00:00:00 2001
From: Isaac Dunham ibid...@gmail.com
Date: Sat, 22 Mar 2014 11:44:30 -0700
Subject: [PATCH] ntpd: parse /etc/ntp.conf
This is a rudimentary parser for /etc/ntp.conf; it only looks for
lines like this:
server some.pool.ntp.org
server 0.0.0.0
All options are ignored.
+10 bytes when disabled, though I could make it +0.
Enabled, it's +250 bytes with gcc 4.4 and glibc 2.11.3:
function old new delta
ntp_init 406 523+117
add_peers - 107+107
.rodata 141382 141402 +20
packed_usage 29386 29392 +6
--
(add/remove: 1/0 grow/shrink: 3/0 up/down: 250/0) Total: 250 bytes
text data bss dec hex filename
759413 2059 9020 770492 bc1bc busybox_old
759657 2059 9020 770736 bc2b0 busybox_unstripped
---
networking/Config.src |8
networking/ntpd.c | 29 +++--
2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/networking/Config.src b/networking/Config.src
index ca0ddcd..fbad7ec 100644
--- a/networking/Config.src
+++ b/networking/Config.src
@@ -664,6 +664,14 @@ config FEATURE_NTPD_SERVER
Make ntpd usable as a NTP server. If you disable this option
ntpd will be usable only as a NTP client.
+config FEATURE_NTPD_CONF
+ bool Make ntpd understand /etc/ntp.conf
+ default y
+ depends on NTPD
+ help
+ Make ntpd look in /etc/ntp.conf for peers. Only server address
+ is supported.
+
config PSCAN
bool pscan
default y
diff --git a/networking/ntpd.c b/networking/ntpd.c
index 44592ce..bbecdd3 100644
--- a/networking/ntpd.c
+++ b/networking/ntpd.c
@@ -42,6 +42,9 @@
//usage: )
//usage: \n -S PROG Run PROG after stepping time, stratum change, and every 11 mins
//usage: \n -p PEER Obtain time from PEER (may be repeated)
+//usage: IF_FEATURE_NTPD_CONF(
+//usage: \nIf -p is not specified, look in /etc/ntp.conf
+//usage: )
#include libbb.h
#include math.h
@@ -2087,17 +2090,39 @@ static NOINLINE void ntp_init(char **argv)
d /* compat */
46aAbgL, /* compat, ignored */
peers, G.script_name, G.verbose);
- if (!(opts (OPT_p|OPT_l)))
- bb_show_usage();
+
// if (opts OPT_x) /* disable stepping, only slew is allowed */
// G.time_was_stepped = 1;
if (peers) {
while (peers)
add_peers(llist_pop(peers));
} else {
+#if ENABLE_FEATURE_NTPD_CONF
+ size_t bsiz = 0, i=0;
+ char *buf = 0;
+ FILE *stream = fopen(/etc/ntp.conf,r);
+
+ while (stream !errno) {
+ getline(buf, bsiz, stream);
+ if (!strncmp(buf, server, 6)) {
+char *cbuf = buf+6;
+while (cbuf[0]=='\t' || cbuf[0]==' ')
+ cbuf++;
+while (cbuf[i] 35) i++;
+cbuf[i] = 0;
+add_peers(xstrdup(cbuf));
+ }
+ }
+ if (buf) free(buf);
+ if (stream) fclose(stream);
+ errno = 0;
+ }
+ if ((opts OPT_l) !G.peer_cnt) {
+#endif
/* -l but no peers: stratum 1 server mode */
G.stratum = 1;
}
+ if (!(opts OPT_l) !G.peer_cnt) bb_show_usage();
if (!(opts OPT_n)) {
bb_daemonize_or_rexec(DAEMON_DEVNULL_STDIO, argv);
logmode = LOGMODE_NONE;
--
1.7.10.4
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] Ntpd config file support
Hi Isaac ! Your program will fail on lines starting with the word server (eg. serverxyz), that is it does not check for clear word boundary and gives wrong results in that case. while (cbuf[i] 35) i++; Unwise to do this in a not poor ASCII environment, as most systems are nowadays. This way you allow unprintable and any kind of illegal characters in time server addresses. ... and what about buffer overflow? Won't this loop then run to unknown locations? Beside this: Make it a default NO configuration, not being included in binaries build from standard options. -- Harald ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] Ntpd config file support
Harald Becker wrote: Your program will fail on lines starting with the word server (eg. serverxyz), that is it does not check for clear word boundary and gives wrong results in that case. The program will not fail for serverxyz, it will add a server xyz. This may be a bug or a feature :-) while (cbuf[i] 35) i++; Unwise to do this in a not poor ASCII environment, as most systems are nowadays. This way you allow unprintable and any kind of illegal characters in time server addresses. What is special about 35? Why is ' fine while is not? Of course the configuration comes from someone who already has root access, so whatever happens here to an invalid input can't be worse than rm -rf /. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] Ntpd config file support
On Sat, Mar 22, 2014 at 08:59:28PM +0100, Ralf Friedl wrote: Harald Becker wrote: Your program will fail on lines starting with the word server (eg. serverxyz), that is it does not check for clear word boundary and gives wrong results in that case. The program will not fail for serverxyz, it will add a server xyz. This may be a bug or a feature :-) while (cbuf[i] 35) i++; Unwise to do this in a not poor ASCII environment, as most systems are nowadays. This way you allow unprintable and any kind of illegal characters in time server addresses. What is special about 35? Why is ' fine while is not? Of course the configuration comes from someone who already has root access, so whatever happens here to an invalid input can't be worse than rm -rf /. Not necessarily. The ntp.conf file might be populated by some kind of autoconfig sysem (does DHCP have a way of offering suggested ntp servers?) which could in turn have malicious content injected. Whose responsibility it is to avoid this is an open question, but I think it's harmful to add a sloppy parser to busybox. If there's going to be a config parser it should attempt to be safe and correct. Rich ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] Ntpd config file support
On 22 March 2014 21:27:51 Rich Felker dal...@aerifal.cx wrote: On Sat, Mar 22, 2014 at 08:59:28PM +0100, Ralf Friedl wrote: Harald Becker wrote: Your program will fail on lines starting with the word server (eg. serverxyz), that is it does not check for clear word boundary and gives wrong results in that case. The program will not fail for serverxyz, it will add a server xyz. This may be a bug or a feature :-) while (cbuf[i] 35) i++; Unwise to do this in a not poor ASCII environment, as most systems are nowadays. This way you allow unprintable and any kind of illegal characters in time server addresses. What is special about 35? Why is ' fine while is not? Of course the configuration comes from someone who already has root access, so whatever happens here to an invalid input can't be worse than rm -rf /. Not necessarily. The ntp.conf file might be populated by some kind of autoconfig sysem (does DHCP have a way of offering suggested ntp servers?) which could in turn have malicious content injected. Whose responsibility it is to avoid this is an open question, but I think it's harmful to add a sloppy parser to busybox. If there's going to be a config parser it should attempt to be safe and correct. We already have one, see parse_config / config_parse (forgot which one).. Rich ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox Sent with AquaMail for Android http://www.aqua-mail.com ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v2] Ntpd config file support
On Sat, Mar 22, 2014 at 08:40:48PM +0100, Harald Becker wrote:
Hi Isaac !
Your program will fail on lines starting with the word server
(eg. serverxyz), that is it does not check for clear word
boundary and gives wrong results in that case.
...which are not legitimate entries in ntp.conf.
My aim is to parse a correct ntp.conf, and not cause security problems
on incorrect ones.
while (cbuf[i] 35) i++;
Unwise to do this in a not poor ASCII environment, as most
systems are nowadays. This way you allow unprintable and any
kind of illegal characters in time server addresses.
Fixing.
The fix expects chars exclusively in the set [-.:0-9a-zA-Z], which all
valid hostnames and IP addresses (ipv4/ipv6) have.
... and what
about buffer overflow? Won't this loop then run to unknown
locations?
Not possible. i is size_t, and getline() is _always_ \0 terminated.
However, the previous loop did have a potential buffer overrun if the
line ended prematurely:
server \n\0 would result in it walking over the end and writing 0 to the
first character less than 36 after a sequence of chars greater than
35 ('#')...
Beside this: Make it a default NO configuration, not being
included in binaries build from standard options.
OK.
(Denys gets the final say on that, though.)
Here's a version that has the issues mentioned fixed, and removes the 10
byte overhead.
It accepts peer as well as server, and runs 320 bytes.
Thanks,
Isaac Dunham
From 6c825a8e6c83715306f82540e540bac8d48af64f Mon Sep 17 00:00:00 2001
From: Isaac Dunham ibid...@gmail.com
Date: Sat, 22 Mar 2014 11:44:30 -0700
Subject: [PATCH] ntpd: parse /etc/ntp.conf
This is a rudimentary parser for /etc/ntp.conf; it only looks for
lines like this:
server some.pool.ntp.org
peer 0.0.0.0
All options are ignored.
+0 bytes when disabled.
Enabled, it's +320 bytes with gcc 4.4 and glibc 2.11.3:
function old new delta
ntp_init 406 593+187
add_peers - 107+107
.rodata 141382 141402 +20
packed_usage 29386 29392 +6
--
(add/remove: 1/0 grow/shrink: 3/0 up/down: 320/0) Total: 320 bytes
text data bss dec hex filename
759413 2059 9020 770492 bc1bc busybox_old
759727 2059 9020 770806 bc2f6 busybox_unstripped
---
networking/Config.src |8
networking/ntpd.c | 33 +
2 files changed, 41 insertions(+)
diff --git a/networking/Config.src b/networking/Config.src
index ca0ddcd..ec50134 100644
--- a/networking/Config.src
+++ b/networking/Config.src
@@ -664,6 +664,14 @@ config FEATURE_NTPD_SERVER
Make ntpd usable as a NTP server. If you disable this option
ntpd will be usable only as a NTP client.
+config FEATURE_NTPD_CONF
+ bool Make ntpd understand /etc/ntp.conf
+ default n
+ depends on NTPD
+ help
+ Make ntpd look in /etc/ntp.conf for peers. Only server address
+ and peer address are supported.
+
config PSCAN
bool pscan
default y
diff --git a/networking/ntpd.c b/networking/ntpd.c
index 44592ce..b65bb91 100644
--- a/networking/ntpd.c
+++ b/networking/ntpd.c
@@ -42,6 +42,9 @@
//usage: )
//usage: \n -S PROG Run PROG after stepping time, stratum change, and every 11 mins
//usage: \n -p PEER Obtain time from PEER (may be repeated)
+//usage: IF_FEATURE_NTPD_CONF(
+//usage: \nIf -p is not specified, look in /etc/ntp.conf
+//usage: )
#include libbb.h
#include math.h
@@ -2087,14 +2090,44 @@ static NOINLINE void ntp_init(char **argv)
d /* compat */
46aAbgL, /* compat, ignored */
peers, G.script_name, G.verbose);
+#if ENABLE_FEATURE_NTPD_CONF
+#else
if (!(opts (OPT_p|OPT_l)))
bb_show_usage();
+#endif
// if (opts OPT_x) /* disable stepping, only slew is allowed */
// G.time_was_stepped = 1;
if (peers) {
while (peers)
add_peers(llist_pop(peers));
} else {
+#if ENABLE_FEATURE_NTPD_CONF
+ size_t bsiz = 0, i=0;
+ char *buf = 0, *cbuf;
+ FILE *stream = fopen(/etc/ntp.conf,r);
+
+ while (stream !errno) {
+ getline(buf, bsiz, stream);
+ cbuf = buf;
+ if (!strncmp(buf, server, 6)) cbuf += 6;
+ if (!strncmp(buf, peer, 4)) cbuf += 4;
+ if (cbuf buf){
+while (cbuf[0] = ' ' cbuf[0]) cbuf++;
+while (cbuf[i]=='.' || cbuf[i]=='-' ||
+cbuf[i]==':' || isalnum(cbuf[i]))
+ i++;
+cbuf[i] = 0;
+add_peers(xstrdup(cbuf));
+ }
+ }
+ if (buf) free(buf);
+ if (stream) fclose(stream);
+ errno = 0;
+ }
+ if (!(opts OPT_l) !G.peer_cnt) {
+ bb_show_usage();
+ } else if (!G.peer_cnt) {
+#endif
/* -l but no peers: stratum 1 server mode */
G.stratum = 1;
}
--
1.7.10.4
___
busybox mailing list
busybox@busybox.net
[PATCH] add discard option -d to swapon
Attached are a series of three patches affecting swaponoff.
Patch #1 fixes two small logic errors: (a) the bb_strtou function was called
inside an instantiation of the MIN macro, which would cause the function to be
called twice in the common case; and (b) the actual maximum allowable swap
priority is SWAP_FLAG_PRIO_MASK SWAP_FLAG_PRIO_SHIFT, not simply
SWAP_FLAG_PRIO_MASK as the code previously assumed. It just so happens that
SWAP_FLAG_PRIO_SHIFT == 0, but one should not rely on that serendipity.
Patch #2 fixes the interaction of the -a and -p options to swapon. When -p is
given on the command line, the specified priority should be applied to any swap
areas given in /etc/fstab that lack a 'pri' option. This is the way swapon from
util-linux does it, and it's the rational behavior, or else -p and -a should be
mutually exclusive options.
Patch #3 adds a -d option to swapon, which sets SWAP_FLAG_DISCARD and
potentially SWAP_FLAG_DISCARD_ONCE or SWAP_FLAG_DISCARD_PAGES if a policy is
given. The patch also adds support for the 'discard' option in swap entries in
/etc/fstab.From 1509caf8e5088ed92ae6c7ab54d65785df570351 Mon Sep 17 00:00:00 2001
From: Matt Whitlock busy...@mattwhitlock.name
Date: Sat, 22 Mar 2014 18:54:24 -0400
Subject: [PATCH 1/3] avoid calling bb_strtou twice in MIN macro expansion
Also, the maximum allowable value of swap priority is technically SWAP_FLAG_PRIO_MASK SWAP_FLAG_PRIO_SHIFT.
---
util-linux/swaponoff.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/util-linux/swaponoff.c b/util-linux/swaponoff.c
index 3f22334..d35e30e 100644
--- a/util-linux/swaponoff.c
+++ b/util-linux/swaponoff.c
@@ -100,12 +100,12 @@ static int do_em_all(void)
g_flags = 0; /* each swap space might have different flags */
p = hasmntopt(m, pri);
if (p) {
- /* Max allowed 32767 (==SWAP_FLAG_PRIO_MASK) */
- unsigned int swap_prio = MIN(bb_strtou(p + 4 , NULL, 10), SWAP_FLAG_PRIO_MASK);
+ /* Max allowed 32767 (== SWAP_FLAG_PRIO_MASK SWAP_FLAG_PRIO_SHIFT) */
+ unsigned prio = bb_strtou(p + 4, NULL, 10);
/* We want to allow ,foo, thus errno == EINVAL is allowed too */
if (errno != ERANGE) {
g_flags = SWAP_FLAG_PREFER |
- (swap_prio SWAP_FLAG_PRIO_SHIFT);
+ MIN(prio, SWAP_FLAG_PRIO_MASK SWAP_FLAG_PRIO_SHIFT) SWAP_FLAG_PRIO_SHIFT;
}
}
#endif
@@ -124,6 +124,9 @@ int swap_on_off_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
int swap_on_off_main(int argc UNUSED_PARAM, char **argv)
{
int ret;
+#if ENABLE_FEATURE_SWAPON_PRI
+ unsigned prio;
+#endif
INIT_G();
@@ -132,11 +135,11 @@ int swap_on_off_main(int argc UNUSED_PARAM, char **argv)
#else
if (applet_name[5] == 'n')
opt_complementary = p+;
- ret = getopt32(argv, (applet_name[5] == 'n') ? ap: : a, g_flags);
+ ret = getopt32(argv, (applet_name[5] == 'n') ? ap: : a, prio);
if (ret 2) { // -p
g_flags = SWAP_FLAG_PREFER |
- ((g_flags SWAP_FLAG_PRIO_MASK) SWAP_FLAG_PRIO_SHIFT);
+ MIN(prio, SWAP_FLAG_PRIO_MASK SWAP_FLAG_PRIO_SHIFT) SWAP_FLAG_PRIO_SHIFT;
ret = 1;
}
#endif
--
1.9.1
From e7d0d02ca14aea0e12c2ca6b84e124db25172209 Mon Sep 17 00:00:00 2001
From: Matt Whitlock busy...@mattwhitlock.name
Date: Sat, 22 Mar 2014 19:10:08 -0400
Subject: [PATCH 2/3] fix interaction of -a and -p options in swapon
Swap entries in /etc/fstab inherit the priority specified on the command line unless they have 'pri' in their mount options.
---
util-linux/swaponoff.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/util-linux/swaponoff.c b/util-linux/swaponoff.c
index d35e30e..df0c524 100644
--- a/util-linux/swaponoff.c
+++ b/util-linux/swaponoff.c
@@ -82,6 +82,9 @@ static int do_em_all(void)
struct mntent *m;
FILE *f;
int err;
+#ifdef G
+ int cl_flags = g_flags;
+#endif
f = setmntent(/etc/fstab, r);
if (f == NULL)
@@ -97,14 +100,14 @@ static int do_em_all(void)
) {
#if ENABLE_FEATURE_SWAPON_PRI
char *p;
-g_flags = 0; /* each swap space might have different flags */
+g_flags = cl_flags; /* each swap space might have different flags */
p = hasmntopt(m, pri);
if (p) {
/* Max allowed 32767 (== SWAP_FLAG_PRIO_MASK SWAP_FLAG_PRIO_SHIFT) */
unsigned prio = bb_strtou(p + 4, NULL, 10);
/* We want to allow ,foo, thus errno == EINVAL is allowed too */
if (errno != ERANGE) {
- g_flags = SWAP_FLAG_PREFER |
+ g_flags = (g_flags ~SWAP_FLAG_PRIO_MASK) | SWAP_FLAG_PREFER |
MIN(prio, SWAP_FLAG_PRIO_MASK SWAP_FLAG_PRIO_SHIFT) SWAP_FLAG_PRIO_SHIFT;
}
}
--
1.9.1
From b2b2e65829785c959881d643bcff8843a6cddf7e Mon Sep 17 00:00:00 2001
From: Matt Whitlock busy...@mattwhitlock.name
Date: Sat, 22 Mar 2014 19:21:01 -0400
Subject: [PATCH 3/3] add discard option -d to swapon
---
util-linux/Config.src | 9 ++
util-linux/swaponoff.c | 85 --
2 files changed, 85
