screen-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [screen-devel] Correctly format lock message if GECOS data is CSV


From: Amadeusz Sławiński
Subject: Re: [screen-devel] Correctly format lock message if GECOS data is CSV
Date: Fri, 1 Sep 2017 10:53:16 +0200

On Thu, 31 Aug 2017 14:05:53 +0200
Tore Sinding Bekkedal <address@hidden> wrote:

> Thank you very much for your comments, Amadeusz! You're right, strndup
> is much better, I didn't know about it.
> 
> I think I have incorporated your suggestions here. Because it's
> cosmetic I didn't see the need to panic if the allocation failed; I
> just fall back to the previous behavior.
> 
> I'm not sure how the list feels about the brevity of collapsing
> assignments and conditionals. I thought it was nice and concise at no
> real cost to clarity.
> 
> Submitted for your approval:
> 

Thanks, pushed to master.

> diff --git a/src/socket.c b/src/socket.c
> index 804592e..9dafeed 100644
> --- a/src/socket.c
> +++ b/src/socket.c
> @@ -1093,21 +1093,31 @@ struct pwdata {
>  static void AskPassword(Message *m)
>  {
>         struct pwdata *pwdata;
>         char prompt[MAXSTR];
> +       char * gecos_comma;
> +       char * realname = NULL;
> 
>         pwdata = calloc(1, sizeof(struct pwdata));
>         if (!pwdata)
>                 Panic(0, "%s", strnomem);
> 
>         pwdata->len = 0;
>         pwdata->m = *m;
> 
>         D_processinputdata = (char *)pwdata;
>         D_processinput = PasswordProcessInput;
> 
> -       snprintf(prompt, sizeof(prompt), "\ascreen used by %s%s<%s> on
> %s.\r\nPassword: ", ppp->pw_gecos,
> -                ppp->pw_gecos[0] ? " " : "", ppp->pw_name, HostName);
> +       /* if GECOS data is CSV, we only want the text before the first comma 
> */
> +       if ((gecos_comma = strchr(ppp->pw_gecos, ',')))
> +               if (!(realname = strndup(ppp->pw_gecos, gecos_comma -
> ppp->pw_gecos)))
> +                       gecos_comma = NULL; /* well, it was worth a shot. */
> +
> +       snprintf(prompt, sizeof(prompt), "\ascreen used by %s%s<%s> on
> %s.\r\nPassword: ",
> +                       gecos_comma ? realname : ppp->pw_gecos,
> +                       ppp->pw_gecos[0] ? " " : "", ppp->pw_name, HostName);
> +
> +       free(realname);
>         AddStr(prompt);
>  }
> 
>  #if ENABLE_PAM
> 
> 
> On Wed, Aug 30, 2017 at 5:25 PM, Amadeusz Sławiński <address@hidden> wrote:
> > Hey,
> >
> > thanks for patch, few notes inline
> >
> > On Mon, 28 Aug 2017 12:20:46 +0200
> > Tore Sinding Bekkedal <address@hidden> wrote:
> >  
> >> Hello - long time user, first time contributor.
> >>
> >> The screen lock message does not present GECOS data correctly if it -
> >> as is tradition - is a comma-separated string; so rather than:
> >>
> >> screen used by Tore Sinding Bekkedal <toresbe> on pascal.
> >>
> >> I get:
> >>
> >> screen used by Tore Sinding Bekkedal,,, <toresbe> on pascal.
> >>
> >> Here's a patch which should fix that. I hope it's been formatted
> >> correctly, sent to the right place, and meets the appropriate
> >> standards - if not, please let me know.
> >>
> >> diff --git a/src/socket.c b/src/socket.c
> >> index 804592e..4402016 100644
> >> --- a/src/socket.c
> >> +++ b/src/socket.c
> >> @@ -1105,8 +1105,19 @@ static void AskPassword(Message *m)
> >>         D_processinputdata = (char *)pwdata;
> >>         D_processinput = PasswordProcessInput;
> >>
> >> -       snprintf(prompt, sizeof(prompt), "\ascreen used by %s%s<%s> on
> >> %s.\r\nPassword: ", ppp->pw_gecos,
> >> +        char * gecos_comma = strchr(ppp->pw_gecos, ',');
> >> +        char * realname = 0;  
> >
> > Please declare variables at the beginning of function.
> >  
> >> +
> >> +        if (gecos_comma) {
> >> +            realname = malloc(gecos_comma - ppp->pw_gecos);  
> >
> > What happens if malloc fails? (look for "strnomem" in screen sources).
> >  
> >> +            memcpy(realname, ppp->pw_gecos, gecos_comma - ppp->pw_gecos); 
> >>  
> >
> > also instead of malloc/memcpy, I think it should be fine to just use
> > strndup() as it is more concise, but as with malloc it can fail to
> > allocate memory, so it needs to be handled.  
> >> +        }
> >> +
> >> +       snprintf(prompt, sizeof(prompt), "\ascreen used by %s%s<%s> on
> >> %s.\r\nPassword: ",
> >> +                gecos_comma ? realname : ppp->pw_gecos,
> >>                  ppp->pw_gecos[0] ? " " : "", ppp->pw_name, HostName);
> >> +
> >> +        free(realname);
> >>         AddStr(prompt);
> >>  }
> >>
> >> regards,  
> >
> >
> > Best regards,
> > Amadeusz Sławiński
> >  
> 
> 
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]