[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Alejandro Colomar]

Hi Paul,

On 3/12/23 23:24, Alejandro Colomar wrote:
> Hi Paul,
> 
> On 3/12/23 22:50, Paul Eggert wrote:
>> On 2023-03-12 08:28, Alejandro Colomar wrote:
>>
>>> I've pushed a signed tag paul1, so you can safely check that the
>>> repo is mine (since I don't have HTTPS).
>>
>> Thanks, I'm not sure what exactly this means as I don't contribute to 
>> shadow-devel. As far as the remaining patches go, please use your best 
>> judgment as I'm running low on time to worry about this.
> 
> Okay.  shadow accepts patches as Github pull requests[1], so I'll open a
> pull request with the patches I already took from you, plus the
> remaining ones which I'll tweak a bit following your suggestion.  When
> it's done, I'll CC you so you can have a look at the final patches.
> 
> Thanks for your patches!  :-)

In case you didn't receive a notification (I mentioned you on Github),
here's a link to the pull request:


Cheers,

Alex

> 
> Alex
> 
> 
> [1]:  
> 

-- 

GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


OpenPGP_signature
Description: OpenPGP digital signature
___
Pkg-shadow-devel mailing list
Pkg-shadow-devel@alioth-lists.debian.net
https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel

[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Alejandro Colomar]

Hi Paul,

On 3/12/23 22:50, Paul Eggert wrote:
> On 2023-03-12 08:28, Alejandro Colomar wrote:
> 
>> I've pushed a signed tag paul1, so you can safely check that the
>> repo is mine (since I don't have HTTPS).
> 
> Thanks, I'm not sure what exactly this means as I don't contribute to 
> shadow-devel. As far as the remaining patches go, please use your best 
> judgment as I'm running low on time to worry about this.

Okay.  shadow accepts patches as Github pull requests[1], so I'll open a
pull request with the patches I already took from you, plus the
remaining ones which I'll tweak a bit following your suggestion.  When
it's done, I'll CC you so you can have a look at the final patches.

Thanks for your patches!  :-)

Alex


[1]:  

-- 

GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


OpenPGP_signature
Description: OpenPGP digital signature
___
Pkg-shadow-devel mailing list
Pkg-shadow-devel@alioth-lists.debian.net
https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel

[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Paul Eggert]

On 2023-03-12 08:28, Alejandro Colomar wrote:


I've pushed a signed tag paul1, so you can safely check that the
repo is mine (since I don't have HTTPS).


Thanks, I'm not sure what exactly this means as I don't contribute to 
shadow-devel. As far as the remaining patches go, please use your best 
judgment as I'm running low on time to worry about this.


___
Pkg-shadow-devel mailing list
Pkg-shadow-devel@alioth-lists.debian.net
https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel

[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Alejandro Colomar]

Hi Bálint,

On 3/12/23 20:22, Bálint Réczey wrote:
> Hi Alejandro,
> 
> Alejandro Colomar  ezt írta (időpont: 2023.
> márc. 12., V, 16:52):
>>
>> Hi Bálint,
>>
>> On 3/12/23 16:38, Bálint Réczey wrote:
 142 lines of a function definition are not something I'd consider easy to
 maintain.  Is it a big deal to add another dependency?  I'd say it's a
 bigger deal to copy verbatim so many lines of code, and sync them from
 time to time from libbsd (or OpenBSD) just to bring in any bugfixes they
 apply.  That's exactly the purpose of libbsd, so I think relying on them
 should be fine.
>>>
>>> The function does not change often. It changed two times in the last 13 
>>> years:
>>> https://gitlab.freedesktop.org/libbsd/libbsd/-/commits/main/src/readpassphrase.c
>>>
>>> I'd be happy to add a GitHub Action job or an autopkgtest in Debian to
>>> check if shadow's local copy needs an update.
>>>
>>> Depending on libbsd would pull the library into every single docker
>>> container image increasing their size and would make libbsd part of
>>> the pseudo-essential set, thus I prefer not depending on it for a few
>>> lines of code.
>>
>> libbsd0 is only ~ 200 kB (installed size).  That should be
>> insignificant compared to a Debian docker image, or even to the
>> shadow packages.
>>
>> libsubid4 is ~ 300 kB
>> uidmap is~ 300 kB
>> login is ~ 2.6 MB
>> passwd is~ 2.8 kB
>>
>> And the unstable-slim Debian Docker image is around 28 MB
>> (compressed size).
> 
> Yes, and libsubid4 and uidmap are not present in the docker images.
> 
>>
>> Moreover, having this libbsd part of the pseudo-essential set would
>> allow many other packages to rely on it, thus deduplicating the
>> copies that some projects currently do to avoid depending on it,
>> so the total distribution size could even shrink in the long term.
> 
> Developers of Debian are expected to be very conservative regarding
> expanding the (pseudo-) essential set:
> https://www.debian.org/doc/debian-policy/ch-binary.html#essential-packages
> 
> I value keeping the essential set minimal above providing one more
> shared library for potential reverse dependencies, too.
> I'd like to hear more people's opinion from the shadow project and if
> the project insists on adding the libbsd dependency I will bring the
> topic to debian-devel following the spirit of the Debian Policy
> offering to either carry a copy of readpassphrase.c as a patch in the
> Debian package or adding the libbsd dependency.

I've CCd Guillem to know his opinion too.

IMO, the functionallity provided by libbsd is essential; so much that
I think glibc should pick it.  However, now that libbsd has it, it's
not so important to add it to glibc, but then libbsd has to have a
status similar to libc.

We've fixed many bugs in shadow with the help of libbsd, and I think
many projects would benefit from having it available.

But of course, that needs agreement of libbsd's maintainer (Guillem),
and the debian-devel team.  Let's see what they and the shadow
maintainers think.

Cheers,
Alex

-- 

GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


OpenPGP_signature
Description: OpenPGP digital signature
___
Pkg-shadow-devel mailing list
Pkg-shadow-devel@alioth-lists.debian.net
https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel

[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Bálint Réczey]

Hi Alejandro,

Alejandro Colomar  ezt írta (időpont: 2023.
márc. 12., V, 16:52):
>
> Hi Bálint,
>
> On 3/12/23 16:38, Bálint Réczey wrote:
> >> 142 lines of a function definition are not something I'd consider easy to
> >> maintain.  Is it a big deal to add another dependency?  I'd say it's a
> >> bigger deal to copy verbatim so many lines of code, and sync them from
> >> time to time from libbsd (or OpenBSD) just to bring in any bugfixes they
> >> apply.  That's exactly the purpose of libbsd, so I think relying on them
> >> should be fine.
> >
> > The function does not change often. It changed two times in the last 13 
> > years:
> > https://gitlab.freedesktop.org/libbsd/libbsd/-/commits/main/src/readpassphrase.c
> >
> > I'd be happy to add a GitHub Action job or an autopkgtest in Debian to
> > check if shadow's local copy needs an update.
> >
> > Depending on libbsd would pull the library into every single docker
> > container image increasing their size and would make libbsd part of
> > the pseudo-essential set, thus I prefer not depending on it for a few
> > lines of code.
>
> libbsd0 is only ~ 200 kB (installed size).  That should be
> insignificant compared to a Debian docker image, or even to the
> shadow packages.
>
> libsubid4 is ~ 300 kB
> uidmap is~ 300 kB
> login is ~ 2.6 MB
> passwd is~ 2.8 kB
>
> And the unstable-slim Debian Docker image is around 28 MB
> (compressed size).

Yes, and libsubid4 and uidmap are not present in the docker images.

>
> Moreover, having this libbsd part of the pseudo-essential set would
> allow many other packages to rely on it, thus deduplicating the
> copies that some projects currently do to avoid depending on it,
> so the total distribution size could even shrink in the long term.

Developers of Debian are expected to be very conservative regarding
expanding the (pseudo-) essential set:
https://www.debian.org/doc/debian-policy/ch-binary.html#essential-packages

I value keeping the essential set minimal above providing one more
shared library for potential reverse dependencies, too.
I'd like to hear more people's opinion from the shadow project and if
the project insists on adding the libbsd dependency I will bring the
topic to debian-devel following the spirit of the Debian Policy
offering to either carry a copy of readpassphrase.c as a patch in the
Debian package or adding the libbsd dependency.

Cheers,
Balint

> Cheers,
>
> Alex
>
> --
> 
> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

___
Pkg-shadow-devel mailing list
Pkg-shadow-devel@alioth-lists.debian.net
https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel

[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Alejandro Colomar]

On 3/12/23 16:52, Alejandro Colomar wrote:

> libsubid4 is ~ 300 kB
> uidmap is~ 300 kB
> login is ~ 2.6 MB
> passwd is~ 2.8 kB

I meant 2.8 MB :)
> > 

