Re: [screen-devel] Remarks on the patch for long login names (was: Re: GNU Screen v.4.2.0)

[Amadeusz Sławiński]

On Tue, 22 Apr 2014 17:39:17 +0200
Axel Beckert <address@hidden> 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