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

[Axel Beckert]

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! :-)

* 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.

  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.)

  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. :-)

  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

                Kind regards, Axel
-- 
/~\  Plain Text Ribbon Campaign                   | Axel Beckert
\ /  Say No to HTML in E-Mail and News            | address@hidden  (Mail)
 X   See http://www.nonhtmlmail.org/campaign.html | address@hidden (Mail+Jabber)
/ \  I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)