Re: [PATCH] password: Fix warning with empty default password

[David Dgien <dgienda125@gmail.com>]

Hello

On Wed, May 20, 2020 at 08:26:32AM +0200, Uwe Kleine-König wrote:
> On Tue, May 19, 2020 at 11:55:55PM -0400, David Dgien wrote:
> > When CONFIG_PASSWORD_DEFAULT is unset, the default_passwd buffer is set
> 
> I assume you mean "If CONFIG_PASSWORD_DEFAULT is set to an empty
> string".

Yes.

> 
> > to the empty string. The read_default_passwd() function wants to read at
> > least two characters from that buffer, causing GCC to generate an array
> > bounds warning.
> 
> I cannot reproduce that warning. Which gcc version do you use and on
> which platform? Mentioning the exact warning in the commit log helps
> finding the resulting commit when searching for a fix.

arm-none-eabi-gcc --version prints "arm-none-eabi-gcc (Arch Repository)
10.1.0"
I found the issue when building for rpi_defconfig and
vexpress_defconfig.

The warning I get when building from master (commit c10b20dc83ac):

barebox/common/password.c: In function 'login':
barebox/common/password.c:173:5: warning: array subscript [1, 2147483647] is outside array bounds of 'const char[1]' [-Warray-bounds]
  173 |   c = buf[i];
      |   ~~^~~~~~~~
In file included from barebox/common/password.c:30:
include/generated/passwd.h:1:19: note: while referencing 'default_passwd'
    1 | static const char default_passwd[] = "";
      |                   ^~~~~~~~~~~~~~

I guess the compiler doesn't know that strlen(default_passwd) = 0, just
that length > 0 so the most it can assume is that the loop has to
consume at least two chars, and the empty string only contains one.

> 
> > Make the default_passwd buffer have at least 2 bytes so
> > this warning is not generated.
> > 
> > Since the read_default_passwd() function is only called when
> > default_passwd is not the empty string, this is not a functional change.
> 
> I don't understand the problem for the empty password. With
> default_passwd = "" we have strlen(default_passwd) = 0 so the for loop
> doesn't run at all.

Yes, that's correct, which is one reason why this is not functionally
different. But the compiler doesn't seem to be smart enough to know
that.

> 
> As I understand the code (at commit c10b20dc83ac) for uneven lengths of
> default_passwd the last accessed byte is the trailing '\0' and for even
> length it's the byte before the trailing '\0'. This should be ok?!
> 
> Am I missing something?

When working on this reply, I realized there was another solution I
missed when I was trying to find ways to short-circut the compiler
previously. If I add:

if (ARRAY_SIZE(default_passwd) == 1)
	return -ENOSYS;

in the read_default_passwd() function, that would short-circut the
compiler preventing the warning message, and is less hacky. I can
resubmit with that change instead.

> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Thanks,
David Dgien

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox