[U-Boot] fs/fs.c - error handling needed?
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
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.
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
-----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
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
participants (3)
-
Simon Glass -
Tom Rini -
Wolfgang Denk