Hi Alejandro,
Alejandro Colomar <alx.manpa...@gmail.com> 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)
> {
> bool trunc;
> 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&literal=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
> --
> <http://www.alejandro-colomar.es/>
> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
