Re: [screen-devel] screen cannot reattach to previous session

[Amadeusz Sławiński]

On Wed, 14 Oct 2015 13:45:04 +0200
Petr Hracek <address@hidden> wrote:

> Hi Amadeusz,
> 
> based on our last discussion, I tried to use it
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=644788;msg=265;att=0
> 
> And I guess, that If I already updated screen with different message 
> structure.
> then now I am not able to reconnect as you mentioned.
> Althought WriteOldMessage(&m)  is used.

Hey,

Yes struct old_msg may need adjusting along with WriteOldMessage
function, so passing new values to old_msg works.

Although now that I look at this patch again, it seems very hacky.
It copies values from attaching (new) client, into old_msg struct of
server and attaches to it with new client. It's wonder that it doesn't
crash, as even with copied values, struct between client and server are
different.

Amadeusz

> 
> Greetings
> Petr
> 
> On 10/12/2015 07:12 PM, Amadeusz Sławiński wrote:
> > On Mon, 12 Oct 2015 12:24:56 +0200
> > Petr Hracek <address@hidden> wrote:
> >
> >> On 09/08/2015 06:26 PM, Amadeusz Sławiński wrote:
> >>> On Tue, 8 Sep 2015 12:08:54 +0200
> >>> Petr Hracek <address@hidden> wrote:
> >>>
> >>>> On 08/31/2015 07:41 PM, Amadeusz Sławiński wrote:
> >>>>>> I have received a bug
> >>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1253697
> >>>>>> which causes that screen indefinite hangs.
> >>>>>>
> >>>>>> Strace is a part of the bug.
> >>>>>> Did you ever discover this problem?
> >>>>>>
> >>>>>> This bug was discovered in screen-4.1.0.
> >>>>> Hi,
> >>>>>
> >>>>> I recall that someone on irc had similar problem, it seemed like
> >>>>> his /dev/pts was being mounted with wrong options
> >>>>> it needed something like those mount options to work:
> >>>>> "gid=5,mode=620" (group 5 being tty on my system).
> >>>>>
> >>>>> Amadeusz
> >>>>>
> >>>> Hi,
> >>>>
> >>>> they provided a options:
> >>>>
> >>>> /dev/pts mount:
> >>>>
> >>>> $ mount | grep pts
> >>>> devpts on /dev/pts type devpts
> >>>> (rw,nosuid,noexec,relatime,seclabel,gid=5,mode=620,ptmxmode=000)
> >>>>
> >>>> But it seems that it is not working still.
> >>>>
> >>>> What about ptmxmode?
> >>>> Could it be relevant?
> >>> Hi,
> >>>
> >>> I don't think so, mine is also 000.
> >>> As far as I can see from bugreport it happens between updates.
> >>> What was version before and after update?
> >>> Also do you have somewhere diff between those versions?
> >>>
> >>> It can be that there is some difference in "struct msg" between
> >>> versions which causes this problem.
> >>>
> >>> Does it only fails to reattach after update?
> >>> By which I mean, does new sessions after update attach properly?
> >>>
> >>> Amadeusz
> >>>
> >> Finally I have got the reason why screen hangs.
> >> It is caused by patch "LoginName too long".
> >>
> >> See related report
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1119794 Patch for this
> >> case was mentioned there.
> >>
> > Hi,
> > " You are not authorized to access bug #1119794. To see this bug,
> > you must first log in to an account with the appropriate
> > permissions. "
> >
> >> Screen related bug is https://savannah.gnu.org/bugs/?21653
> >> Patch which we used is
> >> http://www.mail-archive.com/address@hidden/msg00829.html
> >> and another patch.
> >> https://lists.gnu.org/archive/html/screen-devel/2011-05/msg00000.html'
> >> What is the correct one for this bug?
> > Yeah, those were my first steps in official tree, so I got to a bit
> > messy start, there were some discussions which resulted in:
> > http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4&id=785f5992d84bb72a79ec6cd15389c79b9a434e17
> > http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4&id=b62e4ef0971fc9bbf5298d810819f406abb23862
> > http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4&id=fe8103cccd58c2e5de40634e694e2d99afce67ca
> > http://git.savannah.gnu.org/cgit/screen.git/commit/?h=screen-v4&id=ae76b78836aec6018d6b008c312c4d8ecf70326e
> >
> >> And unfortunately I have applied the wrong patch like:
> > The patch itself doesn't seem wrong, however...
> >
> >> diff --git a/acls.c b/acls.c
> >> index b8a5af4..b8ef048 100644
> >> --- a/acls.c
> >> +++ b/acls.c
> >> @@ -182,7 +182,7 @@ struct acluser **up;
> >>    #endif
> >>      (*up)->u_Esc = DefaultEsc;
> >>      (*up)->u_MetaEsc = DefaultMetaEsc;
> >> -  strncpy((*up)->u_name, name, 20);
> >> +  strncpy((*up)->u_name, name, MAXLOGINLEN);
> >>      (*up)->u_password = NULL;
> >>      if (pass)
> >>        (*up)->u_password = SaveStr(pass);
> >> @@ -318,8 +318,8 @@ struct acluser **up;
> >>        return UserAdd(name, pass, up);
> >>      if (!strcmp(name, "nobody"))        /* he remains without
> >> password */ return -1;
> >> -  strncpy((*up)->u_password, pass ? pass : "", 20);
> >> -  (*up)->u_password[20] = '\0';
> >> +  strncpy((*up)->u_password, pass ? pass : "", MAXLOGINLEN);
> >> +  (*up)->u_password[MAXLOGINLEN] = '\0';
> >>      return 0;
> >>    }
> >>    #endif
> >> diff --git a/acls.h b/acls.h
> >> index 907e953..42c7c18 100644
> >> --- a/acls.h
> >> +++ b/acls.h
> >> @@ -78,7 +78,7 @@ struct plop
> >>    typedef struct acluser
> >>    {
> >>      struct acluser *u_next;        /* continue the main user list
> >> */
> >> -  char u_name[20+1];        /* login name how he showed up */
> >> +  char u_name[MAXLOGINLEN + 1];        /* login name how he showed
> >> up */ char *u_password;        /* his password (may be NullStr). */
> >>      int  u_checkpassword;        /* nonzero if this u_password is
> >> valid */ int  u_detachwin;        /* the window where he last
> >> detached */ diff --git a/comm.c b/comm.c
> >> index 5f4af8a..7705fcb 100644
> >> --- a/comm.c
> >> +++ b/comm.c
> >> @@ -36,6 +36,7 @@
> >>     */
> >>
> >>    #include "config.h"
> >> +#include "os.h"
> >>    #include "acls.h"
> >>    #include "comm.h"
> >>
> >> diff --git a/display.h b/display.h
> >> index b1ab748..a433e6d 100644
> >> --- a/display.h
> >> +++ b/display.h
> >> @@ -73,7 +73,7 @@ struct display
> >>      struct win *d_other;        /* pointer to other window */
> >>      int   d_nonblock;        /* -1 don't block if obufmax reached
> >> */ /* >0: block after nonblock secs */
> >> -  char  d_termname[40 + 1];    /* $TERM */
> >> +  char  d_termname[MAXTERMLEN + 1];    /* $TERM */
> >>      char    *d_tentry;        /* buffer for tgetstr */
> >>      char    d_tcinited;        /* termcap inited flag */
> >>      int    d_width, d_height;    /* width/height of the screen */
> >> diff --git a/os.h b/os.h
> >> index 5c17c83..92846ad 100644
> >> --- a/os.h
> >> +++ b/os.h
> >> @@ -521,3 +521,8 @@ typedef struct fd_set { int fds_bits[1]; }
> >> fd_set; */
> >>    #define IOSIZE        4096
> >>
> >> +/* Changing those you won't be able to attach to your old sessions
> >> + * when changing those values in official tree don't forget to
> >> bump
> >> + * MSG_VERSION */
> >> +#define MAXTERMLEN  50
> >> +#define MAXLOGINLEN 256
> > here is your problem, MSG_VERSION is not revbumped in patch
> > (if you check git links above, I've revbumped it separately before
> > release). Basically it changes struct which screen uses to maintain
> > it's state, so attaching to previous versions doesn't work.
> > If you've used some other patches to fix this problem before, you
> > can change those values to be identical to those used previously,
> > and it should probably work fine then.
> >
> > Amadeusz
> >
> >
> >> diff --git a/process.c b/process.c
> >> index 0769cbe..623bf87 100644
> >> --- a/process.c
> >> +++ b/process.c
> >> @@ -2664,9 +2664,9 @@ int key;
> >>          s = NULL;
> >>          if (ParseSaveStr(act, &s))
> >>        break;
> >> -      if (strlen(s) >= 20)
> >> +      if (strlen(s) >= MAXTERMLEN)
> >>        {
> >> -      OutputMsg(0, "%s: term: argument too long ( < 20)",
> >> rc_name);
> >> +     OutputMsg(0, "%s: term: argument too long ( < %d)", rc_name,
> >> MAXTERMLEN);
> >>          free(s);
> >>          break;
> >>        }
> >> diff --git a/screen.c b/screen.c
> >> index 1c88157..eda16a3 100644
> >> --- a/screen.c
> >> +++ b/screen.c
> >> @@ -995,10 +995,10 @@ char **av;
> >>
> >>      if (home == 0 || *home == '\0')
> >>        home = ppp->pw_dir;
> >> -  if (strlen(LoginName) > 20)
> >>    #ifdef MULTIUSER
> >> -  if (multi && strlen(multi) > 20)
> >> +  if (multi && strlen(multi) > MAXLOGINLEN)
> >>        Panic(0, "Screen owner name too long - sorry.");
> >>    #endif
> >>      if (strlen(home) > MAXPATHLEN - 25)
> >> diff --git a/screen.h b/screen.h
> >> index 1a388e3..b95f8a2 100644
> >> --- a/screen.h
> >> +++ b/screen.h
> >> @@ -202,32 +202,32 @@ struct msg
> >>          int nargs;
> >>          char line[MAXPATHLEN];
> >>          char dir[MAXPATHLEN];
> >> -      char screenterm[20];    /* is screen really "screen" ? */
> >> +     char screenterm[MAXTERMLEN];    /* is screen really
> >> "screen" ? */ }
> >>          create;
> >>          struct
> >>        {
> >> -      char auser[20 + 1];    /* username */
> >> +     char auser[MAXLOGINLEN + 1];    /* username */
> >>          int apid;        /* pid of frontend */
> >>          int adaptflag;    /* adapt window size? */
> >>          int lines, columns;    /* display size */
> >>          char preselect[20];
> >>          int esc;        /* his new escape character unless -1 */
> >>          int meta_esc;        /* his new meta esc character unless
> >> -1 */
> >> -      char envterm[40 + 1];    /* terminal type */
> >> +     char envterm[MAXTERMLEN + 1];    /* terminal type */
> >>          int encoding;        /* encoding of display */
> >>          int detachfirst;      /* whether to detach remote sessions
> >> first */ }
> >>          attach;
> >>          struct
> >>        {
> >> -      char duser[20 + 1];    /* username */
> >> +     char duser[MAXLOGINLEN + 1];    /* username */
> >>          int dpid;        /* pid of frontend */
> >>        }
> >>          detach;
> >>          struct
> >>        {
> >> -      char auser[20 + 1];    /* username */
> >> +     char auser[MAXLOGINLEN + 1];    /* username */
> >>          int nargs;
> >>          char cmd[MAXPATHLEN];    /* command */
> >>          int apid;        /* pid of frontend */
> >> diff --git a/socket.c b/socket.c
> >> index 45887fb..b4d2400 100644
> >> --- a/socket.c
> >> +++ b/socket.c
> >> @@ -1528,7 +1528,7 @@ static void PasswordProcessInput __P((char *,
> >> int));
> >>
> >>    struct pwdata {
> >>      int l;
> >> -  char buf[20 + 1];
> >> +  char buf[MAXLOGINLEN + 1];
> >>      struct msg m;
> >>    };
> >>
> >>
> >> Greetings.
> >>
> >>
> 
>