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

[Guillem Jover]

On Sun, 2023-03-12 at 21:53:14 +0100, Alejandro Colomar wrote:
> On 3/12/23 20:22, Bálint Réczey wrote:
> > Alejandro Colomar ezt írta (időpont: 2023. márc. 12., V, 16:52):
> >> 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).

The sid-slim image I've got here uses around 82 MiB installed.

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

Thanks. Because there's been no users before, I've never considered
such addition. And for packages where I'm upstream in the essential
set, I've always preferred to use portable interfaces to depending on
libbsd myself.

But then, the main reason to create libbsd, was precisely to try to
reduce code duplication and have a single place to store and maintain
these interfaces. I strongly believe that we should in general try to
reduce embedded code and vendoring, which are a technical detriment
to the distribution (and the software ecosystem in general).

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

It is already picking up parts of it. But there are parts in it that I
don't see glibc adopting, so I'm afraid there will always be something
that keeps libbsd alive.

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

dpkg started depending on libmd recently (which libbsd depends on),
also because I didn't feel like keeping embedding yet another MD5
implementation, and to make it possible to start using SHA variants in
the future in the code base.

On the cons, I'm perhaps missing something, but I don't see any other
obvious candidates in the essential-set that I could see starting to
use libbsd, but OTOH we have other libraries depended on by only a
single source package so this does not seem like a huge issue.

On the pros, libbsd is rather small in size; adding a shared library
to the essential-set should be a non-issue, as that's an implementation
detail, and does not increase the exposed interface for eternity, as
once a package there stops using the shared library it simply gets
evicted from the essential-set. If the shadow upstream and Debian
maintainers do not see an issue with depending on libbsd, then
personally I have no issues with it being added to the essential-set
if debian-devel sees no issue with it.

Also, another option in the future could be to remove login from the

[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

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

[Paul Eggert]

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.From 324bb0e914b5470650df02bd7b64e963665d44c1 Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Sat, 11 Mar 2023 00:01:02 -0800
Subject: [PATCH 1/8] Simplify change_field by using strcpy

* lib/fields.c (change_field): Since we know the string fits,
use strcpy rather than strlcpy.

Signed-off-by: Paul Eggert 
---
 lib/fields.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/fields.c b/lib/fields.c
index 0b5f91b2..8801bce6 100644
--- a/lib/fields.c
+++ b/lib/fields.c
@@ -100,7 +100,6 @@ void change_field (char *buf, size_t maxsize, const char *prompt)
 			cp++;
 		}
 
-		strlcpy (buf, cp, maxsize);
+		strcpy (buf, cp);
 	}
 }
-
-- 
2.37.2

From b13ffb86dcd10e8160eee10bd286fc73937c3e8b Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Sat, 11 Mar 2023 13:41:54 -0800
Subject: [PATCH 2/8] Omit unneeded change_field test

* fields.c (change_field): Omit unnecessary test.

Signed-off-by: Paul Eggert 
---
 lib/fields.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/fields.c b/lib/fields.c
index 8801bce6..fa5fd156 100644
--- a/lib/fields.c
+++ b/lib/fields.c
@@ -96,7 +96,7 @@ void change_field (char *buf, size_t maxsize, const char *prompt)
 		*cp = '\0';
 
 		cp = newf;
-		while (('\0' != *cp) && isspace (*cp)) {
+		while (isspace (*cp)) {
 			cp++;
 		}
 
-- 
2.37.2

From 090722a20765cf9a248050524143fce5b68cfe8c Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Sat, 11 Mar 2023 13:43:36 -0800
Subject: [PATCH 3/8] Fix change_field buffer underrun

* lib/fields.c (change_field): Don't point
before array start; that has undefined behavior.

Signed-off-by: Paul Eggert 
---
 lib/fields.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/fields.c b/lib/fields.c
index fa5fd156..640be931 100644
--- a/lib/fields.c
+++ b/lib/fields.c
@@ -91,8 +91,9 @@ void change_field (char *buf, size_t maxsize, const char *prompt)
 		 * entering a space.  --marekm
 		 */
 
-		while (--cp >= newf && isspace (*cp));
-		cp++;
+		while (newf < cp && isspace (cp[-1])) {
+			cp--;
+		}
 		*cp = '\0';
 
 		cp = newf;
-- 
2.37.2

From 4982d5f2fe2f2c339568996ebb17a99200d2f106 Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Sat, 11 Mar 2023 00:02:45 -0800
Subject: [PATCH 4/8] Prefer strcpy to strlcpy when either works

* lib/gshadow.c (sgetsgent): Use strcpy not strlcpy,
since the string is known to fit.

Signed-off-by: Paul Eggert 
---
 lib/gshadow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/gshadow.c b/lib/gshadow.c
index c17af67f..ca14449a 100644
--- a/lib/gshadow.c
+++ b/lib/gshadow.c
@@ -128,7 +128,7 @@ void endsgent (void)
 		sgrbuflen = len;
 	}
 