-- 

GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


OpenPGP_signature
Description: OpenPGP digital signature
___
Pkg-shadow-devel mailing list
Pkg-shadow-devel@alioth-lists.debian.net
https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel

[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Alejandro Colomar]

Hi Bálint,

On 3/12/23 16:38, Bálint Réczey wrote:
>> 142 lines of a function definition are not something I'd consider easy to
>> maintain.  Is it a big deal to add another dependency?  I'd say it's a
>> bigger deal to copy verbatim so many lines of code, and sync them from
>> time to time from libbsd (or OpenBSD) just to bring in any bugfixes they
>> apply.  That's exactly the purpose of libbsd, so I think relying on them
>> should be fine.
> 
> The function does not change often. It changed two times in the last 13 years:
> https://gitlab.freedesktop.org/libbsd/libbsd/-/commits/main/src/readpassphrase.c
> 
> I'd be happy to add a GitHub Action job or an autopkgtest in Debian to
> check if shadow's local copy needs an update.
> 
> Depending on libbsd would pull the library into every single docker
> container image increasing their size and would make libbsd part of
> the pseudo-essential set, thus I prefer not depending on it for a few
> lines of code.

libbsd0 is only ~ 200 kB (installed size).  That should be
insignificant compared to a Debian docker image, or even to the
shadow packages.

libsubid4 is ~ 300 kB
uidmap is~ 300 kB
login is ~ 2.6 MB
passwd is~ 2.8 kB

And the unstable-slim Debian Docker image is around 28 MB
(compressed size).

Moreover, having this libbsd part of the pseudo-essential set would
allow many other packages to rely on it, thus deduplicating the
copies that some projects currently do to avoid depending on it,
so the total distribution size could even shrink in the long term.

Cheers,

Alex

-- 

GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


OpenPGP_signature
Description: OpenPGP digital signature
___
Pkg-shadow-devel mailing list
Pkg-shadow-devel@alioth-lists.debian.net
https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel

[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Bálint Réczey]

Hi Alejandro,

