Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 19 Jun 2015 17:09:45 -0600
From:      Ian Lepore <ian@freebsd.org>
To:        Maxim Sobolev <sobomax@FreeBSD.org>
Cc:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   Re: svn commit: r284614 - head/sys/boot/uboot/lib
Message-ID:  <1434755385.1415.114.camel@freebsd.org>
In-Reply-To: <201506192224.t5JMOxpC097306@svn.freebsd.org>
References:  <201506192224.t5JMOxpC097306@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 2015-06-19 at 22:24 +0000, Maxim Sobolev wrote:
> Author: sobomax
> Date: Fri Jun 19 22:24:58 2015
> New Revision: 284614
> URL: https://svnweb.freebsd.org/changeset/base/284614
> 
> Log:
>   Provide bug4bug workaround for certain dumbiness of the u-boot's API_env_enum
>   function, which is expected to set returned env to NULL upon reaching the end
>   of the environment list but fails to do so in certain cases. The respective
>   u-boot code looks like the following (HEAD at the time of this commit):
>   
>   --- api.c ---
>    496 static int API_env_enum(va_list ap)
>    ...
>    510                 *next = last;
>    511
>    512                 for (i = 0; env_get_char(i) != '\0'; i = n + 1) {
>    513                         for (n = i; env_get_char(n) != '\0'; ++n) {
>    514                                 if (n >= CONFIG_ENV_SIZE) {
>    515                                         /* XXX shouldn't we set *next = NULL?? */
>    516                                         return 0;
>    517                                 }
>    518                         }
>   -------------
>   
>   The net result is that any unfortunate user of the loader's ub_env_enum()
>   function hitting this condition would be trapped in the infinite loop, as
>   the main use pattern of ub_env_enum() is basically the following:
>   
>   while ((env = ub_env_enum(env)) != NULL) { DO STUFF }
>   
>   Which would stuck forever with the last element.
> 
> Modified:
>   head/sys/boot/uboot/lib/glue.c
> 
> Modified: head/sys/boot/uboot/lib/glue.c
> ==============================================================================
> --- head/sys/boot/uboot/lib/glue.c	Fri Jun 19 21:55:12 2015	(r284613)
> +++ head/sys/boot/uboot/lib/glue.c	Fri Jun 19 22:24:58 2015	(r284614)
> @@ -513,7 +513,7 @@ ub_env_enum(const char *last)
>  	if (!syscall(API_ENV_ENUM, NULL, (uint32_t)last, (uint32_t)&env))
>  		return (NULL);
>  
> -	if (env == NULL)
> +	if (env == NULL || last == env)
>  		/* no more env. variables to enumerate */
>  		return (NULL);
>  
> 

This is only a problem with an unpatched u-boot, which has a completely
bogus and un-useful implementation of API_env_enum().  That's why every
one of our u-boot ports has the same patch to put in an implementation
that actually works.

Your change works around the worst part of the bug (the infinite loop)
but leaves the major problem of the implementation only returning values
initially loaded from the saved environment, not anything set by the
scripts that loaded ubldr().

-- Ian




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1434755385.1415.114.camel>