[PATCH v4 10/14] net-lwip: add wget command

[Jerome Forissier]

On 6/25/24 05:20, Jon Humphreys wrote:
> Jerome Forissier <jerome.forissier at linaro.org> writes:
> 
>> On 6/24/24 08:21, Jon Humphreys wrote:
>>> Jerome Forissier <jerome.forissier at linaro.org> writes:
>>>
>>>> Add support for the wget command with NET_LWIP.
>>>>
>>>> Based on code initially developed by Maxim U.
>>>>
>>>> Signed-off-by: Jerome Forissier <jerome.forissier at linaro.org>
>>>> Co-developed-by: Maxim Uvarov <muvarov at gmail.com>
>>>> Cc: Maxim Uvarov <muvarov at gmail.com>
>>>> Signed-off-by: Jerome Forissier <jerome.forissier at linaro.org>
>>>> ---
>>>>  cmd/Kconfig        |   7 ++
>>>>  cmd/net-lwip.c     |   8 ++
>>>>  include/net-lwip.h |  18 +++
>>>>  net-lwip/Makefile  |   1 +
>>>>  net-lwip/wget.c    | 269 +++++++++++++++++++++++++++++++++++++++++++++
>>>>  5 files changed, 303 insertions(+)
>>>>  create mode 100644 net-lwip/wget.c
>>>>

<snip>

>>>> +/**
>>>> + * wget_validate_uri() - validate the uri for wget
>>>> + *
>>>> + * @uri:	uri string
>>>> + *
>>>> + * This function follows the current U-Boot wget implementation.
>>>> + * scheme: only "http:" is supported
>>>> + * authority:
>>>> + *   - user information: not supported
>>>> + *   - host: supported
>>>> + *   - port: not supported(always use the default port)
>>>> + *
>>>> + * Uri is expected to be correctly percent encoded.
>>>> + * This is the minimum check, control codes(0x1-0x19, 0x7F, except '\0')
>>>> + * and space character(0x20) are not allowed.
>>>> + *
>>>> + * TODO: stricter uri conformance check
>>>> + *
>>>> + * Return:	true on success, false on failure
>>>> + */
>>>> +bool wget_validate_uri(char *uri)
>>>> +{
>>>> +	char c;
>>>> +	bool ret = true;
>>>> +	char *str_copy, *s, *authority;
>>>> +
>>>> +	for (c = 0x1; c < 0x21; c++) {
>>>> +		if (strchr(uri, c)) {
>>>> +			log_err("invalid character is used\n");
>>>> +			return false;
>>>> +		}
>>>> +	}
>>>> +	if (strchr(uri, 0x7f)) {
>>>> +		log_err("invalid character is used\n");
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	if (strncmp(uri, "http://", 7)) {
>>>> +		log_err("only http:// is supported\n");
>>>> +		return false;
>>>> +	}
>>>> +	str_copy = strdup(uri);
>>>> +	if (!str_copy)
>>>> +		return false;
>>>> +
>>>> +	s = str_copy + strlen("http://");
>>>> +	authority = strsep(&s, "/");
>>>> +	if (!s) {
>>>> +		log_err("invalid uri, no file path\n");
>>>> +		ret = false;
>>>> +		goto out;
>>>> +	}
>>>> +	s = strchr(authority, '@');
>>>> +	if (s) {
>>>> +		log_err("user information is not supported\n");
>>>> +		ret = false;
>>>> +		goto out;
>>>> +	}
>>>> +	s = strchr(authority, ':');
>>>> +	if (s) {
>>>> +		log_err("user defined port is not supported\n");
>>>> +		ret = false;
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +out:
>>>> +	free(str_copy);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> -- 
>>>> 2.40.1
>>
>> Hi Jon,
>>
>>> Hi Jerome, I am trying out the lwIP stack on TI boards.
>>>
>>> For wget, I noticed a bug and a user experience improvement. The bug is an
>>> off-by-one array access when parsing the url.
>>
>> Indeed, someone reported this bug to me off-list and it's already fixed in
>> my WIP branch.
>>
>>> The improvement emits a
>>> message if the url isn't an http://, since that is all we are supporting,
>>> and without the message, the wget command silently fails, which is
>>> confusing to the user.
>>
>> I agree.
>>
>>> I will post the 2 patches as a reply to this email. LMK if you want to use
>>> another method to collaborate. I can already see that efi http boot support
>>> will not work. wget_validate_uri() looks to be copied vertabim from the
>>> current implementation, and IMO we should completely remove it.
>>
>> It is indeed copied. It is needed to avoid an unresolved symbol but I
>> understand more work may be needed for EFI HTTP boot support (not tested by
>> me).
>>
>> If you are willing to help with this then you are welcome to post patches as
>> replies and I will integrate them in the series.
>>
>> Thanks,
>> -- 
>> Jerome
> 
> Jerome, here is a patch to allow a port specifier in wget's uri, which lwIP
> supports.
> 
> thanks
> Jon
> 
> Subject: [PATCH] net-lwip: lwIP wget supports user defined port in the uri, so
>  allow it.
> 
> Signed-off-by: Jonathan Humphreys <j-humphreys at ti.com>
> ---
>  net-lwip/wget.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/net-lwip/wget.c b/net-lwip/wget.c
> index 6b9c953ef51..d3fdd44a29e 100644
> --- a/net-lwip/wget.c
> +++ b/net-lwip/wget.c
> @@ -257,12 +257,6 @@ bool wget_validate_uri(char *uri)
>  		ret = false;
>  		goto out;
>  	}
> -	s = strchr(authority, ':');
> -	if (s) {
> -		log_err("user defined port is not supported\n");
> -		ret = false;
> -		goto out;
> -	}
>  
>  out:
>  	free(str_copy);

Patch added to v5, thanks!

-- 
Jerome