[U-Boot] fs/fs.c - error handling needed?

[U-Boot] fs/fs.c - error handling needed?

[Wolfgang Denk]

Dear Simon,

with commit a8f6ab5 "fs: Add support for saving data to filesystems"
you add the function do_save() to U-Boot.  This includes the following
code (line numbers as of current master):

"fs/fs.c":

...
331         filename = argv[3];
332         addr = simple_strtoul(argv[4], NULL, cmdline_base);
333         bytes = simple_strtoul(argv[5], NULL, cmdline_base);
334         if (argc >= 7)
335                 pos = simple_strtoul(argv[6], NULL, cmdline_base);
336         else
337                 pos = 0;


Should we not perform at least minimal error checking, i. e. verify
that no garbage arguments have been passed to that function?

Best regards,
Viele Gr??e,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The easiest way to figure the cost of living is to take  your  income
and add ten percent.

[U-Boot] fs/fs.c - error handling needed?

[Tom Rini]

On Sat, Oct 05, 2013 at 09:49:41PM +0200, Wolfgang Denk wrote:
> Dear Simon,
> 
> with commit a8f6ab5 "fs: Add support for saving data to filesystems"
> you add the function do_save() to U-Boot.  This includes the following
> code (line numbers as of current master):
> 
> "fs/fs.c":
> 
> ...
> 331         filename = argv[3];
> 332         addr = simple_strtoul(argv[4], NULL, cmdline_base);
> 333         bytes = simple_strtoul(argv[5], NULL, cmdline_base);
> 334         if (argc >= 7)
> 335                 pos = simple_strtoul(argv[6], NULL, cmdline_base);
> 336         else
> 337                 pos = 0;
> 
> 
> Should we not perform at least minimal error checking, i. e. verify
> that no garbage arguments have been passed to that function?

Yes, we ought to.  If you don't pass fatwrite the right number of
arguments we get data aborts, for example.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131007/4f2469a8/attachment.pgp>

[U-Boot] fs/fs.c - error handling needed?

[Wolfgang Denk]

Dear Tom,

In message <20131007121252.GS15917@bill-the-cat> you wrote:
> 
> > 331         filename = argv[3];
> > 332         addr = simple_strtoul(argv[4], NULL, cmdline_base);
> > 333         bytes = simple_strtoul(argv[5], NULL, cmdline_base);
> > 334         if (argc >= 7)
> > 335                 pos = simple_strtoul(argv[6], NULL, cmdline_base);
> > 336         else
> > 337                 pos = 0;
> > 
> > 
> > Should we not perform at least minimal error checking, i. e. verify
> > that no garbage arguments have been passed to that function?
>
> Yes, we ought to.  If you don't pass fatwrite the right number of
> arguments we get data aborts, for example.

Well, this is not a problem here, in do_save():

...
325         if (argc < 6 || argc > 7)
326                 return CMD_RET_USAGE;
...

And are you sure of "fatwrite"?  This calls do_fat_fswrite(). and here
I also see some test:

...
 98         if (argc < 5)
 99                 return cmd_usage(cmdtp);
...

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Men don't talk peace unless they're ready to back it up with war.
	-- Col. Green, "The Savage Curtain", stardate 5906.4

[U-Boot] fs/fs.c - error handling needed?

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/07/2013 09:55 AM, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20131007121252.GS15917@bill-the-cat> you wrote:
>> 
>>> 331         filename = argv[3]; 332         addr =
>>> simple_strtoul(argv[4], NULL, cmdline_base); 333         bytes
>>> = simple_strtoul(argv[5], NULL, cmdline_base); 334         if
>>> (argc >= 7) 335                 pos = simple_strtoul(argv[6],
>>> NULL, cmdline_base); 336         else 337                 pos =
>>> 0;
>>> 
>>> 
>>> Should we not perform at least minimal error checking, i. e.
>>> verify that no garbage arguments have been passed to that
>>> function?
>> 
>> Yes, we ought to.  If you don't pass fatwrite the right number
>> of arguments we get data aborts, for example.
> 
> Well, this is not a problem here, in do_save():
> 
> ... 325         if (argc < 6 || argc > 7) 326
> return CMD_RET_USAGE; ...
> 
> And are you sure of "fatwrite"?  This calls do_fat_fswrite(). and
> here I also see some test:
> 
> ... 98         if (argc < 5) 99                 return
> cmd_usage(cmdtp); ...

Off-by-one error somewhere? 'mmc' 'part' 'ddr-addr' 'filename' will
blowup as we use an insane value for size.  I suspect we aren't eating
'fatwrite' somewhere along the line.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSUsPdAAoJENk4IS6UOR1W6kEP/ijUEElKWLxsfJKLR1y0BbJ6
NaovYZ2Pz2UiOIEbk186hqEnpyaMU+2WlTavs61nufZ7fiteWkHS+536pgg/7GlG
pA9Uiq0DzI5RVG0EuaBJ62lt55JV9RWavdYlnGBEcgaD7ESLBTtizGbdKW30vW4S
FLfBXvEZVnYzTcIGT0kj8zEOLlg05QN6Asik8sv9GbwjX4wxVKwD0lpLYMmP/hy1
d/H6ql2a682HSAQw/6C+QxK89bSYbFay5+VhmFCZESM97tI1vWYIFC6hQsg8q+jl
/1asD1hquvv5Of7O2xIMyvmExPD25mbeflyTYnXoMFb8SL1VKNBfSdljcBPeTe8G
TBJMdURCf6zptmR+qUt8fRmWImhYZhalq+oyEImA4uKxu9zSBIBFcILKCJQ9LCXI
Hp0oaRaBUZVfD2Dno2g7fYgNwuOkXv/IpK/pWY0F6a2jNXGZooni9TdvkLeqX0sB
Bdgk1gz2EDeXIxvG/H4vTTWF5lVDh7Y5NHnqPW+HJ0fhaSqqxsklaXIU4np7EjIB
hfmwhi25fJZrGlUVgDG6YTkjTE8FOKfCaYVlFtIbrGZo14U27JucUBbLOeVxtEU0
VcdVcJx/pzi9377DbpWd8dF6XryMAMR3/de58ady6/TsoWZ90axIMykz3xE1TgAF
cppQkXNHHtao0PDxeWcV
=ntyL
-----END PGP SIGNATURE-----

[U-Boot] fs/fs.c - error handling needed?

[Simon Glass]

Hi Wolfgang,

On Sat, Oct 5, 2013 at 1:49 PM, Wolfgang Denk <wd@denx.de> wrote:

> Dear Simon,
>
> with commit a8f6ab5 "fs: Add support for saving data to filesystems"
> you add the function do_save() to U-Boot.  This includes the following
> code (line numbers as of current master):
>
> "fs/fs.c":
>
> ...
> 331         filename = argv[3];
> 332         addr = simple_strtoul(argv[4], NULL, cmdline_base);
> 333         bytes = simple_strtoul(argv[5], NULL, cmdline_base);
> 334         if (argc >= 7)
> 335                 pos = simple_strtoul(argv[6], NULL, cmdline_base);
> 336         else
> 337                 pos = 0;
>
>
> Should we not perform at least minimal error checking, i. e. verify
> that no garbage arguments have been passed to that function?
>

Do you mean passing an 'endp' parameter instead of NULL to simple_strtoul()
and checking that it processed at least one character? I compared it to
do_load() and it seems similar.

Regards,
Simon