[PATCH] password: Fix warning with empty default password

[PATCH] password: Fix warning with empty default password

[David Dgien]

When CONFIG_PASSWORD_DEFAULT is unset, the default_passwd buffer is set
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. 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.

Signed-off-by: David Dgien <dgienda125@gmail.com>
---
 common/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/Makefile b/common/Makefile
index c14af692f..3b63e89ed 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -74,7 +74,7 @@ ifdef CONFIG_PASSWORD
 
 ifeq ($(CONFIG_PASSWORD_DEFAULT),"")
 define filechk_passwd
-	echo "static const char default_passwd[] = \"\";"
+	echo "static const char default_passwd[] = \"\\0\";"
 endef
 else
 define filechk_passwd
-- 
2.26.2


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

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

[Uwe Kleine-König]

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

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

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

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?

Best regards
Uwe

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

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

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

[David Dgien]

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