[U-Boot] [PATCH v6 3/3] GPT: provide commands to selectively rename partitions

[Lothar Waßmann]

Hi,

On Tue, 27 Jun 2017 09:05:14 +0200 Lothar Waßmann wrote:
> Hi,
> 
> On Sun, 25 Jun 2017 14:54:56 -0700 Alison Chaiken wrote:
> > On Sun, Jun 18, 2017 at 4:03 AM, Wolfgang Denk <wd at denx.de> wrote:
> > 
> > > Dear Alison,
> > >
> > > In message <CAOuSAjdHerD7iWSwv5HQmx07nALRHschnH5=XToNEZDqA9JsvQ at mail.
> > > gmail.com> you wrote:
> > > >
> > > > The idea behind the 'swap' mode is that a storage device can have two
> > > sets
> > > > of partitions, one set all named 'primary' and one set all named
> > > 'backup'.
> > > >   The software updater in userspace can then simply rename the partitions
> > > > with sgdisk in order to pick the new image.   The swap mode changes the
> > > > whole set of labels at once, so there's little chance of being
> > > interrupted.
> > >
> > > It's still a sequential, non-atomic operation, and "little chance"
> > > is exactly the places where Murphy likes to hit you.
> > >
> > > > One additional note: the last version I posted worked fine for the
> > > sandbox,
> > > > but wouldn't link for an ARM target with the Linaro toolchain, as the
> > > > linker couldn't find atoi().   I guess the libc for the x86 compiler
> > > > includes it.   To test on ARM, I copied in simple_atoi() from
> > > > lib/vsprintf.c, but assuredly that is an ugly solution.    Does anyone
> > > have
> > > > a better idea to solve this problem?
> > >
> > > U-Boot should always be self-contained and not link regular library
> > > code from the tool chain.
> > >
> > > Best regards,
> > >
> > > Wolfgang Denk
> > >
> > 
> > I'm about to submit a new version of the patches that adopts Wolfgang's and
> > Tom's suggestions about replacing atoi().
> > 
> > Regarding the atomicity of 'gpt swap, the point is that 'gpt swap' first
> > modifies the names in an in-memory
> > data structure, and then uses the existing 'gpt write' functionality to
> > change the actual partition table stored on the device.  Thus,
> > interruption of the new command is low-risk, as interruption of the
> > modification of the new data structure has no persistent effect, and
> > the risk associated with 'gpt write' is the same as always.
> > 
> > By the way, in the course of testing an earlier version of this patch
> > series, I noticed that 'gpt write' and 'gpt verify' segv if presented with
> > a non-null-terminated partitions string.  It's the strlen function in lib
> > that actually generates an error. I haven't yet quite figured out what the
> > best solution to the problem is: should strlen() itself be modified, or is
> > it enough to test in gpt.c?   The right solution is not to present the
> > commands with poorly formed strings, but it's easy to do so.
> > 
> You can use strnlen() if you know the maximum allowed length of the
> string.
> 
> NB: A quick glance at set_gpt_info() revealed this potential crash
> cause:
> |	str = strdup(str_part);
> |
> |	/* extract disk guid */
> |	s = str;
> |	val = extract_val(str, "uuid_disk");
> strdup() may fail (especially if the input string is not zero
> terminated) and return a NULL pointer which then will happily be used
> by extract_val().
> 

There are some more highlights in this code:
|		*str_disk_guid = malloc(UUID_STR_LEN + 1);
|		gen_rand_uuid_str(*str_disk_guid, UUID_STR_FORMAT_STD);
>
malloc() can fail too.


|		*str_disk_guid = strdup(p);
|		free(val);
|		/* Move s to first partition */
|		strsep(&s, ";");
>
if strdup() fails, *str_disk_guid will be a NULL pointer, but the
function will return success eventually and the NULL pointer will
be passed on to subsequent functions without further checks.


|static bool found_key(const char *str, const char *key)
|{
|	char *k;
|	char *s, *strcopy;
|	bool result = false;
|
|	strcopy = strdup(str);
|	if (!strcopy)
|		return NULL;
>
The function has a bool return type, but returns a pointer type here.
This accidentally works as expected.



Lothar Waßmann