-	strlcpy (sgrbuf, string, len);
+	strcpy (sgrbuf, string);
 
 	cp = strrchr (sgrbuf, '\n');
 	if (NULL != cp) {
-- 
2.37.2

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);
+		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 = 

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

[Paul Eggert]

On 2023-03-11 14:39, Alejandro Colomar wrote:

I wonder
if the patch is really "simplifying".


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.


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


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. To be honest though I like 
it the way it is (though it could use a comment; I'll add that).


___
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-11 13:49, Alejandro Colomar wrote:

+: mempcpy (full_tty, "/dev/", sizeof"/dev/" - 1)),

This is a great use case for stpcpy(3).


I came up with a slightly better approach, that needs neither mempcpy 
nor stpcpy. I plan to send it along soon.


___
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/11/23 23:38, Paul Eggert wrote:
> On 2023-03-11 13:59, Alejandro Colomar wrote:
>> If the function is allowed
>> to dereference, then NULL is not allowed, but if the values are
>> uninitialized, then reading any of them should also trigger UB, no?
> 
> Sure, but the standard says that strftime reads only the struct tm 
> members needed to interpret the format. If the format contains no 
> conversion specs, strftime reads no struct tm members and thus there is 
> no UB even if the struct tm is entirely uninitialized.

Yeah, my point then is that if you don't read members, you don't even
need to dereference.  However, I see that glibc takes a copy, which is
a reason to dereference without reading any values.  So, yes, you're
right ;)

-- 

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]

On 3/11/23 23:34, Paul Eggert wrote:
> On 2023-03-11 14:18, Alejandro Colomar wrote:
> 
>> What I'm not sure is that strftime(3) requires nonnull.
> 
> glibc's strftime implementation segfaults if you pass a null pointer, so 
> we can't pass NULL regardless of whether the strftime API in time.h uses 
> __attribute__ ((nonnull))'.

Hmm, please fix the API :-)

So, yes, then that dummy seems to be necessary.  However, then I wonder
if the patch is really "simplifying".  I think strlcpy(3) was simpler.

-- 

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-11 13:59, Alejandro Colomar wrote:

If the function is allowed
to dereference, then NULL is not allowed, but if the values are
uninitialized, then reading any of them should also trigger UB, no?


Sure, but the standard says that strftime reads only the struct tm 
members needed to interpret the format. If the format contains no 
conversion specs, strftime reads no struct tm members and thus there is 
no UB even if the struct tm is entirely uninitialized.


___
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-11 14:18, Alejandro Colomar wrote:


What I'm not sure is that strftime(3) requires nonnull.


glibc's strftime implementation segfaults if you pass a null pointer, so 
we can't pass NULL regardless of whether the strftime API in time.h uses 
__attribute__ ((nonnull))'.


___
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/11/23 23:08, Paul Eggert wrote:
> On 2023-03-11 13:59, Alejandro Colomar wrote:
>> Unless the standard specifically allows us to do so, but I can't find
>> anything clear.
> 
> It's pretty clear if you're a time nerd like me. :-)

:-)

> The standard for 
> strftime says "The appropriate characters are determined using the 
> LC_TIME category of the current locale and by the values of zero or more 
> members of the broken-down time structure pointed to by timeptr, as 
> specified in brackets in the description. If any of the specified values 
> are outside the normal range, the characters stored are unspecified."
> 
> The "zero" means that if no conversion specs are present in the format 
> string, then no struct tm members are examined, and it's therefore OK 
> for all members to be uninitialized if no conversion specs are present.

Hmmm, might make sense.  I'm thinking of the following code:

int foo(bool x, int *_Nonnull i)
{
if (!x)
return *i;

return 42;
}

