[U-Boot] [PATCH 3/8] sandbox: Use memcpy() to move overlapping regions

[Simon Glass]

Hi Heinrich,

On 9 June 2018 at 10:56, Heinrich Schuchardt <xypron.glpk at 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 <sjg at 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);
>>
>