[U-Boot-Users] Hush and Autoscript Bug

[U-Boot-Users] Hush and Autoscript Bug

[Daniel Hobi]

Hello,

The following code snippet which was added by the U-Boot team to 
busybox' hush shell seems a bit strange. From u-boot-1.1.5/common/hush.c:

int parse_string_outer(char *s, int flag)
{
        struct in_str input;
#ifdef __U_BOOT__
        char *p = NULL;
        int rcode;
        if ( !s || !*s)
                return 1;
        if (!(p = strchr(s, '\n')) || *++p) {
                p = xmalloc(strlen(s) + 2);
                strcpy(p, s);
                strcat(p, "\n");
                setup_string_in_str(&input, p);
                rcode = parse_stream_outer(&input, flag);
                free(p);
                return rcode;
        } else {
#endif
        setup_string_in_str(&input, s);
        return parse_stream_outer(&input, flag);
#ifdef __U_BOOT__
        }
#endif
}

It looks like the second if() clause was added to make sure that the 
passed string s is terminated by \n\0. However, this has some flaws:

 1) If the string contains a \n which is not followed by a \0, the code 
tries to fix it by adding a \0. But strlen, strcpy and strcat work on 
zero-terminated strings. As it is, the code replaces the first \0 in (or 
beyond) the string with \n\0.

 2) If 1) would be fixed, the function could not process strings 
containing more than one \n anymore (because a \0 would be added after 
the first \n).

 3) If the string contains no \n (but is zero-terminated), the code 
inserts a \n before the \0. This messes up the return value of 
parse_stream_outer which scans for commands (everything which is 
terminated by either \n or \0), but returns the exit status of the last 
command only. If string s is terminated by \n\0, the last command is 
empty and parse_stream_outer always returns 0.

Problem 3) is the cause why autoscripts (both the function and the 
command) always return an exit value of 0.

In my opinion, the above check should be removed completely because the 
parameter string s should always be zero-terminated (all functions 
calling parse_string_outer already seem to ensure this). And, if you 
want the function to process the first command (terminated by \n or \0) 
only, the parameter flag should have bit FLAG_EXIT_FROM_LOOP set (like 
the commands 'boot' and 'run' do, which may be considered another bug..).

What do you think? Does anyone know the author of the above code snippet?

Best Regards,
Daniel