Some compiler might decide to read-ahead the contents of *i knowing that
it can't be NULL.  If x is true, then it just discards that value.  Since
the compiler is allowed to perform UB if it knows that it can't affect
observable behavior, it should be fine.  If you pass NULL, then it would
crash.

What I'm not sure is that strftime(3) requires nonnull.  I didn't find it
in the C17 text.  glibc doesn't seem to use nonnull attributes for it:

$ grepc strftime /usr/include/
/usr/include/time.h:100:
extern size_t strftime (char *__restrict __s, size_t __maxsize,
const char *__restrict __format,
const struct tm *__restrict __tp) __THROW;


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

[Alejandro Colomar]

Hi Paul,

On 3/11/23 20:29, Paul Eggert wrote:
> From 522b2db5619bd26631bd444d208768f740c2fdba Mon Sep 17 00:00:00 2001
> From: Paul Eggert 
> Date: Sat, 11 Mar 2023 10:34:21 -0800
> Subject: [PATCH 6/6] Fix su silent truncation
> 
> * src/su.c (check_perms): Do not silently truncate user name.
> Use snprintf instead of strlcpy as the latter doesn't buy much here
> and this avoids depending on strlcpy.
> 
> 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..740d31f9 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));
> + int tmp_namelen = snprintf (name, sizeof name, tmp_name);

This will likely trigger a warning about using a variable for the format
string.  Are you sure it's can't have conversion specifiers?  Otherwise,
we should use "%s" (if we go the way of snprintf(3)).

But I suggest adding error using strlcpy(3), since it reads much simpler,
and adding error checking to it.  Anyway, we can't stop depending on
libbsd until we find a solution for readpassphrase(3bsd).

Cheers,

Alex

> + if (! (0 <= tmp_namelen && tmp_namelen < sizeof name)) {
> + fprintf (stderr, _("Overlong user name '%s'\n"),
> +  tmp_name);
> + SYSLOG ((LOG_NOTICE, "Overlong user name '%s'",
> +  tmp_name));
> + su_failure (caller_tty, true);
> + }
>   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]

On 3/11/23 22:52, Paul Eggert wrote:
> On 2023-03-11 13:31, Alejandro Colomar wrote:
>> What's this  exactly for?
> 
> It avoids undefined behavior. A call like strftime (buf, sizeof buf, 
> "XXX", NULL) has undefined behavior, as near as I can make out.

Ahh, sure, it makes sense.  Didn't consider that.

> It's OK 
> that the dummy is uninitialized.

It's not so trivial to arrive to this conclusion.  If the function is
not allowed to dereference the pointer if the fmt string doesn't
require it, then NULL should be allowed.  If the function is allowed
to dereference, then NULL is not allowed, but if the values are
uninitialized, then reading any of them should also trigger UB, no?

Unless the standard specifically allows us to do so, but I can't find
anything clear.  Maybe it would be safer to bzero(3) it.

What do you think?


-- 

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-11 13:31, Alejandro Colomar wrote:

What's this  exactly for?


It avoids undefined behavior. A call like strftime (buf, sizeof buf, 
"XXX", NULL) has undefined behavior, as near as I can make out. It's OK 
that the dummy is uninitialized.


___
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/11/23 20:29, Paul Eggert wrote:
> From 70985857d6d24262fc57a10bd62e6dbc642dda70 Mon Sep 17 00:00:00 2001
> From: Paul Eggert 
> Date: Sat, 11 Mar 2023 10:07:32 -0800
> Subject: [PATCH 5/6] Fix is_my_tty overruns and truncations
> 
> * libmisc/utmp.c: Include mempcpy.h.
> (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 snprintf instead of strlcpy as the latter doesn't buy much here
> and this avoids depending on strlcpy.
> 
> Signed-off-by: Paul Eggert 
> ---
>  libmisc/utmp.c | 50 --
>  1 file changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/libmisc/utmp.c b/libmisc/utmp.c
> index ff6acee0..9d40470e 100644
> --- a/libmisc/utmp.c
> +++ b/libmisc/utmp.c
> @@ -21,39 +21,45 @@
>  #include 
>  
>  #include "alloc.h"
> +#include "mempcpy.h"
>  
>  #ident "$Id$"
>  
> +enum { UT_LINE_LEN = sizeof (getutent ()->ut_line) };
>  
>  /*
>   * 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];
> - }
> -
> - if ('\0' == tmptty[0]) {
> - const char *tname = ttyname (STDIN_FILENO);
> - if (NULL != tname)
> - (void) strlcpy (tmptty, tname, sizeof tmptty);
> - }
> -
> + /* 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 (('/' == *tty
> +  ? full_tty

I think it might be easier to read if we conditionally call stpcpy(3),
and then a simple sprintf(3) catenated to it.

> +  : mempcpy (full_tty, "/dev/", sizeof "/dev/" - 1)),

This is a great use case for stpcpy(3).  It's in POSIX.1-2008, which is
a base requirement for shadow since recently, so we can use it.

Cheers,

Alex

> + "%.*s", UT_LINE_LEN, tty);
> +
> + /* Cache of ttyname, valid if tmptty[0].  */
> + static char tmptty[UT_LINE_LEN + 1];
> +
> + const char *tname;
>   if ('\0' == tmptty[0]) {
> - (void) puts (_("Unable to determine your tty name."));
> - exit (EXIT_FAILURE);
> - } else if (strncmp (tty, tmptty, sizeof (tmptty)) != 0) {
> - return false;
> + tname = ttyname (STDIN_FILENO);
> + if (! tname) {
> + (void) puts (_("Unable to determine your tty name."));
> + exit (EXIT_FAILURE);
> + }
> + int tnamelen = snprintf (tmptty, sizeof tmptty, "%s", tname);
> + if (! (0 <= tnamelen && tnamelen < sizeof tmptty)) {
> + tmptty[0] = '\0';
> + }
>   } else {
> - return true;
> + tname = tmptty;
>   }
> +
> + return strcmp (full_tty, tname) == 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/11/23 20:29, Paul Eggert wrote:
> From 1c8388d1d1831e976cdaa6e6f27bb08bf31aedc5 Mon Sep 17 00:00:00 2001
> From: Paul Eggert 
> Date: Sat, 11 Mar 2023 00:42:29 -0800
> Subject: [PATCH 4/6] 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.

Thanks :)

> 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 | 15 +++
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/libmisc/date_to_str.c b/libmisc/date_to_str.c
> index f3b9dc76..4b3a4f48 100644
> --- a/libmisc/date_to_str.c
> +++ b/libmisc/date_to_str.c
> @@ -35,13 +35,12 @@
>  
>  void date_to_str (size_t size, char buf[size], long date)
>  {
> - time_t t;
> + time_t t = date;
> + struct tm *tm = gmtime ();
> + struct tm dummy;
>  
> - t = date;
> - if (date < 0) {
> - (void) strlcpy (buf, "never", size);
> - } else {
> - (void) strftime (buf, size, "%Y-%m-%d", gmtime ());
> - buf[size - 1] = '\0';
> - }
> + (void) strftime (buf, size,
> +  date < 0 ? "never" : tm ? "%Y-%m-%d" : "future",
> +  tm ? tm : );

What's this  exactly for?

Cheers,

Alex

> + 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/11/23 20:29, Paul Eggert wrote:
> From 7e88c5914c1fab6c4d88e1ca39d6b6319e7ee768 Mon Sep 17 00:00:00 2001
> From: Paul Eggert 
> Date: Sat, 11 Mar 2023 00:02:45 -0800
> Subject: [PATCH 2/6] Prefer memcpy to strlcpy when either works
> 
> memcpy is standardized and should be faster here.
> * lib/gshadow.c (sgetsgent): Use memcpy not strlcpy,
> since the string is known to fit.
> 
> Signed-off-by: Paul Eggert 
> ---
>  lib/gshadow.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/gshadow.c b/lib/gshadow.c
> index c17af67f..1976c9a9 100644
> --- a/lib/gshadow.c
> +++ b/lib/gshadow.c
> @@ -128,7 +128,7 @@ void endsgent (void)
>   sgrbuflen = len;
>   }
>  
> - strlcpy (sgrbuf, string, len);
> + memcpy (sgrbuf, string, len);

While this one is less of a concern, and memcpy(3) is faster than
strcpy(3), I think I'd also use strcpy(3) here.  Also, the 'len'
variable seems confusing, since it's really a size.

Cheers,
Alex

>  
>   cp = strrchr (sgrbuf, '\n');
>   if (NULL != cp) {
> -- 
> 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/11/23 20:29, Paul Eggert wrote:
> From d40e2f92f3e50d13d87393bd30b2b4b20b89a2d6 Mon Sep 17 00:00:00 2001
> From: Paul Eggert 
> Date: Sat, 11 Mar 2023 00:01:02 -0800
> Subject: [PATCH 1/6] Fix undefined behavior in change_field
> 
> * lib/fields.c (change_field): Do not ever compute [-1],
> as behavior is undefined.  Since we know that the string fits,
> use memcpy rather than strlcpy.

I'd separate the UB fix, from the transformation to memcpy(3),
in two separate commits, since they are conceptually unrelated.

> 
> Signed-off-by: Paul Eggert 
> ---
>  lib/fields.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/fields.c b/lib/fields.c
> index 0b5f91b2..3b119502 100644
> --- a/lib/fields.c
> +++ b/lib/fields.c
> @@ -90,17 +90,17 @@ void change_field (char *buf, size_t maxsize, const char 
> *prompt)
>* makes it possible to change the field to empty, by
>* entering a space.  --marekm
>*/
> + char *bp = newf;
>  
> - while (--cp >= newf && isspace (*cp));
> - cp++;
> + while (newf < cp && isspace (cp[-1])) {
> + cp--;
> + }
>   *cp = '\0';
>  
> - cp = newf;
> - while (('\0' != *cp) && isspace (*cp)) {
> - cp++;
> + while (isspace (*bp)) {
> + bp++;
>   }
>  
> - strlcpy (buf, cp, maxsize);
> + memcpy (buf, bp, cp + 1 - bp);

Regarding this transformation, I'd prefer transforming to strcpy(3).
It avoids the manual `cp + 1 - bp` calculation.

Thanks for the review and patches!

Cheers,
Alex

>   }
>  }
> -
> -- 
> 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

[Paul Eggert]

I looked into this, and five of the shadow package's six uses of strlcpy 
are wrong, i.e., they are associated with silent truncation or buffer 
overrun/underrun or dereferencing NULL in nearby code. This isn't 
surprising, as strlcpy is commonly used in code that has been 
slapdashedly hacked to try to make it safer, and in my experience code 
that that has been modified in that way is usually wrong.


Proposed patches attached.

Although there is one correct use of strlcpy, the correct use (in 
sgetsgent) is equivalent to memcpy so there is no need for strlcpy there 
(see patch 0002).
From d40e2f92f3e50d13d87393bd30b2b4b20b89a2d6 Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Sat, 11 Mar 2023 00:01:02 -0800
Subject: [PATCH 1/6] Fix undefined behavior in change_field

* lib/fields.c (change_field): Do not ever compute [-1],
as behavior is undefined.  Since we know that the string fits,
use memcpy rather than strlcpy.

Signed-off-by: Paul Eggert 
---
 lib/fields.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/fields.c b/lib/fields.c
index 0b5f91b2..3b119502 100644
--- a/lib/fields.c
+++ b/lib/fields.c
@@ -90,17 +90,17 @@ void change_field (char *buf, size_t maxsize, const char *prompt)
 		 * makes it possible to change the field to empty, by
 		 * entering a space.  --marekm
 		 */
+		char *bp = newf;
 
-		while (--cp >= newf && isspace (*cp));
-		cp++;
+		while (newf < cp && isspace (cp[-1])) {
+			cp--;
+		}
 		*cp = '\0';
 
-		cp = newf;
-		while (('\0' != *cp) && isspace (*cp)) {
-			cp++;
+		while (isspace (*bp)) {
+			bp++;
 		}
 
-		strlcpy (buf, cp, maxsize);
+		memcpy (buf, bp, cp + 1 - bp);
 	}
 }
-
-- 
2.37.2

From 7e88c5914c1fab6c4d88e1ca39d6b6319e7ee768 Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Sat, 11 Mar 2023 00:02:45 -0800
Subject: [PATCH 2/6] Prefer memcpy to strlcpy when either works

memcpy is standardized and should be faster here.
* lib/gshadow.c (sgetsgent): Use memcpy not strlcpy,
since the string is known to fit.

Signed-off-by: Paul Eggert 
---
 lib/gshadow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/gshadow.c b/lib/gshadow.c
index c17af67f..1976c9a9 100644
--- a/lib/gshadow.c
+++ b/lib/gshadow.c
@@ -128,7 +128,7 @@ void endsgent (void)
 		sgrbuflen = len;
 	}
 
-	strlcpy (sgrbuf, string, len);
+	memcpy (sgrbuf, string, len);
 
 	cp = strrchr (sgrbuf, '\n');
 	if (NULL != cp) {
-- 
2.37.2

From a1c2e046d52042cf60ff7196a9d9a972573290bd Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Sat, 11 Mar 2023 00:38:24 -0800
Subject: [PATCH 3/6] 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);
+		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;
+	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

From 1c8388d1d1831e976cdaa6e6f27bb08bf31aedc5 Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Sat, 11 Mar 2023 00:42:29 -0800
Subject: [PATCH 4/6] 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 

[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/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.

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. 8., Sze, 13:55):
>
> Hi Bálint,
>
> [I reordered some quotes for my reply]
> [CC Paul, since he's been mentioned, and I'm curious to know
> if he has any comments]
>
> On 3/8/23 11:59, Bálint Réczey wrote:
> > Hi Alejandro,
> >
> > Alejandro Colomar  ezt írta (időpont: 2023.
> > márc. 5., V, 20:44):
> >>
> >> Package: passwd
> >> Source: shadow
> >> Tags: patch
> >> X-Debbugs-CC: Bálint Réczey 
> >> X-Debbugs-CC: Iker Pedrosa 
> >> X-Debbugs-CC: Serge Hallyn 
> >>
> >> These dependencies were added upstream recently.
> >>
> >> Signed-off-by: Alejandro Colomar 
> >> Cc: Iker Pedrosa 
> >> Cc: Serge Hallyn 
> >> ---
> >>  debian/control | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/debian/control b/debian/control
> >> index 3cc66f8d..75015c35 100644
> >> --- a/debian/control
> >> +++ b/debian/control
> >> @@ -11,11 +11,13 @@ Build-Depends: bison,
> >> gettext,
> >> itstool,
> >> libaudit-dev [linux-any],
> >> +   libbsd-dev,
> >
> > I checked out recent changes in shadow's master and I'm very happy
> > about many of the fixes for memory allocation problems,
>
> Thanks! :-)
>
> > but wearing my maintainer hat I believe linking to a new library for a
> > few functions which are not very different from their glibc
> > counterpart is not worth it.
>
> We added it with strlcpy(3) in mind, but I agree with you that
> it's not a critical reason, and we could live without it; in fact
> I introduced a similar (and IMO superior) function, stpecpy(),
> which could replace strlcpy(3) in all 6 calls.
>
> But we didn't really add it for it; we already had the libbsd-dev
> dependency before adding strlcpy(3).  libbsd-dev was added for
> readpassphrase(3bsd), which has nothing similar in glibc, and I don't
> want to be rewriting it in terms of glibc stuff, since it's not
> trivial.
>
> It was added in this patch set:
>
> * 2a5b8810 - Mon, 21 Nov 2022 14:00:13 +0100 (4 months ago)
> |   agetpass: Hook into build-system - Guillem Jover
> * ab91ec10 - Wed, 28 Sep 2022 23:09:19 +0200 (5 months ago)
> |   Hide [[gnu::malloc(deallocator)]] in a macro - Alejandro Colomar
> * 554f86ba - Tue, 27 Sep 2022 21:21:35 +0200 (5 months ago)
> |   Replace the deprecated getpass(3) by our agetpass() - Alejandro 
> Colomar
> * 155c9421 - Mon, 26 Sep 2022 22:22:24 +0200 (5 months ago)
> |   libmisc: agetpass(), erase_pass(): Add functions for getting 
> passwords safely - Alex Colomar
> * 8cce4557 - Wed, 28 Sep 2022 00:03:46 +0200 (5 months ago)
> |   Don't 'else' after a 'noreturn' call - Alex Colomar
> * 99ce21a3 - Tue, 22 Nov 2022 14:35:06 +0100 (4 months ago)
> |   CI: add libbsd and pkg-config dependencies - Iker Pedrosa
>
> >
> > Freezero() also provides little extra benefit over memset() and free()
> > and is used only 4 times in the code.
>
> Use of freezero(3bsd) was added later to erase_pass() for one reason:
> that API pair --agetpass(), erase_pass()-- already strongly depends on a
> libbsd-dev function --readpassphrase(3bsd)--, so depending on two of them
> doesn't add any issues.  Anyway, freezero(3) is easy to implement if we
> had a need.
>
>
>
> > There are reasons for strlcpy() not being provided by glibc [1]:
> >
> > "Reactions among core glibc contributors on the topic of including
> > strlcpy() and strlcat() have been varied over the years. Christoph
> > Hellwig's early patch was rejected in the then-primary maintainer's
> > inimitable style (1 and 2). But reactions from other glibc developers
> > have been more nuanced, indicating, for example, some willingness to
> > accept the functions. Perhaps most insightfully, Paul Eggert notes
> > that even when these functions are provided (as an add-on packaged
> > with the application), projects such as OpenSSH, where security is of
> > paramount concern, still manage to either misuse the functions
> > (silently truncating data) or use them unnecessarily (i.e., the
> > traditional strcpy() and strcat() could equally have been used without
> > harm); such a state of affairs does not constitute a strong argument
> > for including the functions in glibc. "
> >
> > I agree with their position and the 6 cases where strlcpy() is used in
> > shadow's current master could be implemented with strncpy() as safely
> > as with strlcpy().
>
> I would strongly advise against that.  strncpy(3) doesn't produce
> strings.
>
> See the following manual pages:
>
> 
> 
>
> My main concerns with strncpy(3) are:
>
> -  It zeroes the rest of the buffer, which isn't bad per se, but
>then when reading code it's hard to tell if that was necessary
>or if it was just some inessential side effect of calling
>

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

