[OpenIndiana-discuss] ufsdump/ufsrestore

[James Carlson]

On 07/01/16 12:11, Dr Robert Pasken wrote:
> I recently added a 80gb sata drive to test OI-Hipster. On a whole
> OI-Hipster is a significant improvement EXCEPT with regards to ufsrestore.
> Ufsrestore dumps core shortly after starting up. I copied the ufsrestore
> from OI-151a9 and as is well.
> 
> A "where" from dbx /usr/sbin/ufsrestore shows (addresses deleted):
> 
> [1] lookupparent
> [2] addentry
> [3] initsymtable
> [4] main
> 
> Signal SEGV [access to address exceeded protection] in lookupparent at
> 0x805b168
> 
> 0x805b168: lookupparent+0x007a  movb $0x0000000 (%esi)

I'm kinda shocked this code ever worked or was ever tested.  :-/

At a guess, the one reason it may have once appeared to work is that
ufsrestore may have been compiled with writable strings, and it now
fails because someone decided that making strings constant was a good thing.

Or, at another guess, it was pure happenstance.  The string involved
(see discussion below) just happened to abut a series of zeros in
memory, and nothing bad happened even though the code was skating away
on the thin ice of a new segment.

Among other places, you can view the source here:

https://hg.openindiana.org/upstream/illumos/illumos-gate/file/afe390b9f1e0/usr/src/cmd/backup/restore

main() (in main.c) invokes initsymtable(NULL) from a number of places,
including when unpacking a level-0 dump.  Inside initsymtable() (in
symtab.c), if the argument is NULL, then it does this:

  ep = addentry(".", ROOTINO, NODE);

This seems like a fairly logical thing to do, but the rest of the code
makes it clear that this is quite toxic, because addentry() actually
ends up modifying its first argument.  That function rather quickly
invokes lookupparent("."), and lookupparent is just a world of hurt.

It first does LASTPART() on that pointer.  That rather comically marches
well off the end of the passed-in constant "." string:

#define LASTPART(s)     {int len = strlen(s)+1;\
                                while (s[len] != '\0')\
                                        {s += len; len = strlen(s)+1; }\
                        }

It's unclear where the 'name' argument ends up pointing after that's
done, but it can't possibly be anywhere good.

It then tries to do strrchr() to look for '/' (somewhere possibly after
the end of "." -- in uncharted memory).  If it doesn't find it (it
probably doesn't), then it stores two consecutive 0 bytes starting at
whatever garbage LASTPART() returned.

That's where the explosion appears to occur.

This code appears to be related to UFS extended attributes, but that's
just a guess based on some neighboring comments.

I believe that removing the addentry() invocation from initsymtable()
and replacing it with code like this should fix the immediate problem:

ep = calloc(1, sizeof (*ep));
ep->e_type = NODE;
ep->e_name = savename(".");
ep->e_namlen = 1;
ep->e_parent = ep;
addino(ROOTINO, ep);

... but, really, after reading this stuff and then scrubbing my eyeballs
with a scouring pad, I wouldn't trust it much further than that.  Who
knows what else is wrong?

-- 
James Carlson         42.703N 71.076W         <carlsonj at workingcode.com>