On Tue, Aug 5, 2014 at 2:14 PM, Laszlo Papp <lp...@kde.org> wrote: > > > > On Tue, Aug 5, 2014 at 2:06 PM, tito <farmat...@tiscali.it> wrote: > >> On Tuesday 05 August 2014 14:47:53 you wrote: >> > On Tue, Aug 5, 2014 at 1:42 PM, tito <farmat...@tiscali.it> wrote: >> > >> > > On Tuesday 05 August 2014 14:27:57 you wrote: >> > > > I disagree. There is nothing to reject here. It fixes _my issue_ at >> hand. >> > > > If you want to fix other bugs in your system that you are facing in >> > > > reality, by all means, fix them in a separate change. >> > > > >> > > > I definitely do not agree with dynamically allocated buffer for the >> > > simple >> > > > reasons of: >> > > > >> > > > 1) Slow. >> > > > 2) You would still need a maximum anyway. >> > > > 3) Needless code complication. >> > > > >> > > > Let us keep things simple and good, you know the good old KISS >> principle. >> > > > >> > > > P.S.: please do not bikeshed. >> > > > >> > > > >> > > > On Tue, Aug 5, 2014 at 1:17 PM, tito <farmat...@tiscali.it> wrote: >> > > > >> > > > > On Tuesday 05 August 2014 12:46:08 you wrote: >> > > > > > Here is my tested fix without being to debug the busybox code, >> so >> > > only >> > > > > code >> > > > > > reading and understanding were my friends: >> > > > > > >> > > > > > commit 9610650b6ce2a4c1904f78a2dcdb47cad3d2e3d1 >> > > > > > Author: Laszlo Papp <lp...@kde.org> >> > > > > > Date: Tue Aug 5 11:42:24 2014 +0100 >> > > > > > >> > > > > > Allow 256 bytes long usernames as per Unix standards >> (usually) >> > > > > > >> > > > > > diff --git a/libpwdgrp/pwd_grp.c b/libpwdgrp/pwd_grp.c >> > > > > > index 2060d78..368c252 100644 >> > > > > > --- a/libpwdgrp/pwd_grp.c >> > > > > > +++ b/libpwdgrp/pwd_grp.c >> > > > > > @@ -23,8 +23,8 @@ >> > > > > > >> > > >> /**********************************************************************/ >> > > > > > /* Sizes for statically allocated buffers. */ >> > > > > > >> > > > > > -#define PWD_BUFFER_SIZE 256 >> > > > > > -#define GRP_BUFFER_SIZE 256 >> > > > > > +#define PWD_BUFFER_SIZE LOGIN_NAME_MAX+256 >> > > > > > +#define GRP_BUFFER_SIZE LOGIN_NAME_MAX+256 >> > > > > > >> > > > > > >> > > >> /**********************************************************************/ >> > > > > > /* Prototypes for internal functions. */ >> > > > > > >> > > > > >> > > > > Hi, >> > > > > yes I've thought also about this solution but rejected it because: >> > > > > >> > > > > 1) there will not be enough space to hold the home dir if named >> the >> > > same >> > > > > as the user >> > > > > so you need at least: >> > > > > >> > > > > #define PWD_BUFFER_SIZE LOGIN_NAME_MAX+ 6 (for home) + >> LOGIN_NAME_MAX >> > > (or >> > > > > PATH_MAX?) + 256 (for the rest) >> > > > > >> > > > > yet this might not be enough as in my passwd file I see a 101 >> char >> > > long >> > > > > passwd. >> > > > > Add the gecos fields (arbitrary lenght) to it and the default >> > > shell >> > > > > and we are short on space again. >> > > > > >> > > > > 2) same for #define GRP_BUFFER_SIZE LOGIN_NAME_MAX+256: >> > > > > here you need to take into account the group member list with >> > > > > an arbitrary number of members: >> > > > > >> > > > > >> > > > > #define GRP_BUFFER_SIZE LOGIN_NAME_MAX+256+ LOGIN_NAME_MAX*n: >> > > > > >> > > > > The only clean solution to fix it forever is dynamically allocated >> > > buffers >> > > > > created at the time you read in the line from the passwd files. >> > > > > >> > > > > Ciao, >> > > > > Tito >> > > > > >> > > > > >> > > > > P.S.: please don't top post. >> > > > > >> > > > > >> > > > > > >> > > > > > On Tue, Aug 5, 2014 at 11:22 AM, Laszlo Papp <lp...@kde.org> >> wrote: >> > > > > > >> > > > > > > Yeah, mayhaps... Thanks for the prompt reply. I tried to >> debug the >> > > > > code, >> > > > > > > but the busybox code here is a bit messy heavily abusing >> macros in >> > > C >> > > > > and >> > > > > > > all that. It ain't easy I must confess! >> > > > > > > >> > > > > > > For instance, see this section: >> > > > > > > >> > > > > > > http://git.busybox.net/busybox/tree/libpwdgrp/pwd_grp.c#n227 >> > > > > > > >> > > > > > > The _source_ is included several times always getting a new >> meaning >> > > > > based >> > > > > > > on some defines... Now, check this function: >> > > > > > > >> > > > > > > >> > > http://git.busybox.net/busybox/tree/libpwdgrp/pwd_grp_internal.c#n20 >> > > > > > > >> > > > > > > "resultbuf" will be always different depending on which >> include it >> > > is. >> > > > > > > Since it is failing at the pw_name check in there, I wanted to >> > > print it >> > > > > > > out, but no easy joy there as printing it like that will yield >> > > > > compilation >> > > > > > > error when the file is being included next time from above. >> > > > > > > >> > > > > > > Right, I thought instead of doing some "#if a == b" hackery, >> > > debugging >> > > > > > > would be easier, BUT: >> > > > > > > >> > > > > > > 1) The default build is stripped, yuck! >> > > > > > > >> > > > > > > 2) The unstripped build cannot locate the symbols (*). >> > > > > > > >> > > > > > > So, I am giving up on this for now; this is not the type of >> source >> > > code >> > > > > > > that is so pleasant to work with. ;-) >> > > > > > > >> > > > > > > Cheers, L. >> > > > > > > >> > > > > > > * (gdb) b main >> > > > > > > >> > > > > > > Breakpoint 2 at 0x407c71 >> > > > > > > (gdb) r >> > > > > > > Starting program: >> /home/lpapp/Projects/busybox/busybox_unstripped >> > > > > deluser >> > > > > >> > > >> fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff >> > > > > > > warning: Could not load shared library symbols for >> linux-vdso.so.1. >> > > > > > > Do you need "set solib-search-path" or "set sysroot"? >> > > > > > > >> > > > > > > Breakpoint 2, 0x0000000000407c71 in main () >> > > > > > > (gdb) list >> > > > > > > No symbol table is loaded. Use the "file" command. >> > > > > > > (gdb) >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > On Mon, Aug 4, 2014 at 10:40 PM, tito <farmat...@tiscali.it> >> > > wrote: >> > > > > > > >> > > > > > >> On Monday 04 August 2014 19:06:39 Laszlo Papp wrote: >> > > > > > >> > Hi, >> > > > > > >> > >> > > > > > >> > sudo busybox adduser >> > > > > > >> > >> > > > > > >> >> > > > > >> > > >> fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff >> > > > > > >> > passwd: unknown user >> > > > > > >> > >> > > > > > >> >> > > > > >> > > >> fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff >> > > > > > >> > >> > > > > > >> > Yet, the user is created in /etc/shadow: >> > > > > > >> > >> > > > > > >> > >> > > > > > >> >> > > > > >> > > >> fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff:!:16286:0:99999:7::: >> > > > > > >> > >> > > > > > >> > This is at least one issue, but there is another here: >> > > > > > >> > >> > > > > > >> > sudo busybox deluser >> > > > > > >> > >> > > > > > >> >> > > > > >> > > >> fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff >> > > > > > >> > deluser: unknown user >> > > > > > >> > >> > > > > > >> >> > > > > >> > > >> fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff >> > > > > > >> > >> > > > > > >> > So, basically, once you create that long username, you >> cannot >> > > > > remove it >> > > > > > >> > anymore with busybox and it keeps polluting your system. >> > > > > > >> > >> > > > > > >> > You have to gain other means to clean up! I am using this >> > > version >> > > > > over >> > > > > > >> here: >> > > > > > >> > >> > > > > > >> > BusyBox v1.22.1 (2014-06-02 14:47:37 MSK) multi-call >> binary. >> > > > > > >> > >> > > > > > >> > Could you please look into this and potentially fix it? >> Thanks >> > > in >> > > > > > >> advance. >> > > > > > >> > >> > > > > > >> > Cheers, L. >> > > > > > >> > >> > > > > > >> Hi, >> > > > > > >> if disabling libb's internal pwd/grp lib and by jumping >> through >> > > some >> > > > > hops >> > > > > > >> it >> > > > > > >> works for me: >> > > > > > >> >> > > > > > >> I need to add the group first as my system's groupadd command >> > > called >> > > > > by >> > > > > > >> bb's adduser >> > > > > > >> supports only groupnames of max 32 chars. >> > > > > > >> >> > > > > > >> ./busybox addgroup >> > > > > > >> >> > > > > >> > > >> fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff >> > > > > > >> ./busybox adduser >> > > > > > >> >> > > > > >> > > >> fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff >> > > > > > >> -G >> > > > > > >> >> > > > > >> > > >> fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff >> > > > > > >> Adding user >> > > > > > >> >> > > > > >> > > >> `fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff' >> > > > > > >> to group >> > > > > > >> >> > > > > >> > > >> `fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff' >> > > > > > >> ... >> > > > > > >> Adding user >> > > > > > >> >> > > > > >> > > >> fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff >> > > > > > >> to group >> > > > > > >> >> > > > > >> > > >> fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff >> > > > > > >> Done. >> > > > > > >> Enter new UNIX password: >> > > > > > >> Retype new UNIX password: >> > > > > > >> passwd: password updated successfully >> > > > > > >> >> > > > > > >> and then I could also delete it: >> > > > > > >> >> > > > > > >> ./busybox deluser >> > > > > > >> >> > > > > >> > > >> fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff >> > > > > > >> ./busybox delgroup >> > > > > > >> >> > > > > >> > > >> fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff >> > > > > > >> delgroup: unknown group >> > > > > > >> >> > > > > >> > > >> fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff >> > > > > > >> >> > > > > > >> (deluser correctly deleted also the group) >> > > > > > >> >> > > > > > >> I suspect that the buffer size used in libbpwdgrp/pwd_grp.c >> is to >> > > > > small: >> > > > > > >> >> > > > > > >> #define PWD_BUFFER_SIZE 256 >> > > > > > >> #define GRP_BUFFER_SIZE 256 >> > > > > > >> >> > > > > > >> as it is meant for the whole struct pw passwd (or struct gr >> group) >> > > > > fields >> > > > > > >> which could be easily be bigger: >> > > > > > >> >> > > > > > >> grep ffff* /etc/shadow | wc >> > > > > > >> 1 1 240 >> > > > > > >> grep ffff* /etc/passwd | wc >> > > > > > >> 1 2 286 >> > > > > > >> >> > > > > > >> I think that the buffers' size must be increased for example >> to: >> > > > > > >> >> > > > > > >> #define PWD_BUFFER_SIZE LOGIN_NAME_MAX+LOGIN_NAME_MAX+256 >> > > > > > >> >> > > > > > >> we need one LOGIN_NAME_MAX size for the login and one more >> for the >> > > > > home >> > > > > > >> dir >> > > > > > >> if same as login, plus 256 for the remaining data (passwd, >> gecos, >> > > > > shell, >> > > > > > >> etc). >> > > > > > >> >> > > > > > >> #define GRP_BUFFER_SIZE LOGIN_NAME_MAX+256+LOGIN_NAME_MAX*n >> (?) >> > > > > > >> >> > > > > > >> we need one LOGIN_NAME_MAX size for the group name (for >> which we >> > > > > > >> actually enforce the same size as for the username) plus 256 >> for >> > > the >> > > > > > >> remaining data >> > > > > > >> plus LOGIN_NAME_MAX * n group member names. >> > > > > > >> >> > > > > > >> So for my limited understanding using static buffers here is >> > > rather >> > > > > > >> difficult >> > > > > > >> as the size of data is not easily predictable. >> > > > > > >> I don't know how real libc manages it (maybe realloc on >> ERANGE?) >> > > > > > >> >> > > > > > >> Your particular example for me is fixed by using. >> > > > > > >> >> > > > > > >> #define PWD_BUFFER_SIZE 1024 >> > > > > > >> >> > > > > > >> #define GRP_BUFFER_SIZE 1024 >> > > > > > >> >> > > > > > >> But to me it seems not an optimal solution, >> > > > > > >> so other more experienced developers should >> > > > > > >> take a look at it. >> > > > > > >> >> > > > > > >> Hope this helps. >> > > > > > >> Ciao, >> > > > > > >> Tito >> > > > > > >> >> > > > > > >> PS: in libbbpwdgrp functions we need to check for errors: >> > > > > > >> >> > > > > > >> 0 or ENOENT or ESRCH or EBADF or EPERM or ... >> > > > > > >> The given name or gid was not found. >> > > > > > >> >> > > > > > >> EINTR A signal was caught. >> > > > > > >> >> > > > > > >> EIO I/O error. >> > > > > > >> >> > > > > > >> EMFILE The maximum number (OPEN_MAX) of files was open >> > > already >> > > > > in >> > > > > > >> the calling process. >> > > > > > >> >> > > > > > >> ENFILE The maximum number of files was open already >> in the >> > > > > system. >> > > > > > >> >> > > > > > >> ENOMEM Insufficient memory to allocate group >> structure. >> > > > > > >> >> > > > > > >> ERANGE Insufficient buffer space supplied. >> > > > > > >> >> > > > > > >> as for example the xgroup_study function in the addgroup >> applet >> > > > > > >> assigns a wrong gid if getgrgid fails for example for ERANGE >> > > > > > >> >> > > > > > >> /* Check if the desired gid is free >> > > > > > >> * or find the first free one */ >> > > > > > >> while (1) { >> > > > > > >> printf("gid %d\n", g->gr_gid); >> > > > > > >> if (!getgrgid(g->gr_gid)) { >> > > > > > >> return; /* found free group: return >> */ >> > > > > > >> } >> > > > > > >> >> > > > > > >> >> > > Hi, >> > > I doubt it fixes your issue >> > >> > >> > What you are basically telling me here that I submitted untested changes >> > for my use cases, directly or indirectly. Either way, I refuse this >> claim >> > since I _did_ test the change for my use case. >> > >> > >> > > because I tested the same fix and it did in fact >> > >> > fail. Please test: >> > > >> > > 1) ./busybox addgroup LONGUSERNAME (this fails in subtle ways as a >> > > already in use gid is assigned) >> > > 2) ./busybox adduser LONGUSERNAME -G LONGUSERNAME >> > > 3) ./busybox deluser LONGUSERNAME >> > > 4) ./busybox delgroup LONGUSERNAME >> > > >> > >> > I do not see how this is any relevant to my initial steps. This is a >> > completely different test step with a different issue. As I said >> earlier, >> > please welcome contributors rather than being blocker just because they >> fix >> > one particular and real bug, and not every "imaginary" use case that can >> > just exist in the world out there. >> > >> >> Hi, >> there is nothing imaginary here, i just would like to point out that >> due to the fact that busybox adduser program by default creates >> a home dir with the same name as the user the buffer size you >> propose in your fix may not be big enough to hold all the data >> and that this fact will affect also other commands that use >> the functions in libbbpwdgrp in subtle ways. >> I of course welcome you as a contributor and by the way I have >> no decisional power to accept or reject your patches (which is fine >> for me), but nonetheless sometimes I disagree with you for the sake >> of cleaner solutions (from my point of view). >> > > This script disagrees with you: > > #!/bin/bash > > USERNAME='e' > for i in {1..232} > do > USERNAME+='f' > done > echo $USERNAME > > ./busybox adduser -D $USERNAME > ./busybox deluser $USERNAME >
Furthermore, I am planning to use -H for now, so I am not in any way affected by this (other than -H is broken here for some reason because it does create the home folder in /etc/passwd, but that is discussion for another thread). If you really need to support home in practice now, too, I can add a "2*foo" (not foo + foo though because it is ugly IMHO ;) ), but I do not have the urgent need for it, however I do have the urgent need for the fix I am proposing.
_______________________________________________ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