Alejandro Colomar  ezt írta (időpont: 2023.
márc. 11., Szo, 1:08):
>
> Hi Bálint,
>
> On 3/10/23 21:34, Bálint Réczey wrote:
> [...]
>
> >> I didn't have the time to look into that, but we should really
> >> check if we need to add some error checking.  With strlcpy(3),
> >> at least we can do it, contrary to strncpy(3), which doesn't
> >> really help detecting truncation (except that you can check
> >> the last byte _before_ overwriting it with the '\0', which is
> >> really cumbersome).
> >
> > I did not find setting the last '\0' that cumbersome,
>
> It's not just setting '\0', but also checking truncation.  As I
> said, strncpy(3) is not suited for that, but memcpy(3) could be
> used.  However, using memcpy(3) for copying strings with truncation
> and detecting truncation is:
>
> memcpy(dst, src, sizeof(dst) - 1)
> if (strlen(src) >= sizeof(dst))
> goto trunc;
> dst[sizeof(dst) - 1] = '\0';
>
> There are a few things I don't like:
>
> -  setting '\0' is in a separate line.  Just a minor thing.
> -  Two '-1', which are likely to produce off-by-one errors
>at some point (I've already fixed a few of them, IIRC).  At
>least they didn't seem bad, since we had then on the good
>side (we were just wasting one byte).
>
> But the behavior is indeed what we want.  Here's the definition of
> stpecpy(), which basically does that (I call strnlen(3) for optimizing):
>
> $ grepc -tfd stpecpy
> ./lib/stpecpy.h:67:
> inline char *
> stpecpy(char *dst, char *end, const char *restrict src)
> {
> booltrunc;
> char*p;
> size_t  dsize, dlen, slen;
>
> if (dst == end)
> return end;
> if (dst == NULL)
> return NULL;
>
> dsize = end - dst;
> slen = strnlen(src, dsize);
> trunc = (slen == dsize);
> dlen = slen - trunc;
>
> p = mempcpy(dst, src, dlen);
> *p = '\0';
>
> return p + trunc;
> }
>
>
> > but I'd be OK
> > with any implementation that's correct and uses only glibc symbols
> > including strcpy() and memcpy().
>
> Okay, stpecpy() would be enough.
>
> >> But we can't trivially replace readpassphrase(3bsd).  We could try
> >> to reimplement it ourselves, but I don't see avoiding libbsd-dev
> >> as a strong-enough reason.
> >
> > Indeed, readpassphrase() is the most problematic, but looking at its
> > implementation in libbsd it could be just copied to shadow. I'm not a
> > fan of such copies, but it seems this function has been copied
> > extensively already:
> > https://codesearch.debian.net/search?q=readpassphrase%28const+char=1
>
> I'm not a fan either; rather the opposite.  I'd vote against doing so.
>
> >
> > readpassphrase.c's ISC license allows that, too, and I think copying
> > would not be a ton of work.
>
> Copying it, probably not.  Maintaining it, maybe yes.  I mean, just look
> at it:
>
> $ grepc -tfd readpassphrase | wc -l
> 142
>
>
> 142 lines of a function definition are not something I'd consider easy to
> maintain.  Is it a big deal to add another dependency?  I'd say it's a
> bigger deal to copy verbatim so many lines of code, and sync them from
> time to time from libbsd (or OpenBSD) just to bring in any bugfixes they
> apply.  That's exactly the purpose of libbsd, so I think relying on them
> should be fine.

The function does not change often. It changed two times in the last 13 years:
https://gitlab.freedesktop.org/libbsd/libbsd/-/commits/main/src/readpassphrase.c

I'd be happy to add a GitHub Action job or an autopkgtest in Debian to
check if shadow's local copy needs an update.

Depending on libbsd would pull the library into every single docker
container image increasing their size and would make libbsd part of
the pseudo-essential set, thus I prefer not depending on it for a few
lines of code.

Cheers,
Balint


> Cheers,
>
> Alex
> --
> 
> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

___
Pkg-shadow-devel mailing list
Pkg-shadow-devel@alioth-lists.debian.net
https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel

[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Alejandro Colomar]

On 3/12/23 13:54, Alejandro Colomar wrote:
> Hi Paul,
> 
> On 3/12/23 02:44, Paul Eggert wrote:
>> On 2023-03-11 14:02, Alejandro Colomar wrote:
>>> we should use "%s" (if we go the way of snprintf(3)).
>>
>> Yes, thanks for catching that. However, I came up with a better way that 
>> avoids snprintf (and strlcpy) entirely both here and the other place I 
>> used snprintf.
>>
>> Attached is a revised set of patches that addresses the comments you 
>> made and embodies my followups.
> 
> I've applied patches 1, 2, 3, and 4 to my repo (I'll take care of them,
> and forward them to the maintainers).  You can rebase to
> 

