Hi Heinrich,
On 9 June 2018 at 10:56, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
> On 06/09/2018 08:22 PM, Simon Glass wrote:
>> The use of strcpy() to remove characters at the start of a string is safe
>> in U-Boot, since we know the implementation. But in os.c we are using the
>> C library's strcpy() function, where this behaviour is not permitted.
>>
>> Update the code to use memcpy() instead.
>>
>> Reported-by: Coverity (CID: 173279)
>>
>> Signed-off-by: Simon Glass <s...@chromium.org>
>> ---
>>
>> arch/sandbox/cpu/os.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
>> index 5839932b00..496a8f9bd8 100644
>> --- a/arch/sandbox/cpu/os.c
>> +++ b/arch/sandbox/cpu/os.c
>> @@ -587,7 +587,8 @@ int os_find_u_boot(char *fname, int maxlen)
>> /* Look for 'u-boot' in the parent directory of spl/ */
>> p = strstr(fname, "/spl/");
>
> I have built sandbox_spl_defconfig.
>
> First observation: any parallel build fails. But that is to be handled
> in a seperate patch.
I haven't seen that problem myself, but please send a patch or any
details you have.
>
> The logic to find u-boot here is broken. From the top directory of the
> git repository I executed:
>
> $ spl/u-boot-spl
>
> U-Boot SPL 2018.07-rc1-00063-g277daa5c0f (Jun 09 2018 - 20:40:23 +0200)
> Trying to boot from sandbox
> (spl/u-boot not found, error -2)
> SPL: failed to boot from all boot devices
> ### ERROR ### Please RESET the board ###
>
> Why would you expect fname to start with /spl/?
>
> In my case fname = 'spl/u-boot'. Why do you test for '/spl/' (only)?
>
> Just for the record if I execute
> $ ./spl/u-boot-spl
> everything works fine. But who would precede a directory name with './'?
It should be save enough to drop the leading / from the strstr(), if
that is what you are suggesting.
>
> Should we not concatenate the spl executable and the u-boot executable
> during the build process so that spl/u-boot knows where to find u-boot?
> That would be much safer than any assumption about relative paths.
We could do that, and there is support for doing this at a low level
(os_jump_to_image()). I've been working on some binman enhancements to
build a Chrome OS image, which would use that feature.
But executing from the filesystem seems reasonable to me, particularly
for running tests.
Regards,
Simon
>
> Best regards
>
> Heinrich
>
>> if (p) {
>> - strcpy(p, p + 4);
>> + /* Remove the "/spl" characters */
>> + memmove(p, p + 4, strlen(p + 4) + 1);
>> fd = os_open(fname, O_RDONLY);
>> if (fd >= 0) {
>> close(fd);
>>
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
