On Tue, 22 Apr 2014 17:39:17 +0200 Axel Beckert <a...@deuxchevaux.org> wrote:
> Hi, > > On Tue, Apr 22, 2014 at 02:20:21PM +0200, Amadeusz Sławiński wrote: > > I also pushed patch for fixing login length limit, because it was > > also 20 chars, but can have up to 32. > > I think you refer to > http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4&id=b62e4ef0971fc9bbf5298d810819f406abb23862 > > A few remarks to that commit: > > * With that commit you can also close > https://savannah.gnu.org/bugs/?21653 > > Yay! :-) Yes, I will look through bugzilla I'm pretty sure this one is also fixed with altscreen patch for example: https://savannah.gnu.org/bugs/?35757 > * 32 seems a little bit short to me. Why not use what the system > provides? According to https://bugs.debian.org/560231 at least on > Linux there is LOGIN_NAME_MAX defined in > /usr/include/bits/local_lim.h which seems to default to 256 > nowadays. I think screen should support at least that on Linux then, > too. > Ah, I based my value on utmp entries bits/utmp.h:#define UT_NAMESIZE 32 bits/utmp.h: char ut_user[UT_NAMESIZE]; /* Username. */ bits/utmpx.h:#define __UT_NAMESIZE 32 bits/utmpx.h: char ut_user[__UT_NAMESIZE]; /* Username. */ Well, let's just use 256, it will probably allow for use with most crazy login schemes. > I hence propose to check if LOGIN_NAME_MAX is defined and if so, use > that, otherwise use a fixed value, maybe 32 to be on the save side > for ancient OS. The patch proposed in > https://lists.gnu.org/archive/html/screen-devel/2011-05/msg00000.html > (see also below) proposed to use 50, but I prefer numbers which are > powers of two. (Debian uses 50 currently, too, though.) In previous mail Jürgen suggested that it's bad idea to have build time changing defines in struct msg. I will hardcode them. > If wanted, I can try to figure out a patch for that. > > * Can we please make that value a single "#define" somewhere for once > and for all, so that the next time we have to change that, we only > have to change it at one single place? (Unless we missed some code > parts. :-) > Found a perfect place, os.h at bottom allows for user defined values. > Here's a patch which did that (applied in the Debian package, but > won't apply anymore if Amadeusz's patch is already applied): > > > http://anonscm.debian.org/gitweb/?p=collab-maint/screen.git;a=blob;f=debian/patches/49long-usernames.patch > > (based on > https://lists.gnu.org/archive/html/screen-devel/2011-05/msg00000.html, > but that one missed at least one more place where that restriction > was hardcoded, see https://bugs.debian.org/735554) > > * You seem to have missed the same multiuser code part as the patch in > https://lists.gnu.org/archive/html/screen-devel/2011-05/msg00000.html > and as explained in https://bugs.debian.org/735554 -- it's even > visible in > > http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4&id=b62e4ef0971fc9bbf5298d810819f406abb23862 > > See this hunk: > > diff --git a/src/screen.c b/src/screen.c > index 6e19732..fd6eb2f 100644 > --- a/src/screen.c > +++ b/src/screen.c > @@ -978,7 +978,7 @@ char **av; > > if (home == 0 || *home == '\0') > home = ppp->pw_dir; > - if (strlen(LoginName) > 20) > + if (strlen(LoginName) > 32) > Panic(0, "LoginName too long - sorry."); > #ifdef MULTIUSER > if (multi && strlen(multi) > 20) > ^^ > > That last line needs to be changed to 32 (or LOGIN_NAME_MAX or > MAX_USERNAME_LEN or whatever), too, because the whole #ifdef reads > as follows: > > #ifdef MULTIUSER > if (multi && strlen(multi) > 20) > Panic(0, "Screen owner name too long - sorry."); > #endif > Yes, thanks for checking. Amadeusz
