Project

General

Profile

Bug #6036

ufsrestore segfaults in lookupparent

Added by Josef Sipek over 5 years ago. Updated 12 days ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
cmd - userland programs
Start date:
2015-06-25
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
needs-triage
Gerrit CR:

Description

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

> $c
lookupparent+0x7a(8064b25, 2c, 66664, fef6e000, fef6e824, 80ef600)
addentry+0x8d(8064b25, 2, 2, 0, 10001, fef6e000)
initsymtable+0xa1(0, 8045c00, 80460a8, 8058b0e, ffff, 0)
main+0x933(804609c, fef786e8, 80460d8, 8054b5b, 4, 80460e4)
_start+0x83(4, 804658c, 80465a1, 80465a5, 8064b25, 0)
> ::status
debugging core file of ufsrestore (32-bit) from ganymede
file: /usr/lib/fs/ufs/ufsrestore
initial argv: /usr/sbin/ufsrestore rbf 512 /dev/rdsk/c1t5000C500563ED21Fd0s0
threading model: native threads
status: process terminated by SIGSEGV (Segmentation Fault), addr=8065dca
> 8064b25::dump
           0 1 2 3  4\/ 6 7  8 9 a b  c d e f  01234v6789abcdef
8064b20:  2e2f002e 2e2e0063 6f727275 70746564  ./.....corrupted
> 8064b25/s
0x8064b25:      .

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


Files

#1

Updated by James Carlson over 4 years ago

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.

#2

Updated by Electric Monk 18 days ago

  • Gerrit CR set to 1220
#3

Updated by Toomas Soome 18 days ago

  • Status changed from New to In Progress
  • Assignee set to Toomas Soome
  • % Done changed from 0 to 90
#4

Updated by Toomas Soome 12 days ago

  • Description updated (diff)
#5

Updated by Toomas Soome 12 days ago

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.

#6

Updated by Electric Monk 12 days ago

  • Status changed from In Progress to Closed
  • % Done changed from 90 to 100

git commit d9529689937cca41f8af4b28094109ebe366870d

commit  d9529689937cca41f8af4b28094109ebe366870d
Author: Toomas Soome <tsoome@me.com>
Date:   2021-02-13T06:29:54.000Z

    6036 ufsrestore segfaults in lookupparent
    Reviewed by: Andy Fiddaman <andy@omnios.org>
    Approved by: Dan McDonald <danmcd@joyent.com>

Also available in: Atom PDF