Re: [PATCH 08/10] env: Use strncpy() instead of ad-hoc code to copy variable value

["Marek Behún" <kabel@kernel.org>]

On Tue, 12 Oct 2021 13:41:43 +0200
Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:

> On 12/10/2021 13.04, Marek Behún wrote:
> > From: Marek Behún <marek.behun@nic.cz>
> > 
> > Copy the value of the found variable into given buffer with
> > strncpy().
> > 
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  cmd/nvedit.c | 15 +++++----------
> >  1 file changed, 5 insertions(+), 10 deletions(-)
> > 
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index 412918a000..51b9e4ffa4 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -746,18 +746,13 @@ int env_get_f(const char *name, char *buf,
> > unsigned len) continue;
> >  
> >  		/* found; copy out */
> > -		for (n = 0; n < len; ++n, ++buf) {
> > -			*buf = env[val++];
> > -			if (*buf == '\0')
> > -				return n;
> > +		n = strncpy(buf, &env[val], len) - buf;  
> 
> strncpy by definition returns the dst argument, so this is, apart from
> the side effects done by strncpy, a complicated way of saying "n =
> 0;".

You are right, this is a mistake.

> > +		if (len && n == len) {  
> 
> which then makes this automatically false always.
> 
> I understand why you want to avoid an open-coded copying, but strncpy
> is almost certainly the wrong tool for the job. Apart from not
> revealing anything about the actual length of the source string, it
> also has the well-known defect of zeroing the tail of the buffer in
> the (usual) case where the source is shorter than the buffer.

U-Boot's strncpy does not do that:
 * Note that unlike userspace strncpy, this does not %NUL-pad the
buffer.
 * However, the result is not %NUL-terminated if the source exceeds
 * @count bytes.

But you are right that I should use strlcpy here.

Thanks.

> n = strlcpy(buf, &env[val], len);
> if (n >= len) ...
> 
> is probably closer to what you want. But only if you can trust
> &env[val] to actually have a nul byte before going into lala land.
> 
> Rasmus