I've pushed a signed tag paul1, so you can safely check that the
repo is mine (since I don't have HTTPS).



$ git show paul1
tag paul1
Tagger: Alejandro Colomar 
Date:   Sun Mar 12 16:24:54 2023 +0100

First round of patches applied from Paul Eggert.
-BEGIN PGP SIGNATURE-

iQIzBAABCgAdFiEE6jqH8KTroDDkXfJAnowa+77/2zIFAmQN7tsACgkQnowa+77/
2zJUNg/+OTaAFD276I8DHo/HvOtwPuKWiwcB7u0Yp4gdzkcbU/AGZ5S0xpdqbsIP
IWgiR402NIPv5zfQPA/v5vg0UNHxK6pQx2vhIUWsm0awG1m+N0+4VifXFbWo+LoY
bwxocWnEQJS8fIzK6YKi1CMw5++LKtx/orygZF/tfEQIVSTey/AktjaQY6OM4Yrb
U4HN9w0M5PELuyUl5v92XGSmSPxUDR5b3UdJ88Pz4wW1mnSOl4DpDdmFhVjzRkQp
1MBdB2OM4l5oCxSWPguYTyRYF2qOdVbZhxujPx0ob8uOzV78+plEas5gYYRxtIwo
ymcPLvet7qXfr5JE0I8vIt8AIXBRdyvdWydyaZOQecwCr5gqW0XTmo84cVzcTJ4q
4wqpJ4N9MODiolDYhDr97CN2D9DY0jeKvFj1vTdy6lC8nxWR9E55TuFXur7AEXTJ
TP9IxBK/7IoR7D9MidSmyq9zuHcDr3lkMEvHWLDfzYoFsI5fNeumyW52ylOUs7CV
u6BpWQpX/bPvUR+p1KuEPnP/nAzDpjDdlWWmd6cdTvu1H1JJCyFwjO7GVhts0JLF
LkxFiwHF8XmEz6g5HOYL16LpuClUtIAOEP9YXCpaiF1jROHjkErFm2YjEbtQMSZD
u5Ea8or9RVFQTWgbRHS4+SfmN8292gR9QxmU6Xhxmz3CldiqypM=
=aDa2
-END PGP SIGNATURE-

commit 67be4a8f2585c8af423e358efc4934291880a900 (HEAD -> paul, tag: paul1, 
alx/paul)

Cheers,

Alex

> 
> For the rest of the patches, I'll send some comments.
> 
> Thanks!
> 
> Alex
> 

-- 

GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


OpenPGP_signature
Description: OpenPGP digital signature
___
Pkg-shadow-devel mailing list
Pkg-shadow-devel@alioth-lists.debian.net
https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel

[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Alejandro Colomar]

Hi Paul,

On 3/12/23 02:44, Paul Eggert wrote:
> From 9ebf228fb33f66d248b230d23b633800267e5a16 Mon Sep 17 00:00:00 2001
> From: Paul Eggert 
> Date: Sat, 11 Mar 2023 10:34:21 -0800
> Subject: [PATCH 8/8] Fix su silent truncation
> 
> * src/su.c (check_perms): Do not silently truncate user name.
> 
> Signed-off-by: Paul Eggert 
> ---
>  src/su.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/src/su.c b/src/su.c
> index 9c134a9b..112be456 100644
> --- a/src/su.c
> +++ b/src/su.c
> @@ -658,7 +658,14 @@ static /*@only@*/struct passwd * check_perms (void)
>   SYSLOG ((LOG_INFO,
>"Change user from '%s' to '%s' as requested by PAM",
>name, tmp_name));
> - strlcpy (name, tmp_name, sizeof(name));
> + if (sizeof name <= strnlen (tmp_name, sizeof name)) {

strnlen(3)'s output is limited by the second argument, which makes
the previous condition to always be false, AFAICS.  I guess you
wanted to call strlen(3)?  Which BTW we can, since we use "%s" with
tmp_name in the previous line, so we know it's a string (or we
would have already crashed --or worse--).

Cheers,

Alex

> + fprintf (stderr, _("Overlong user name '%s'\n"),
> +  tmp_name);
> + SYSLOG ((LOG_NOTICE, "Overlong user name '%s'",
> +  tmp_name));
> + su_failure (caller_tty, true);
> + }
> + strcpy (name, tmp_name);
>   pw = xgetpwnam (name);
>   if (NULL == pw) {
>   (void) fprintf (stderr,
> @@ -1213,4 +1220,3 @@ int main (int argc, char **argv)
>  
>   return (errno == ENOENT ? E_CMD_NOTFOUND : E_CMD_NOEXEC);
>  }
> -
> -- 
> 2.37.2
> 


-- 

GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


OpenPGP_signature
Description: OpenPGP digital signature
___
Pkg-shadow-devel mailing list
Pkg-shadow-devel@alioth-lists.debian.net
https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel

[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Alejandro Colomar]

Hi Paul,

On 3/12/23 02:44, Paul Eggert wrote:
> From fab3bcdcb3f38c7f6f5c326f4ceafb3ea54bba73 Mon Sep 17 00:00:00 2001
> From: Paul Eggert 
> Date: Sat, 11 Mar 2023 10:07:32 -0800
> Subject: [PATCH 7/8] Fix is_my_tty overruns and truncations

Is there any chance those can be fixed individually?  The patch
is rather long, and complex to review.  Maybe it's not possible
and all fixes depend on each other; if so, please confirm.  But
if it's possible, I'd like to review it split.

> 
> * libmisc/utmp.c (is_my_tty): Declare the parameter
> as a char array not char *, as it is not necessarily null-terminated.
> Avoid a read overrun when reading ut_utname.  Do not silently truncate
> the string returned by ttyname; instead, do not cache an overlong
> ttyname, as the behavior is correct in this case (albeit slower).
> Use strnlen + strcpy instead of strlcpy to avoid polluting the
> cache with truncated data.
> 
> Signed-off-by: Paul Eggert 
> ---
>  libmisc/utmp.c | 42 ++
>  1 file changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/libmisc/utmp.c b/libmisc/utmp.c
> index ff6acee0..bf7e5675 100644
> --- a/libmisc/utmp.c
> +++ b/libmisc/utmp.c
> @@ -24,36 +24,38 @@
>  
>  #ident "$Id$"
>  
> +enum { UT_LINE_LEN = sizeof (getutent ()->ut_line) };

Isn't UT_LINESIZE (from , see utmp(5)) what you want?

alx@asus5775:~/src/linux/man-pages/man-pages/main$ grepc -tt -x. utmp man5
man5/utmp.5:72:
struct utmp {
short   ut_type;  /* Type of record */
pid_t   ut_pid;   /* PID of login process */
charut_line[UT_LINESIZE]; /* Device name of tty \- "/dev/" */
charut_id[4]; /* Terminal name suffix,
 or inittab(5) ID */
charut_user[UT_NAMESIZE]; /* Username */
charut_host[UT_HOSTSIZE]; /* Hostname for remote login, or
 kernel version for run\-level
 messages */
struct  exit_status ut_exit;  /* Exit status of a process
 marked as DEAD_PROCESS; not
 used by Linux init(1) */
/* The ut_session and ut_tv fields must be the same size when
   compiled 32\- and 64\-bit.  This allows data files and shared
   memory to be shared between 32\- and 64\-bit applications. */
#if __WORDSIZE == 64 && defined __WORDSIZE_COMPAT32
int32_t ut_session;   /* Session ID (\fBgetsid\fP(2)),
 used for windowing */
struct {
int32_t tv_sec;   /* Seconds */
int32_t tv_usec;  /* Microseconds */
} ut_tv;  /* Time entry was made */
#else
 long   ut_session;   /* Session ID */
 struct timeval ut_tv;/* Time entry was made */
#endif

int32_t ut_addr_v6[4];/* Internet address of remote
 host; IPv4 address uses
 just ut_addr_v6[0] */
char __unused[20];/* Reserved for future use */
};


>  
>  /*
>   * is_my_tty -- determine if "tty" is the same TTY stdin is using
>   */
> -static bool is_my_tty (const char *tty)
> +static bool is_my_tty (const char tty[UT_LINE_LEN])
>  {
> - /* full_tty shall be at least sizeof utmp.ut_line + 5 */
> - char full_tty[200];
> - /* tmptty shall be bigger than full_tty */
> - static char tmptty[sizeof (full_tty)+1];
> -
> - if ('/' != *tty) {
> - (void) snprintf (full_tty, sizeof full_tty, "/dev/%s", tty);
> - tty = _tty[0];
> - }
> + /* A null-terminated copy of tty, prefixed with "/dev/" if tty
> +is not absolute.  There is no need for snprintf, as sprintf
> +cannot overrun.  */
> + char full_tty[sizeof "/dev/" + UT_LINE_LEN];
> + (void) sprintf (full_tty, "%s%.*s", '/' == *tty ? "" : "/dev/",
> + UT_LINE_LEN, tty);

To be honest, I still prefer stpcpy(3).  Avoiding one call is
probably not even an optimization, since sprintf(3) internals
will counter any gains, right?

I think the following reads simpler, and hopefully it's even
faster:

p = full_tty;
*p = '\0';
if (tty[0] == '/')
p = stpcpy(p, "/dev/");
strncat(p, tty, UT_LINESIZE);

After writing, since shadow isn't performance-critical, and
32 bytes will probably not take too much to catenate, I
realized we could just use strcpy(3) instead of stpcpy(3),
to simplify even more:

full_tty[0] = '\0';
if (tty[0] == '/')
strcpy(full_tty, "/dev/");
strncat(full_tty, tty, UT_LINESIZE);

>  
> - if ('\0' == tmptty[0]) {
> - const char *tname = ttyname (STDIN_FILENO);
> - if (NULL != tname)
> - (void) strlcpy (tmptty, tname, sizeof tmptty);
> - }
> + /* Cache of ttyname, valid if tmptty[0].  */
> + static char tmptty[UT_LINE_LEN + 1];
>  
> + const char *tname;
>   if 

[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Alejandro Colomar]

Hi Paul,

On 3/12/23 02:44, Paul Eggert wrote:
> From f3514f26297e884a00d4fb29191bd9978eb03e7b Mon Sep 17 00:00:00 2001
> From: Paul Eggert 
> Date: Sat, 11 Mar 2023 00:42:29 -0800
> Subject: [PATCH 6/8] Fix crash with large timestamps
> 
> * libmisc/date_to_str.c (date_to_str): Do not crash if gmtime
> returns NULL because the timestamp is far in the future.
> Instead, use a dummy struct tm * to pacify any pedantic runtime.
> Simplify by always calling strftime, instead of sometimes strftime
> and sometimes strlcpy.
> 
> Signed-off-by: Paul Eggert 
> ---
>  libmisc/date_to_str.c | 19 +++
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/libmisc/date_to_str.c b/libmisc/date_to_str.c
> index f3b9dc76..0840c38c 100644
> --- a/libmisc/date_to_str.c
> +++ b/libmisc/date_to_str.c
> @@ -35,13 +35,16 @@
>  
>  void date_to_str (size_t size, char buf[size], long date)
>  {
> - time_t t;
> + time_t t = date;
> + struct tm *tm = gmtime ();
>  
> - t = date;
> - if (date < 0) {
> - (void) strlcpy (buf, "never", size);
> - } else {
> - (void) strftime (buf, size, "%Y-%m-%d", gmtime ());
> - buf[size - 1] = '\0';
> - }

Please split the patch into two: one that fixes the UB,
and another that removes the strlcpy(3) calls.  They
can be done independently, and I'm not convinced by your
measurement of simplicity :)


On 3/12/23 02:38, Paul Eggert wrote:
> It depends on how one measures simplicity. The reader will need to know 
> strftime's API regardless; requiring the reader to also know strlcpy's 
> API makes the reader's job harder.

I expect readers to easily learn that from the relevant man page :)
Once you know what strlcpy(3) is, that is, a function that copies a
string with truncation, it's straightforward to read.  Knowing
strftime(3)'s details is more obscure for those who are not time
nerds :-)

