illumos gate

Josef Sipek wrote:

One of the zfs-tests (functional/inuse/inuse_003_pos.ksh) uses ufsrestore which happens to segfault. I don't think it is an issue with the test, but rather ufsrestore has a bug of some sort. Since some folks have run into it independently:

http://lists.omniti.com/pipermail/omnios-discuss/2014-November/003710.html

[...]

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, it then 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 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 happens.

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

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);

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 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.

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? It could use some careful code inspection.

Josef Sipek wrote:

One of the zfs-tests (functional/inuse/inuse_003_pos.ksh) uses ufsrestore which happens to segfault. I don't think it is an issue with the test, but rather ufsrestore has a bug of some sort. Since some folks have run into it independently:

http://lists.omniti.com/pipermail/omnios-discuss/2014-November/003710.html

[...]

Testing done: ufsrestore appears to list (t) or restore (x) files.

James Carlson wrote in #note-1:

Josef Sipek wrote:

One of the zfs-tests (functional/inuse/inuse_003_pos.ksh) uses ufsrestore which happens to segfault. I don't think it is an issue with the test, but rather ufsrestore has a bug of some sort. Since some folks have run into it independently:

http://lists.omniti.com/pipermail/omnios-discuss/2014-November/003710.html

[...]

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, it then 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 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 {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 happens.

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

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);

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 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.

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? It could use some careful code inspection.

Please note, addentry() is expecting not C-string, but complex string, which is array of c-strings and is terminated by NUL byte. The "." should be presented as { '.', 0, 0 }. Other fix could be to use local variable set to have sequence { '.', 0, 0 }, but I actually do like the code block above better, it does feel more clear.