[Alejandro Colomar]

Hi Bálint,

[I reordered some quotes for my reply]
[CC Paul, since he's been mentioned, and I'm curious to know
if he has any comments]

On 3/8/23 11:59, Bálint Réczey wrote:
> Hi Alejandro,
> 
> Alejandro Colomar  ezt írta (időpont: 2023.
> márc. 5., V, 20:44):
>>
>> Package: passwd
>> Source: shadow
>> Tags: patch
>> X-Debbugs-CC: Bálint Réczey 
>> X-Debbugs-CC: Iker Pedrosa 
>> X-Debbugs-CC: Serge Hallyn 
>>
>> These dependencies were added upstream recently.
>>
>> Signed-off-by: Alejandro Colomar 
>> Cc: Iker Pedrosa 
>> Cc: Serge Hallyn 
>> ---
>>  debian/control | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/debian/control b/debian/control
>> index 3cc66f8d..75015c35 100644
>> --- a/debian/control
>> +++ b/debian/control
>> @@ -11,11 +11,13 @@ Build-Depends: bison,
>> gettext,
>> itstool,
>> libaudit-dev [linux-any],
>> +   libbsd-dev,
> 
> I checked out recent changes in shadow's master and I'm very happy
> about many of the fixes for memory allocation problems,

Thanks! :-)

> but wearing my maintainer hat I believe linking to a new library for a
> few functions which are not very different from their glibc
> counterpart is not worth it.

We added it with strlcpy(3) in mind, but I agree with you that
it's not a critical reason, and we could live without it; in fact
I introduced a similar (and IMO superior) function, stpecpy(),
which could replace strlcpy(3) in all 6 calls.

But we didn't really add it for it; we already had the libbsd-dev
dependency before adding strlcpy(3).  libbsd-dev was added for
readpassphrase(3bsd), which has nothing similar in glibc, and I don't
want to be rewriting it in terms of glibc stuff, since it's not
trivial.

It was added in this patch set:

* 2a5b8810 - Mon, 21 Nov 2022 14:00:13 +0100 (4 months ago)
|   agetpass: Hook into build-system - Guillem Jover
* ab91ec10 - Wed, 28 Sep 2022 23:09:19 +0200 (5 months ago)
|   Hide [[gnu::malloc(deallocator)]] in a macro - Alejandro Colomar
* 554f86ba - Tue, 27 Sep 2022 21:21:35 +0200 (5 months ago)
|   Replace the deprecated getpass(3) by our agetpass() - Alejandro 
Colomar
* 155c9421 - Mon, 26 Sep 2022 22:22:24 +0200 (5 months ago)
|   libmisc: agetpass(), erase_pass(): Add functions for getting 
passwords safely - Alex Colomar
* 8cce4557 - Wed, 28 Sep 2022 00:03:46 +0200 (5 months ago)
|   Don't 'else' after a 'noreturn' call - Alex Colomar
* 99ce21a3 - Tue, 22 Nov 2022 14:35:06 +0100 (4 months ago)
|   CI: add libbsd and pkg-config dependencies - Iker Pedrosa


> 
> Freezero() also provides little extra benefit over memset() and free()
> and is used only 4 times in the code.

Use of freezero(3bsd) was added later to erase_pass() for one reason:
that API pair --agetpass(), erase_pass()-- already strongly depends on a
libbsd-dev function --readpassphrase(3bsd)--, so depending on two of them
doesn't add any issues.  Anyway, freezero(3) is easy to implement if we
had a need.



> There are reasons for strlcpy() not being provided by glibc [1]:
> 
> "Reactions among core glibc contributors on the topic of including
> strlcpy() and strlcat() have been varied over the years. Christoph
> Hellwig's early patch was rejected in the then-primary maintainer's
> inimitable style (1 and 2). But reactions from other glibc developers
> have been more nuanced, indicating, for example, some willingness to
> accept the functions. Perhaps most insightfully, Paul Eggert notes
> that even when these functions are provided (as an add-on packaged
> with the application), projects such as OpenSSH, where security is of
> paramount concern, still manage to either misuse the functions
> (silently truncating data) or use them unnecessarily (i.e., the
> traditional strcpy() and strcat() could equally have been used without
> harm); such a state of affairs does not constitute a strong argument
> for including the functions in glibc. "
> 
> I agree with their position and the 6 cases where strlcpy() is used in
> shadow's current master could be implemented with strncpy() as safely
> as with strlcpy().

I would strongly advise against that.  strncpy(3) doesn't produce
strings.

See the following manual pages:




My main concerns with strncpy(3) are:

-  It zeroes the rest of the buffer, which isn't bad per se, but
   then when reading code it's hard to tell if that was necessary
   or if it was just some inessential side effect of calling
   strncpy(3).  Which was exactly what happened when I did the
   migration from strncpy(3) to strlcpy(3): I had a hard time
   telling for sure at every call site if I could do it, or if
   I was doing something wrong by removing that zeroing.

-  It doesn't terminate the string, so you still need to manually
   

[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. 5., V, 20:44):
>
> Package: passwd
> Source: shadow
> Tags: patch
> X-Debbugs-CC: Bálint Réczey 
> X-Debbugs-CC: Iker Pedrosa 
> X-Debbugs-CC: Serge Hallyn 
>
> These dependencies were added upstream recently.
>
> Signed-off-by: Alejandro Colomar 
> Cc: Iker Pedrosa 
> Cc: Serge Hallyn 
> ---
>  debian/control | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/debian/control b/debian/control
> index 3cc66f8d..75015c35 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -11,11 +11,13 @@ Build-Depends: bison,
> gettext,
> itstool,
> libaudit-dev [linux-any],
> +   libbsd-dev,

I checked out recent changes in shadow's master and I'm very happy
about many of the fixes for memory allocation problems,
but wearing my maintainer hat I believe linking to a new library for a
few functions which are not very different from their glibc
counterpart is not worth it.
There are reasons for strlcpy() not being provided by glibc [1]:

"Reactions among core glibc contributors on the topic of including
strlcpy() and strlcat() have been varied over the years. Christoph
Hellwig's early patch was rejected in the then-primary maintainer's
inimitable style (1 and 2). But reactions from other glibc developers
have been more nuanced, indicating, for example, some willingness to
accept the functions. Perhaps most insightfully, Paul Eggert notes
that even when these functions are provided (as an add-on packaged
with the application), projects such as OpenSSH, where security is of
paramount concern, still manage to either misuse the functions
(silently truncating data) or use them unnecessarily (i.e., the
traditional strcpy() and strcat() could equally have been used without
harm); such a state of affairs does not constitute a strong argument
for including the functions in glibc. "

I agree with their position and the 6 cases where strlcpy() is used in
shadow's current master could be implemented with strncpy() as safely
as with strlcpy().

Freezero() also provides little extra benefit over memset() and free()
and is used only 4 times in the code.

Could you please return to using functions provided by glibc instead
of pulling in libbsd in the next upstream release?
That way there would be no need for pkg-config or pkgconf either.

Cheers,
Balint

[1] https://lwn.net/Articles/507319/

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