> 
> Also, it's less machine code to call just the one function (if one cares 
> about simplicity of debugging .

Heh, I could count with my fingers the number of days I've used
gdb(1) in my entire life :D.  Hail fprintf(3) and stderr!  Which
BTW benefit from expanded conditionals, rather than ternary ops.

> 
> If you still prefer calling two different functions instead of just one, 
> feel free to modify it to use plain strcpy. strlcpy isn't needed here as 
> the destination buffers are all big enough.

Do we know that?  The API receives a size as a parameter, which
could perfectly be 1 (well, I know we're not going to do that,
but since it's a generic API, I wouldn't rely on current sizes).

Cheers,

Alex

> To be honest though I like 
> it the way it is (though it could use a comment; I'll add that).

> + /* A dummy whose address can be passed to strftime to avoid
> +undefined behavior.  It's OK for it to be uninitialized
> +since no conversion specs are used.  */
> + struct tm dummy;
> +
> + (void) strftime (buf, size,
> +  date < 0 ? "never" : tm ? "%Y-%m-%d" : "future",
> +  tm ? tm : );
> + buf[size - 1] = '\0';
>  }
> -- 
> 2.37.2
> 


-- 

GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


OpenPGP_signature
Description: OpenPGP digital signature
___
Pkg-shadow-devel mailing list
Pkg-shadow-devel@alioth-lists.debian.net
https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel

[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Alejandro Colomar]

Hi Paul,

On 3/12/23 02:44, Paul Eggert wrote:
> From 54fac7560f87a134c4d3045ce7048f4819c4e492 Mon Sep 17 00:00:00 2001
> From: Paul Eggert 
> Date: Sat, 11 Mar 2023 00:38:24 -0800
> Subject: [PATCH 5/8] Avoid silent truncation of console file data
> 
> * libmisc/console.c (is_listed): Rework so that there is no
> fixed-size buffer, and no need to use fgets or strlcpy or strtok.
> Instead, the code works with arbitrary-sized input,
> without silently truncating data or mishandling NUL
> bytes in the console file.
> 
> Signed-off-by: Paul Eggert 
> ---
>  libmisc/console.c | 41 -
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/libmisc/console.c b/libmisc/console.c
> index 7e2132dd..8264e1a3 100644
> --- a/libmisc/console.c
> +++ b/libmisc/console.c
> @@ -24,7 +24,6 @@
>  static bool is_listed (const char *cfgin, const char *tty, bool def)
>  {
>   FILE *fp;
> - char buf[1024], *s;
>   const char *cons;
>  
>   /*
> @@ -43,17 +42,17 @@ static bool is_listed (const char *cfgin, const char 
> *tty, bool def)
>*/
>  
>   if (*cons != '/') {
> - char *pbuf;
> - strlcpy (buf, cons, sizeof (buf));
> - pbuf = [0];
> - while ((s = strtok (pbuf, ":")) != NULL) {
> - if (strcmp (s, tty) == 0) {
> + size_t ttylen = strlen (tty);

Please separate the initialization from the declaration, and
leave a blank line in between.

> + for (;;) {
> + if (strncmp (cons, tty, ttylen) == 0
> + && (cons[ttylen] == ':' || !cons[ttylen])) {
>   return true;
>   }
> -
> - pbuf = NULL;
> + cons = strchr (cons, ':');
> + if (!cons)
> + return false;
> + cons++;
>   }
> - return false;
>   }
>  
>   /*
> @@ -70,21 +69,22 @@ static bool is_listed (const char *cfgin, const char 
> *tty, bool def)
>* See if this tty is listed in the console file.
>*/
>  
> - while (fgets (buf, sizeof (buf), fp) != NULL) {
> - /* Remove optional trailing '\n'. */
> - buf[strcspn (buf, "\n")] = '\0';
> - if (strcmp (buf, tty) == 0) {
> - (void) fclose (fp);
> - return true;
> + const char *tp = tty;
> + bool listed = false;

Please -Wdeclaration-after-statement.  If the declaration is
so far that it helps mix declarations with code, it may be the
time to split something into a helper function...

Cheers,

Alex

> + for (int c; 0 <= (c = getc (fp)); ) {
> + if (c == '\n') {
> + if (tp && !*tp) {
> + listed = true;
> + break;
> + }
> + tp = tty;
> + } else if (tp) {
> + tp = *tp == c && c ? tp + 1 : NULL;
>   }
>   }
>  
> - /*
> -  * This tty isn't a console.
> -  */
> -
>   (void) fclose (fp);
> - return false;
> + return listed;
>  }
>  
>  /*
> @@ -105,4 +105,3 @@ bool console (const char *tty)
>  
>   return is_listed ("CONSOLE", tty, true);
>  }
> -
> -- 
> 2.37.2
> 


-- 

GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


OpenPGP_signature
Description: OpenPGP digital signature
___
Pkg-shadow-devel mailing list
Pkg-shadow-devel@alioth-lists.debian.net
https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel

[Pkg-shadow-devel] Bug#1032393: Bug#1032393: [PATCH v2 2/2] debian/control: Add libbsd-dev and pkg-config

[Alejandro Colomar]

Hi Paul,

On 3/12/23 02:44, Paul Eggert wrote:
> On 2023-03-11 14:02, Alejandro Colomar wrote:
>> we should use "%s" (if we go the way of snprintf(3)).
> 
> Yes, thanks for catching that. However, I came up with a better way that 
> avoids snprintf (and strlcpy) entirely both here and the other place I 
> used snprintf.
> 
> Attached is a revised set of patches that addresses the comments you 
> made and embodies my followups.

I've applied patches 1, 2, 3, and 4 to my repo (I'll take care of them,
and forward them to the maintainers).  You can rebase to


For the rest of the patches, I'll send some comments.

Thanks!

Alex

-- 

GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


OpenPGP_signature
Description: OpenPGP digital signature
___
Pkg-shadow-devel mailing list
Pkg-shadow-devel@alioth-lists.debian.net
https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel