[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)
{
	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.

Cheers,

Alex
-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://alioth-lists.debian.net/pipermail/pkg-shadow-devel/attachments/20230311/7a0cefcd/attachment.sig>