kill builtin in ash might behave like killall
Hello, I'm using BB 1.26.2 with an heavily modified LEDE where I added procps-ng and sysvinit. Given that setup, I'm using /usr/bin/kill from props-ng and killall5 from sysvinit, meaning that I have the following configuration: # CONFIG_FEATURE_KILL_REMOVED is not set CONFIG_FEATURE_KILL_DELAY=0 # CONFIG_RFKILL is not set # CONFIG_KILL is not set CONFIG_KILLALL=y # CONFIG_KILLALL5 is not set # CONFIG_PKILL is not set The code in the kill.c applet is quite clear in this case: ... #elif !ENABLE_KILL && ENABLE_KILLALL && !ENABLE_KILLALL5 # define killall 1 # define killall5 0 ... My problem is that I'm also using ash with job control enabled, meaning that while I don't have the kill applet installed, I am still using the kill.c code through the kill builtin. You probably begin to see where I'm going: in this specific case, the kill builtin does not work correctly, because it believes it should behave as the killall command. And, of course, loads of scripts are now unable to work correctly. The obvious solution would be to disable CONFIG_KILLALL yet I also use it from time to time on the command line, so I'd prefer to keep it. For now, I'm doing a local patch to always check char3. This is probably suboptimal yet I cannot see a better solution for this specific issue. Does any of you have a better idea? Best regards, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: kill builtin in ash might behave like killall
Hello, On Fri, Mar 3, 2017 at 1:44 PM, Tito wrote: > > > On 03/03/2017 01:01 PM, Emmanuel Deloget wrote: >> >> Hello, >> >> I'm using BB 1.26.2 with an heavily modified LEDE where I added >> procps-ng and sysvinit. Given that setup, I'm using /usr/bin/kill from >> props-ng and killall5 from sysvinit, meaning that I have the following >> configuration: >> >> # CONFIG_FEATURE_KILL_REMOVED is not set >> CONFIG_FEATURE_KILL_DELAY=0 >> # CONFIG_RFKILL is not set >> # CONFIG_KILL is not set >> CONFIG_KILLALL=y >> # CONFIG_KILLALL5 is not set >> # CONFIG_PKILL is not set >> >> The code in the kill.c applet is quite clear in this case: >> >> ... >> #elif !ENABLE_KILL && ENABLE_KILLALL && !ENABLE_KILLALL5 >> # define killall 1 >> # define killall5 0 >> ... >> >> My problem is that I'm also using ash with job control enabled, >> meaning that while I don't have the kill applet installed, I am still >> using the kill.c code through the kill builtin. >> >> You probably begin to see where I'm going: in this specific case, the >> kill builtin does not work correctly, because it believes it should >> behave as the killall command. And, of course, loads of scripts are >> now unable to work correctly. >> >> The obvious solution would be to disable CONFIG_KILLALL yet I also use >> it from time to time on the command line, so I'd prefer to keep it. >> >> For now, I'm doing a local patch to always check char3. This is >> probably suboptimal yet I cannot see a better solution for this >> specific issue. Does any of you have a better idea? >> >> Best regards, >> >> -- Emmanuel Deloget > > > Hi, > you could disable CONFIG_KILLALL in your main busybox binary > and create a second binary that contains only the killall > applet and rename it killall, if space is not a problem. > > Ciao, > Tito Thanks Tito, but unfortunatly that would mean to change the way LEDE packages are built (which is way harder than applying my small, miserable patch on top of BB source). Space is not much an issue, yet having a full BB binary for killall is a bit, well, overkill. It would be far simpler to add psmisc as a LEDE package. But the best solution would still be to fix the kill applet, since the configuration I use is a valid configuration (and the kill builtin should never behave as killall, whatever the configuration is). Best regards, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: kill builtin in ash might behave like killall
Hi, On Fri, Mar 3, 2017 at 3:17 PM, Kang-Che Sung wrote: > On 03/03/2017 01:01 PM, Emmanuel Deloget wrote: >> >> Hello, >> >> I'm using BB 1.26.2 with an heavily modified LEDE where I added >> procps-ng and sysvinit. Given that setup, I'm using /usr/bin/kill from >> props-ng and killall5 from sysvinit, meaning that I have the following >> configuration: >> >> # CONFIG_FEATURE_KILL_REMOVED is not set >> CONFIG_FEATURE_KILL_DELAY=0 >> # CONFIG_RFKILL is not set >> # CONFIG_KILL is not set >> CONFIG_KILLALL=y >> # CONFIG_KILLALL5 is not set >> # CONFIG_PKILL is not set >> >> The code in the kill.c applet is quite clear in this case: >> >> ... >> #elif !ENABLE_KILL && ENABLE_KILLALL && !ENABLE_KILLALL5 >> # define killall 1 >> # define killall5 0 >> ... >> >> My problem is that I'm also using ash with job control enabled, >> meaning that while I don't have the kill applet installed, I am still >> using the kill.c code through the kill builtin. >> >> You probably begin to see where I'm going: in this specific case, the >> kill builtin does not work correctly, because it believes it should >> behave as the killall command. And, of course, loads of scripts are >> now unable to work correctly. >> >> The obvious solution would be to disable CONFIG_KILLALL yet I also use >> it from time to time on the command line, so I'd prefer to keep it. >> >> For now, I'm doing a local patch to always check char3. This is >> probably suboptimal yet I cannot see a better solution for this >> specific issue. Does any of you have a better idea? >> >> Best regards, >> >> -- Emmanuel Deloget > > I think I have proposed a patch that fixes this and it's on master now. > > Here, if you can adapt it and apply it for yourself: > https://git.busybox.net/busybox/commit/?id=61a91af63dbc91f85058efda5c542dfc859ab1be I missed it. And yeah, it does exactly what I want to do. Thanks for your help! -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] add lsscsi implementation
Hi Markus, On Fri, Apr 7, 2017 at 9:52 PM, Markus Gothe wrote: > > Ahhh, for some reason it disappeared… > > > > On 07 Apr 2017, at 21:01 , Tito wrote: > > > Hi, > > you forgot to attach the patch? > > > > Ciao, > > Tito > > > > On 04/07/2017 07:12 PM, Markus Gothe wrote: > >> Please see attached patch. This is a minimal lsscsi which is useful for > >> getting information about USB-disks and other devices populating the > >> SCSI bus in Linux. > It seems the config: comments are incorrectly refering to lsusb instead of lsscsi. > > >> BR, > >> //Markus - The panama-hat hacker > >> Best regards, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 1/1] makedevs: set path size to match linux
Hello, On Mon, Jun 26, 2017 at 11:23 PM, Matthew Weber < matthew.we...@rockwellcollins.com> wrote: > Baruch, > > On Mon, Jun 26, 2017 at 3:55 PM, Baruch Siach wrote: > > Hi Jared, > > > > On Mon, Jun 26, 2017 at 03:33:09PM -0500, Matt Weber wrote: > >> From: Jared Bents > >> > >> Update to increase the pathname limit to the > >> linux limit of 4096 characters. > >> > >> Similar patch: > >> https://patchwork.openembedded.org/patch/131475/ > >> > >> Signed-off-by: Jared Bents > >> Signed-off-by: Matt Weber > >> --- > >> miscutils/makedevs.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/miscutils/makedevs.c b/miscutils/makedevs.c > >> index 9e7ca34..0049edb 100644 > >> --- a/miscutils/makedevs.c > >> +++ b/miscutils/makedevs.c > >> @@ -208,7 +208,7 @@ int makedevs_main(int argc UNUSED_PARAM, char > **argv) > >> unsigned count = 0; > >> unsigned increment = 0; > >> unsigned start = 0; > >> - char name[41]; > >> + char name[4096]; > > > > Why not use PATH_MAX here? > > Agree, that would be cleaner. Will submit v2 after some testing. > > That still leaves a hardcoded value in the sscanf of 4095 > should I add a comment to the affect we're assuming PATH_MAX is at > least 4096? Maybe a check is also needed? > Alternatively you may use the m modifier, when implemented, to auto-allocate the name pointer. This way you don't have to hardcode anything in the sscanf(), you let the library for the job for you. Later, you can check the string length. Such a change would induce an allocation, a free but will also reduce stack usage. > > Matt > > Best regards, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] Silence misguided GCC warning about alignment issues
Hello, On Mon, Aug 7, 2017 at 12:25 PM, Lauri Kasanen wrote: > On Mon, 7 Aug 2017 12:09:45 +0200 (CEST) > Johannes Schindelin wrote: >> Meaning: GCC gets this all wrong and should not complain. So let's force >> it to be quiet for a couple of lines. > > I believe this is both wrong and dangerous. If gcc warns about it, it > will also very likely try to mis-optimize the same case. > > - Lauri I tried to carefully read the code, and I don't see where the aliasing issue migh be. * buf is an uin8_t [] * the code either use it as is, or promote it to const uint8_t *, or cast it to (const) char *. All of these types are compatible w.r.t. aliasing (and the C99 standard). I may have missed something though and my eyes might be rusted. Now, it seems that some might have seen a regression in GCC with respect to aliasing rules [1][2]. This might be an instance of the bug (according to the bug report, it's no longer reproducible in GCC trunk). Anyway, it might be a good idea to avoid unsetting the warning for one buggy GCC version :) Best regards, -- Emmanuel Deloget [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80593 [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80633 ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] Silence misguided GCC warning about alignment issues
On Mon, Aug 7, 2017 at 9:46 PM, Johannes Schindelin < johannes.schinde...@gmx.de> wrote: > Hi Denys, > > On Mon, 7 Aug 2017, Denys Vlasenko wrote: > > > On Mon, Aug 7, 2017 at 12:09 PM, Johannes Schindelin > > wrote: > > > When compiling xz_dec_stream.c with GCC 7.1.0, it complains thusly: > > > > > > In function 'dec_stream_footer': > > > error: dereferencing type-punned pointer will break > strict-aliasing > > > rules [-Werror=strict-aliasing] > > >if (xz_crc32(s->temp.buf + 4, 6, 0) != > get_le32(s->temp.buf)) > > >^~ > > > > > > The thing is, the `buf` field was put just after two fields of type > > > size_t for the express purpose of avoiding alignment issues, as per the > > > comment above the `temp` struct. > > > > > > Meaning: GCC gets this all wrong and should not complain. So let's > force > > > it to be quiet for a couple of lines. > > > > xz_crc32 is: > > > > xz_crc32(const uint8_t *buf, size_t size, uint32_t crc) > > > > I think gcc is not complaining about xz_crc32, > > it complains about get_le32! > > > > Can you test this theory by splitting that statement in two? > > I, too, thought at first that it is clearly about get_le32(), but GCC was > really adamant that it was the xz_crc32() call. > > And a little further investigation suggests that GCC really meant > xz_crc32(), which is defined as a static function in > archival/libarchive/decompress_unxz.c that gets inlined by GCC because it > simply hands off to crc32_block_endian0() which in turn takes an > `uint32_t *` as first parameter. And that's where the type-punning is > broken. > > This would be quite weird - the static function is by definition local to the compilation unit, and the compiler cannot know about the content of decompress_unxz.c from xz_dec_stream.c. BR, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] Silence misguided GCC warning about alignment issues
On Mon, Aug 7, 2017 at 10:37 PM, Emmanuel Deloget wrote: > > > > On Mon, Aug 7, 2017 at 9:46 PM, Johannes Schindelin > wrote: >> >> Hi Denys, >> >> On Mon, 7 Aug 2017, Denys Vlasenko wrote: >> >> > On Mon, Aug 7, 2017 at 12:09 PM, Johannes Schindelin >> > wrote: >> > > When compiling xz_dec_stream.c with GCC 7.1.0, it complains thusly: >> > > >> > > In function 'dec_stream_footer': >> > > error: dereferencing type-punned pointer will break >> > > strict-aliasing >> > > rules [-Werror=strict-aliasing] >> > >if (xz_crc32(s->temp.buf + 4, 6, 0) != get_le32(s->temp.buf)) >> > >^~ >> > > >> > > The thing is, the `buf` field was put just after two fields of type >> > > size_t for the express purpose of avoiding alignment issues, as per the >> > > comment above the `temp` struct. >> > > >> > > Meaning: GCC gets this all wrong and should not complain. So let's force >> > > it to be quiet for a couple of lines. >> > >> > xz_crc32 is: >> > >> > xz_crc32(const uint8_t *buf, size_t size, uint32_t crc) >> > >> > I think gcc is not complaining about xz_crc32, >> > it complains about get_le32! >> > >> > Can you test this theory by splitting that statement in two? >> >> I, too, thought at first that it is clearly about get_le32(), but GCC was >> really adamant that it was the xz_crc32() call. >> >> And a little further investigation suggests that GCC really meant >> xz_crc32(), which is defined as a static function in >> archival/libarchive/decompress_unxz.c that gets inlined by GCC because it >> simply hands off to crc32_block_endian0() which in turn takes an >> `uint32_t *` as first parameter. And that's where the type-punning is >> broken. >> > > This would be quite weird - the static function is by definition local to the > compilation unit, and the compiler cannot know about the content of > decompress_unxz.c from xz_dec_stream.c. My bad. It's the other way round -- sorry for the noise. decompress_unxz.c includes xz_dec_stream.c (sometimes, the Busybox code is a bit hard to follow :)) And yes, its seems that the get_le32() macro in xz_private.h is a bit illegal with respect to strict aliasing, as it casts a uint8_t * into a const uint32_t *. > > BR, > > -- Emmanuel Deloget > ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] Silence misguided GCC warning about alignment issues
Hi Johannes On Tue, Aug 8, 2017 at 1:19 PM, Johannes Schindelin < johannes.schinde...@gmx.de> wrote: > Hi Emmanuel, > > On Mon, 7 Aug 2017, Emmanuel Deloget wrote: > > > And yes, its seems that the get_le32() macro in xz_private.h is a bit > > illegal with respect to strict aliasing, as it casts a uint8_t * into a > > const uint32_t *. > > It would seem so. > > Until you realize that it is used only on s->temp.buf (or `s->temp.buf + 4 > * `), and that the `buf` field was carefully placed after two > fields of type `size_t` (i.e. naturally aligned *at least* to a 32-bit > boundary). > > If you are worried about that magic, you can even mark the `buf` field > using ALIGN4. But that *still* does not fix GCC's compiler warning. What > fixes it is my workaround of defining a static ALWAYS_INLINE function. > > No, I'm not worried by the magic :) Yet the problem is not the aliasing per see, it's the way the compiler handles it and what it means to him. Strict aliasing has been defined as a way to allow the compiler to do some clever code optimization (mostly reordering). If the compiler knows that two memory areas are distinct, there is no R/W dependency on these areas and it can reorder the code as he wants. But when aliasing comes into play, such optimizations should be forbidden since an undetected R/W dependency might exists. Yet, since the -fstrict-aliasing is given to him, we explicitely tell the compiler to force strict aliasing rules and the compiler still apply them -- ditching any unproved R/W dependency in the process. Needless to say, this can be the source of hard-to-catch bugs :) Consider this code: uint32_t k = 0; uint8_t *p = (uint8_t *)&k; k = 0x; if (*p == 0xff) do_some_magic_stuff(); Without strict aliasing, the compiler must order the code so that *p is indeed 0xff when the if condition is evaluated because it cannot be sure that there is a R/W dependency between k and p. With forced strict aliasing, &k and p are assumed to not point to the same memory area, thus there cannot be any R/W dependency, so changing k is not supposed to have any effect on the *p test. As a consequence, the compiler is free to reorder the code and for exemple do the following: if (*p = 0xff) do_some_magic_stuff(); k = 0x; Which, of course, will not work as expected. Thus, this aliasing problem may have other unintended consequences. Silencing the warning will hide a possible bug, and this is not a good thing. It would be far better to fix the aliasing issue correctly. Similarly, the fact that ALWAY_INLINE seems to hide the warning looks more like a GCC bug to me than the expected behavior, as ALWAYS_INLINE does not change the way the compiler plays with the aliasing rules. Something along the line of (not tested): #define get_le32(p) \ ({ \ union { const uint32_t *x32; const uint8_t *x8; } v = { .x8 = (p), }; \ le32_to_cpup(v.x32); \ }) (for the occurence at xz_private.h@24) Maybe it's even better to create an aliastype macro looking like: #define aliasvalue(type, value) \ ({ \ union { \ type *aliased; \ typeof(value) *original; \ } v = { \ .original = &(value), \ }; \ v.aliased; \ }) And then: #define get_le32(p) le32_to_cpup(aliasvalue(uint32_t, (p))) Or #define get_le32(p) (*(aliasvalue(uint32_t,(p I'm not even sure the compiler is not able to fully optimize out this kind of cod since it will look like a move from one register to another register to him (my own tests seems to show that the compiler emits optimized code in this situation). The aliasvalue() macro above is probably not perfect (it might run ok when p in a scalar or an array of scalar but will break if p is a pointer; not to mention its horrible name...) so more though might be needed to come up with a correct version. > Denys' fix works around the problem, alright, but it also makes the code > slower. And it is *not* necessary to make the code slower. > > Ciao, > Johannes > Best regards, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: bbox: musl versus uclibc
On Mon, Aug 14, 2017 at 7:43 PM, Denys Vlasenko wrote: > Only a few options did not build: > EXTRA_COMPAT and FEATURE_VI_REGEX_SEARCH > failed because they need GNU regexp extensions. I have a patch somewhere that enable parts of VI_REGEX_SEARCH (it only does forward regex search while the GNU counterpart implements both forward and backward regex search). I can submit that if the community is interested. BR, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: Re: [PATCH] Fix runsvdir so it reaps children and avoid zombi processes when killed.
Hello, On Wed, Jan 3, 2018 at 11:04 PM, Markus Gothe wrote: > Can you run strace on a process invoked from the inittab? I surely have > issues with doing that. > This is definitely possible, but thou shall not forget -ff to follow forks (and prepare for an awfull lot of output, so maybe you should redirect that in some log file). > > As far as killing the processes it doesnt matter which signal you use; > processes will become zombified forever without the patch. > I, too, don't get how runsvdir children can be zombified if runsvdir is killed. When runsvdir dies, the children are reparented to /bin/init which is, AFAIK, reaping them. I won't disagree with you (you say you have zombies all over the place) but maybe the problem - and the right fix - lies somewhere else. Best regards, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: Re: [PATCH] Fix runsvdir so it reaps children and avoid zombi processes when killed.
Hello, On Thu, Jan 4, 2018 at 9:55 PM, Markus Gothe wrote: > Well, just to be sure we all talk about the same children that needs to be > reaped are those of runsvdir, the bug manifests without using the inittab, > surely init will reap the dead process of runsvdir but not the children > themselves of runsvdir. > Any process of pid 1 is expected to be the ultimate child reaper, so once runsvdir dies its children are bound to be reaped by init. That's how unix has worked for decades :) BR, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [**EXTERNAL**] Re: [PATCH] Fix runsvdir so it reaps children and avoid zombi processes when killed.
Hello, On Thu, Jan 4, 2018 at 6:53 PM, Cathey, Jim wrote: > >Unconditionally sleeping when signals may arrive is a no-no. > > It used to be that sleep(3) was interruptible. Is this no longer the > case? Who broke libc, and when? > * sleep() in musl is implemented using nanosleep() [1]. * ucLibc-ng is using nanosleep() too [2], and implement a workaround for an old linux bug (it seems that this workaround is not needed) * and surpris, sleep() in the glibc is using... nanosleep() [3] Now, sleep() shall be interrupted by any signal who either has a handler in the thread or terminates the program [A]. The sames goes for nanosleep() [B]. So when you install a SIGCHLD handler, the signal will kill sleep(). Now, SIGCHLD is not catched by default (man 7 signal says the default is set to "ignore"). So while sleep() is still interruptible, it won't be if the delivered signal is SIGCHLD - unless you install a handler for this signal. Since busybox' init does not do that, then no, sleep() will not be interrupted in that case (and you face the problem Laurent describes). > > -- Jim > > Best regards, -- Emmanuel Deloget [1] https://git.musl-libc.org/cgit/musl/tree/src/unistd/sleep.c [2] https://cgit.openadk.org/cgi/cgit/uclibc-ng.git/tree/libc/unistd/sleep.c#n74 [3] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/posix/sleep.c [A] http://pubs.opengroup.org/onlinepubs/9699919799/functions/sleep.html [B] http://pubs.opengroup.org/onlinepubs/9699919799/functions/nanosleep.html ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] ash: make bash_source command search current directory after PATH
Hello, On Mon, Jan 29, 2018 at 4:42 PM, Paul Otto wrote: > > > >> > Making bash "source" behavior non-standard had nothing useful in it. >> >> "source" is already non-standard and not specified in POSIX. so simply by >> using it, your script is not POSIX compliant. > > > That is why, incidentally, I wrote my proposed contribution the way I did > initially. In my view, while BASH treats "source" and "." the same, POSIX > doesn't allow for "source" so why not have "source" hold to the BASH > standard and "." hold to the POSIX standard? I definitely caved too quickly > on that point, and wound up with my contribution being swallowed up into a > patch that did the exact opposite of my intent. ¯\_(ツ)_/¯ > > This is probably due to the fact that many (for instance, I) expect . and source to behave the same way :) Having them behaving differently would somewhat violate the principle of least surprise :) BR, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: printf can't handle the '+' char in front of an integer
Hello, Just my two cents here. On Wed, Oct 31, 2018 at 12:34 PM Bernhard Reutner-Fischer wrote: > > > > > 64-bit double's mantissa is only 53 bits... > > > > With "long double" shenanigans, I managed to "improve" this to: > > > > $ ./busybox printf "%f\n" 18446744073709550592 18446744073709553665 > > 18446744073709550591.50 > > 18446744073709553663.50 > > > > at this cost: > > > > function old new delta > > printf_main 909 976 +67 > > strtold- 19 +19 > > print_direc 457 475 +18 > > conv_strtod 54 61 +7 > > -- > > (add/remove: 3/0 grow/shrink: 3/0 up/down: 130/0) Total: 111 > > bytes > > > > See attached. Do you think it's worth it? > > I don't know. > long double can be very slow (emulated) and complex to support, which > might not be of concern here though. > But first and foremost nobody complained yet AFAIK. > Maybe keep it as is? Actively reject values too large for plain double > in printf(1) for now? > dunno. Maybe hide the float/double/long double behind a config ? Some people may want printf to support long values, while some other (like me) does not need it. Best regards, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: Regarding [PATCH] sysklogd: add -Z option to adjust message timezones
Hi Venkat, But i didn't get where *timezone *is defined and what it is contained, Can > you help us where it is defined and what it is contained. > timezone is defined in time.h and is documented in man 3 timezone. Best regards, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] libbb/last_char_is: rewrite for smaller and faster code
Hello, On Fri, Jul 3, 2020 at 9:15 PM Tito wrote: > > > > On 7/3/20 6:13 PM, Xabier Oneca -- xOneca wrote: > > Hi all, > > > >>> On Fri, 2020-07-03 at 08:12 +0200, Tito wrote: > >>> [...] > >>>> Improved version: > >>>> > >>>> char* last_char_is(const char *s, int c) { > >>>> if (!s || !*s) return NULL; > >>>> while (*(s + 1))s++; > >>>> return (c == *s) ? (char *)s : NULL; > >>>> } > > > > Let me "improve" the Tito's -11 bytes, probably introducing some subtle bug > > :) > > > > char* FAST_FUNC last_char_is(const char *s, int c) > > { > > if (!s || !*s) return NULL; > > while (*(++s)); > > return (*(--s) == c) ? s : NULL; > > } > > > > function old new delta > > last_char_is 58 42 -16 > > -- > > (add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-16) Total: -16 > > bytes > >textdata bss dec hex filename > > 95644942351904 962588 eb01c busybox_old > > 95643342351904 962572 eb00c busybox_unstripped > > > > Also, there's a warning compiling this function: return discards > > ‘const’ qualifier from pointer target type > > Hi, > cast it? > > char* FAST_FUNC last_char_is(const char *s, int c) > { > if (!s || !*s) return NULL; > while (*(++s)); > return (*(--s) == c) ? (char *)s : NULL; > } > In favor of const-correctness, wouldn't it be better to return a const char*? It's quite strange to promote a string to const and to a part of it that is non-const (not to mention that if you feed the function with a real const char* then the cast is not a good thing to do). > > > Fun to play with you. > > > > Cheers, > > > > Xabier Oneca_,,_ Best regards, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 4/5] httpd_indexcgi.c: Remove new lines in HTML
On Wed, Aug 5, 2020 at 2:40 PM Tim Tassonis wrote: > On 7/31/20 9:51 PM, Sergey Ponomarev wrote: > > Yes, it looks worse but you can see the resulting DOM tree instead > > (elements/inspector). TBH as a web developer I almost never looked into > > raw source and always watch the DOM tree. > > Not that I care terribly about this, but personally the newlines add > very little in size, but a whole lot in > readability/grep-ability/script-ability. > > The whole point of text formats generally are their readability without > fancy tools such as web inspector, and process-ability with > line-oriented tools such as sed/grep/head/tail/..., so removing the > newlines is clearly a huge step backwards, for very little gain in size. To add my two cents here, I'm with Tim and Denys here. I would prefer to not trade readability and scriptability of the generated HTML for a few bytes. Of course, you're free to ignore my opinion :) Best regards, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] chrt: support for musl C library
Hello, - Mail original - > On Fri, Sep 11, 2020 at 6:18 PM Christian Eggers > wrote: > > musl "implements" several sched_xxx() functions by returning > > ENOSYS. As > > an alternative, either pthread_(g|s)etschedparam() or direct > > syscalls > > can be used. > > > > References: > > https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/commit/schedutils/chrt.c?id=fcc3078754291d2f5121797eb91b364f8e24b2f1 > > References: > > http://git.musl-libc.org/cgit/musl/commit/src/sched/sched_setscheduler.c?id=1e21e78bf7a5c24c217446d8760be7b7188711c2 > > Signed-off-by: Christian Eggers > > --- > > util-linux/chrt.c | 31 +-- > > 1 file changed, 29 insertions(+), 2 deletions(-) > > > > diff --git a/util-linux/chrt.c b/util-linux/chrt.c > > index 4dd78dabf..e465d7cec 100644 > > --- a/util-linux/chrt.c > > +++ b/util-linux/chrt.c > > @@ -34,6 +34,9 @@ > > //usage: "You need CAP_SYS_NICE privileges to set scheduling > > attributes of a process" > > > > #include > > +#if defined (__linux__) > > +# include > > +#endif > > #include "libbb.h" > > #ifndef SCHED_IDLE > > # define SCHED_IDLE 5 > > @@ -85,6 +88,7 @@ int chrt_main(int argc UNUSED_PARAM, char **argv) > > char *priority = priority; /* for compiler */ > > const char *current_new; > > int policy = SCHED_RR; > > + int ret; > > > > opt = getopt32(argv, "^" > > "+" "mprfobi" > > @@ -132,7 +136,15 @@ int chrt_main(int argc UNUSED_PARAM, char > > **argv) > > if (opt & OPT_p) { > > int pol; > > print_rt_info: > > +#if defined (__linux__) && defined(SYS_sched_getscheduler) > > + /* musl libc returns ENOSYS for its > > sched_getscheduler library > > +* function, because the sched_getscheduler Linux > > kernel system call > > +* does not conform to Posix; so we use the system > > call directly > > +*/ > > + pol = syscall(SYS_sched_getscheduler, pid); > > +#else > > pol = sched_getscheduler(pid); > > +#endif > > if (pol < 0) > > bb_perror_msg_and_die("can't %cet pid %u's > > policy", 'g', (int)pid); > > #ifdef SCHED_RESET_ON_FORK > > @@ -149,7 +161,12 @@ int chrt_main(int argc UNUSED_PARAM, char > > **argv) > > printf("pid %u's %s scheduling policy: SCHED_%s\n", > > pid, current_new, policy_name(pol) > > ); > > - if (sched_getparam(pid, &sp)) > > +#if defined (__linux__) && defined(SYS_sched_getparam) > > + ret = syscall(SYS_sched_getparam, pid, &sp); > > +#else > > + ret = sched_getparam(pid, &sp); > > +#endif > > + if (ret) > > bb_perror_msg_and_die("can't get pid %u's > > attributes", (int)pid); > > printf("pid %u's %s scheduling priority: %d\n", > > (int)pid, current_new, sp.sched_priority > > @@ -168,7 +185,17 @@ int chrt_main(int argc UNUSED_PARAM, char > > **argv) > > sched_get_priority_min(policy), > > sched_get_priority_max(policy) > > ); > > > > - if (sched_setscheduler(pid, policy, &sp) < 0) > > +#if defined (__linux__) && defined(SYS_sched_setscheduler) > > + /* musl libc returns ENOSYS for its sched_setscheduler > > library > > +* function, because the sched_setscheduler Linux kernel > > system call > > +* does not conform to Posix; so we use the system call > > directly > > +*/ > > + ret = syscall(SYS_sched_setscheduler, pid, policy, &sp); > > +#else > > + ret = sched_setscheduler(pid, policy, &sp); > > +#endif > > + > > + if (ret < 0) > > bb_perror_msg_and_die("can't %cet pid %u's policy", > > 's', (int)pid); > > > > if (!argv[0]) /* "-p PRIO PID [...]" */ > > > I looked at it several times, but this looks not so good. > These direct syscalls are less maintainable. > > OTOH... by the looks of it, there is no alternative? Maybe define a function or even a static inline that does the job? That should not cause any issue and it should be easier to maintain (not to mention that it could be added to libbb if needed in order to be reused in other places). On a side note, instead of checking for __linux__ only, one can also check for !__GLIBC__ as well in order to limit the use case to libraries that are not the glibc (yes, I link busybox with the glibc, which more or less defeat the purpose of busybox). #if defined(__linux__) && defined(SYS_sched_setscheduler) && !defined(__GLIBC__) > I'll probably apply it. Best regards, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: six patches from tinycore linux
Hello, On Thu, Aug 12, 2021 at 4:01 PM Ron Yorston wrote: > > Roberto A. Foglietta wrote: > >Hi Ron, > > > > what's about the other five? > * busybox-1.33.0_skip-loop-control.patch makes get_free_loop() behave as if there was no free loop devices (unconditionnaly). It would help to have some background info here, as it does not seem to fix a bug. * busybox-1.29.3_root_path.patch adds /usr/local/bin to the list of default PATH ; the whole value can be configured by setting BB_ADDITIONAL_PATH so I'm not sure there is much value here. * busybox-1.27.1-wget-make-default-timeout-configurable.patch does exactly what it says it does - it makes the default wget timeout a configurable value. I agree that the default value of 900 seconds might be a bit surprising. A value in [30, 120] might have been better. I'm not sure whether the good thing to do is to make the value configurable or provide a saner default value (15 minutes? that's a bit long...). Is there a rationale for this very, very long timeout? * busybox-1.33.0_modprobe.patch checks whether the module is an ELF bianry before sending it to the kernel. Since the checks that the kernel performs are more comprehensive, I don't see much value in adding a simple check of the ELF header here -- not to mention that the code is duplicated. * busybox-1.33.0_tc_depmod.patch : it *seems* that this one solves a bug but I cannot wrap my head around it. More particularly, I'm wondering if this is a tinycore thing (having modules hidden behind symlinks) or if this is a real issue. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: shell function name length, question
Hello, On Sat, Aug 14, 2021 at 12:19 PM Roberto A. Foglietta wrote: > > Hi all, > > how long could be a shell function name? > > In my last patch, I used 256 characters because: "On Linux: The maximum > length for a file name is 255 bytes". So, I have extended the concept to a > function name but it is not the same. > > Thank you, According to the discussion in [1], both bash and NETBSD sh can be used with identifiers that are longer than 1000 characters. Bash seems to not have any limit on identifier names. The POSIX sh standard itself does not define anything related to the length of any identifier (be it a variable name or a function name). (I would totally accept shorter identifiers ; mine rarely goes larger than 30-40 bytes, and these are the longest ; yet it's not difficult to image a processor that could create longer identifier names). [1] https://lists.gnu.org/archive/html/bug-bash/2018-11/msg00069.html [2] https://pubs.opengroup.org/onlinepubs/9699919799/ > -- > Roberto A. Foglietta > +39.349.33.30.697 Best regards, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] ash: fix command name of shell scripts
Hello, This is a real issue (one I didn't report and fixed with a patch of my own, as I was not sure this was not caused by another patch of mine). But if I'm not the only one with the problem then it might not be my own fault :) The problem lies in appletlib.c, at the end of main() -- when (ENABLE_FEATURE_SH_STANDALONE || ENABLE_FEATURE_PREFER_APPLETS || !BB_MMU) is true. In this case, we force comm to be the applet name - and in this case, the applet name is sh which is not what we want to get. The way I fix the problem is quite un-sophisticated (so don't apply as is). (patch as attachment, because, well, gmail). It workds well enough for my own usage, but I bet it can be enhanced and corrected. The idea is to replace the comm by the applet name only if the comm is "exe" (which we want to avoid). For unknown reasons it seems to do the trick although I'm not sure why it does ; maybe the condition is not correct and we are not in the case where we always get comm == "exe". I did not investigate that much. Best regards, -- Emmanuel Deloget Le lun. 11 oct. 2021 à 14:24, a écrit : > > I forgot to mention that the issue isn't triggered by the default > busybox configuration. > I noticed the issue in OpenWrt which uses > CONFIG_FEATURE_PREFER_APPLETS=y by default, > and that option triggers the issue. > > Config.in says it's an experimental option. Does it mean it isn't > recommended, and that > OpenWrt maybe shouldn't enable it by default? > > /Mikael > > On 2021-10-11 02:58, Denys Vlasenko wrote: > > On Sun, Oct 10, 2021 at 11:09 PM wrote: > >> Hello, > >> > >> when executing a script with ash from busybox in the shebang then the > >> command name will contain "sh" instead of the name of the script as > >> expected. > > Can't reproduce: I see script name in comm field, not "sh" or "ash": > > > > $ cat comm.tests > > { > > echo "#!$THIS_SH" > > echo 'procdir=/proc/$$' > > echo 'echo " /proc/N/exe: $(basename $(readlink $procdir/exe))"' > > echo 'echo " /proc/N/comm: $(cat $procdir/comm)"' > > } >SCRIPT.sh > > chmod 755 SCRIPT.sh > > echo ./SCRIPT.sh: > > ./SCRIPT.sh > > echo "exec ./SCRIPT.sh:" > > (exec ./SCRIPT.sh) > > echo sh ./SCRIPT.sh: > > $THIS_SH ./SCRIPT.sh > > rm SCRIPT.sh > > > > $ THIS_SH=/bin/bash bash comm.tests > > ./SCRIPT.sh: > >/proc/N/exe: bash > >/proc/N/comm: SCRIPT.sh > > exec ./SCRIPT.sh: > >/proc/N/exe: bash > >/proc/N/comm: SCRIPT.sh > > sh ./SCRIPT.sh: > >/proc/N/exe: bash > >/proc/N/comm: bash > > > > $ THIS_SH=/bin/ash ash comm.tests > > ./SCRIPT.sh: > >/proc/N/exe: ash > >/proc/N/comm: SCRIPT.sh > > exec ./SCRIPT.sh: > >/proc/N/exe: ash > >/proc/N/comm: SCRIPT.sh > > sh ./SCRIPT.sh: > >/proc/N/exe: ash > >/proc/N/comm: ash > > > > $ ash --help > > BusyBox v1.35.0.git (2021-09-07 23:38:50 CEST) multi-call binary. > > ... > > > > $ bash --help > > GNU bash, version 5.0.17(1)-release-(x86_64-redhat-linux-gnu) > > ... > ___ > busybox mailing list > busybox@busybox.net > http://lists.busybox.net/mailman/listinfo/busybox --- a/libbb/appletlib.c +++ b/libbb/appletlib.c @@ -1065,8 +1065,13 @@ int main(int argc UNUSED_PARAM, char **a || ENABLE_FEATURE_PREFER_APPLETS || !BB_MMU ) { - if (NUM_APPLETS > 1) - set_task_comm(applet_name); + if (NUM_APPLETS > 1) { + char old_name[16]; + get_task_comm(old_name); + if (strcmp(old_name, "exe") == 0) { +set_task_comm(applet_name); + } + } } parse_config_file(); /* ...maybe, if FEATURE_SUID_CONFIG */ --- a/include/libbb.h +++ b/include/libbb.h @@ -1143,8 +1143,10 @@ void run_applet_no_and_exit(int a, const #endif #if defined(__linux__) void set_task_comm(const char *comm) FAST_FUNC; +void get_task_comm(char *comm) FAST_FUNC; #else # define set_task_comm(name) ((void)0) +# define get_task_comm(name) ((void)0) #endif /* Helpers for daemonization. --- a/libbb/vfork_daemon_rexec.c +++ b/libbb/vfork_daemon_rexec.c @@ -33,6 +33,10 @@ void FAST_FUNC set_task_comm(const char /* okay if too long (truncates) */ prctl(PR_SET_NAME, (long)comm, 0, 0, 0); } +void FAST_FUNC get_task_comm(char *comm) +{ + prctl(PR_GET_NAME, (long)comm, 0, 0, 0); +} #endif /* ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: Question about isolating and re-using code from busybox
Hello Tim, it seems to me that it should not be that complicated, as the tar handling code is mostly encapsulated into a library of its own. See https://git.busybox.net/busybox/tree/archival/libarchive for futher reference. Best regards, -- Emmanuel Deloget Le jeu. 3 mars 2022 à 18:59, Tim Tassonis a écrit : > > Hi all > > I've written a package manager in C for my distro where packages are > basically gzipped cpio (newc type) archives. > > I'm currently using the libarchive library in the program for > extraction, which works well, but is quite a big dependency. > > So, as busybox handles those archives well (I tested it), I though it > would be nice to get rid of libarchive and use the code from busybox. As > my package manager is GPL, too, there should also be no licensing issues. > > However, I have not dug too deep into the busybox code yet and just > wanted to ask whether someone knows if this is a good idea, e.g. how > much work it would be to isolate the needed parts for use in my code. > > Has anybody else here ever done a similar thing, or is there maybe even > a document giving a rough idea about what has to be considered or how to > go about that the easiest? > > Sorry if this post is considered off-topic, I just though maybe someone > has a quick answer that will save me some work. > > > Bye > Tim > > > ___ > busybox mailing list > busybox@busybox.net > http://lists.busybox.net/mailman/listinfo/busybox ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] seedrng: limit poolsize to 256 bytes and document flock() and fsync() usage
ntly store a seed file then I shall provide another way to seed the RNG. Fortunately, a lot of recent SoC have a hardware RNG which will participate to /dev/random early on. > I also wonder how sane it is to do 'new_key = f(old_key)'. > That doesn't seem significantly better than using the same key. f() being a hash function with a salt + a (maybe not enough) random value, I would think that new_key is going to be widely different than old_key and more or less unpredictable, which is what we want for a seed. > For a really embedded system the only persistent storage > could easily be a small serial EEPROM with a very limited > number of write cycles. > This requires special code to read/write and care to avoid > hitting the write cycle count on a small number of memory cells. > No amount of faffing about with filesystem accesses will > help here at all. As you noted by yourself, it requires special code to read/write, so unless this use-case is fully supported in busybox (with special implementation for open/read/write), I don't see why it should have an impact on the security of _all_ other devices. > There is also the case (that on my systems at least) udev > initialisation reads from /dev/[u]random well before the S20 > script loads any saved entropy. > I've not tried to find out what the value is used for. I find at least one occurrence where the goal is to create a random delay in udev-watch.c [1]. Anyway, and unless some weird security-oriented process decides to read some random bytes without checking whether the random pool in in a correct state (in which case I'm not sure you shall trust this security-oriented process in the first place) the goal of seedrng is not to protect all reads from /dev/urandom, but to make sure that we have enough entropy to do the Real Work. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 > 1PT, UK > Registration No: 1397386 (Wales) It seems to me that one might want to add a CONFIG_FAVOR_SIZE_OVER_SECURITY configuration flag. I wish to say that I'm not sure it's a good idea. Having a small busybox is a good thing, but having a too small busybox that breaks products because of bad security choices is not. Best regards, -- Emmanuel Deloget [1] https://github.com/systemd/systemd/blob/d9338387d94ad611233cf0753801eefccfda432a/src/udev/udev-watch.c#L102 ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] seedrng: limit poolsize to 256 bytes and document flock() and fsync() usage
Le lun. 2 mai 2022 à 03:31, Denys Vlasenko a écrit : > > On Sun, May 1, 2022 at 6:35 PM Emmanuel Deloget wrote: > > > > - RNG is seeded and credited using file A. > > > > - File A is unlinked but not fsync()d. > > > > - TLS connection does something and a nonce is generated. > > > > - System loses power and reboots. > > > > - RNG is seeded and credited using same file A. > > > > - TLS connection does something and the same nonce is generated, > > > > resulting in catastrophic cryptographic failure. > > > > > > > > This is a big deal. Crediting seeds is not to be taken lightly. > > > > Crediting file-based seeds is _only_ safe when they're never used > > > > twice. > > > > > > Using the same file twice is better than having nothing at all. > > > > I beg to differ, and especially on some embedded systems where the RNG > > might be quite controllable by an attacker from the outside (mostly because > > it lacks a lot of entropy crediting inputs, which is exactly the reason why > > we > > need seedrng in the first place). This may lead to catastrophic cryptography > > failures down the road. > > Did you personally encounter such a situation? > I'm not implying it's not really happening, I'm interested to hear > from people who met this situation in real world. Hey :) We're now entering in the "cautionary tales" territory :) This happened, yes. We have not seen any exploitation of this in nature (I would guess that it was mostly because there were other lower hanging fruits; why would you attack the cryptography when you could shellshock the device? yeah, bash was here -- it was a time where ash was not deemed good enough to run the CGI scripts :) ; and that was not the worst vulnerability so in practice you wouldn't even need to use it to take full control of the device) but it was scary to see that our different devices were able to boot and generate the same SSH keys. This happened only on their first boot when the device was not connected to the LAN (and not on all devices). Blank state, 0 source of entropy. Quite a "fun" time. Best regards, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] seedrng: limit poolsize to 256 bytes and document flock() and fsync() usage
Hi, Le lun. 2 mai 2022 à 11:37, Denys Vlasenko a écrit : > > On Mon, May 2, 2022 at 8:26 AM Emmanuel Deloget wrote: > > Le lun. 2 mai 2022 à 03:31, Denys Vlasenko a > > écrit : > > > > I beg to differ, and especially on some embedded systems where the RNG > > > > might be quite controllable by an attacker from the outside (mostly > > > > because > > > > it lacks a lot of entropy crediting inputs, which is exactly the reason > > > > why we > > > > need seedrng in the first place). This may lead to catastrophic > > > > cryptography > > > > failures down the road. > > > > > > Did you personally encounter such a situation? > > > I'm not implying it's not really happening, I'm interested to hear > > > from people who met this situation in real world. > > > > Hey :) We're now entering in the "cautionary tales" territory :) > > > > This happened, yes. We have not seen any exploitation of this in > > nature (I would guess that it was mostly because there were other > > lower hanging fruits; why would you attack the cryptography when you > > could shellshock the device? yeah, bash was here -- it was a time > > where ash was not deemed good enough to run the CGI scripts :) ; and > > that was not the worst vulnerability so in practice you wouldn't even > > need to use it to take full control of the device) but it was scary to > > see that our different devices were able to boot and generate the same > > SSH keys. This happened only on their first boot when the device was > > not connected to the LAN (and not on all devices). Blank state, 0 > > source of entropy. Quite a "fun" time. > > If you still have a device which exhibits this behavior (seemingly > totally deterministic state during each boot), can you try something like > > | xargs md5sum > > and see whether any /proc files display variability? Unfortunately, I no longer have access to these devices. Not many were sold, and I hope that none is still in activity today (although I think that a former colleague told me that some of them are no later than last year but I could be wrong). This was an old device (development started in 2004, production stopped in 2009 IIRC). Best regards, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] pw_encrypt: Add option to enable bcrypt support
$2b$ in your /etc/passwd or /etc/shadow files. These passwords > + are hashed using the bcrypt algorithm. Requires the use of a C > + library that supports bcrypt. > + > +config FEATURE_BCRYPT_COST > + int "bcrypt cost" > + range 4 31 > + default 10 > + depends on USE_BCRYPT > + help > + Cost paramter for the bcrypt hashing algorithm. typo: parameter > + Specifies the number of rounds to use. Must be between 4 and 31, > + inclusive. This value is logarithmic, the actual number of > + iterations used will be 2**rounds – increasing the rounds by +1 > + will double the amount of time taken. > + > INSERT > > endmenu > diff --git a/loginutils/chpasswd.c b/loginutils/chpasswd.c > index a032abbed..74673fa6f 100644 > --- a/loginutils/chpasswd.c > +++ b/loginutils/chpasswd.c > @@ -17,7 +17,8 @@ > //config: default "des" > //config: depends on PASSWD || CRYPTPW || CHPASSWD > //config: help > -//config: Possible choices are "d[es]", "m[d5]", "s[ha256]" or > "sha512". > +//config: Possible choices are "d[es]", "m[d5]", "s[ha256]", > +//config: "sha512" or "b[crypt]" (when enabled). > //applet:IF_CHPASSWD(APPLET(chpasswd, BB_DIR_USR_SBIN, BB_SUID_DROP)) > > -- > 2.34.1 Best regards, -- Emmanuel Deloget ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox