[Bug-cpio] Re: Bug#565474: cpio makes device nodes into hard links when copying out of a cramfs image

[Carl Miller]

On Mon, Jan 18, 2010 at 02:06:07PM -0800, Carl Miller wrote:
> On Mon, Jan 18, 2010 at 11:52:12AM -0800, Carl Miller wrote:
> > 
> > So now my question is, why do callers of add_inode() not check the
> > st_nlinks for being > 1 first, like callers of add_link_defer()?  Is
> > there some reason that it *should* be this way?  If not, I'd propose
> > putting that check in before calling add_inode(), and only having files
> > with nlinks >= 2 in the inode hash table.  Thoughts?
> > 
> > (Proposed patch to follow while others think of reasons this might break
> > something else...)
> 
> It looks to me like copypass.c, the other user of add_inode() only adds
> to the hash table if st_nlinks > 1.  That eases my concerns about having
> copyout.c do the same.  Here's my proposed patch....
> 
> 
> address@hidden:/tmp/cpio_patch/cpio-2.10$ diff -du src/copyout.c{.orig,}
> --- src/copyout.c.orig  2009-02-14 10:15:50.000000000 -0800
> +++ src/copyout.c       2010-01-18 14:02:20.000000000 -0800
> @@ -232,7 +232,8 @@
>                            header->c_name);
>    warn_if_file_changed(header->c_name, file_hdr.c_filesize, 
> file_hdr.c_mtime);
> 
> -  if (archive_format == arf_tar || archive_format == arf_ustar)
> +  if ((archive_format == arf_tar || archive_format == arf_ustar)
> +      && (file_hdr.c_nlink > 1))
>      add_inode (file_hdr.c_ino, file_hdr.c_name, file_hdr.c_dev_maj,
>                file_hdr.c_dev_min);
> 
> @@ -652,7 +653,7 @@
> 
>           if (archive_format == arf_tar || archive_format == arf_ustar)
>             {
> -             if (file_hdr.c_mode & CP_IFDIR)
> +             if ((file_hdr.c_mode & CP_IFMT) == CP_IFDIR)
>                 {
>                   int len = strlen (input_name.ds_string);
>                   /* Make sure the name ends with a slash */
> @@ -696,7 +697,8 @@
>           switch (file_hdr.c_mode & CP_IFMT)
>             {
>             case CP_IFREG:
> -             if (archive_format == arf_tar || archive_format == arf_ustar)
> +             if ((archive_format == arf_tar || archive_format == arf_ustar)
> +                 && (file_hdr.c_nlink > 1))
>                 {
>                   char *otherfile;
>                   if ((otherfile = find_inode_file (file_hdr.c_ino,
> @@ -743,7 +745,8 @@
>               warn_if_file_changed(orig_file_name, file_hdr.c_filesize,
>                                     file_hdr.c_mtime);
> 
> -             if (archive_format == arf_tar || archive_format == arf_ustar)
> +             if ((archive_format == arf_tar || archive_format == arf_ustar)
> +                 && (file_hdr.c_nlink > 1))
>                 add_inode (file_hdr.c_ino, orig_file_name, file_hdr.c_dev_maj,
>                            file_hdr.c_dev_min);
> 
> @@ -777,7 +780,7 @@
>                          orig_file_name);
>                   continue;
>                 }
> -             else if (archive_format == arf_ustar)
> +             else if ((archive_format == arf_ustar) && (file_hdr.c_nlink > 
> 1))
>                 {
>                   char *otherfile;
>                   if ((otherfile = find_inode_file (file_hdr.c_ino,
> ----------------
> 
> I'll compile it and run the same test case against it now...

address@hidden:/tmp/unpack# mkdir mnt_228
address@hidden:/tmp/unpack# mount -t cramfs -o ro,nodev,noexec,loop 
/usr/local/isengard/share/builds/build228/disk_fs.img mnt_228
address@hidden:/tmp/unpack# ls -l mnt_228/dev/loop?
brw-r--r-- 1 root root 7, 0 1969-12-31 16:00 mnt_228/dev/loop0
brw-r--r-- 1 root root 7, 1 1969-12-31 16:00 mnt_228/dev/loop1
brw-r--r-- 1 root root 7, 2 1969-12-31 16:00 mnt_228/dev/loop2
brw-r--r-- 1 root root 7, 3 1969-12-31 16:00 mnt_228/dev/loop3
address@hidden:/tmp/unpack# cd mnt_228
address@hidden:/tmp/unpack/mnt_228# ( echo "dev" ; echo "dev/loop0" ; echo 
"dev/loop1" ; echo "dev/loop2" ; echo "dev/loop3" ) | 
/tmp/cpio_patch/cpio-2.10/src/cpio -o -F /tmp/looptest.tar -H ustar --quiet
address@hidden:/tmp/unpack/mnt_228# tar -tf /tmp/looptest.tar
tar: Record size = 7 blocks
dev/
dev/loop0
dev/loop1
dev/loop2
dev/loop3
address@hidden:/tmp/unpack/mnt_228# cd ..
address@hidden:/tmp/unpack# tar -xvf /tmp/looptest.tar
tar: Record size = 7 blocks
dev/
dev/loop0
tar: dev/loop0: implausibly old time stamp 1969-12-31 16:00:00
dev/loop1
tar: dev/loop1: implausibly old time stamp 1969-12-31 16:00:00
dev/loop2
tar: dev/loop2: implausibly old time stamp 1969-12-31 16:00:00
dev/loop3
tar: dev/loop3: implausibly old time stamp 1969-12-31 16:00:00
tar: dev: implausibly old time stamp 1969-12-31 16:00:00
address@hidden:/tmp/unpack# ls -l dev
total 0
brw-r--r-- 1 root root 7, 0 1969-12-31 16:00 loop0
brw-r--r-- 1 root root 7, 1 1969-12-31 16:00 loop1
brw-r--r-- 1 root root 7, 2 1969-12-31 16:00 loop2
brw-r--r-- 1 root root 7, 3 1969-12-31 16:00 loop3
address@hidden:/tmp/unpack#


Much better.  No trailing slashes; all archived files unpack, and have the
correct st_rdev.  I'm going to call that a fix.

Thanks much, Clint and Tim!


                             -------Carl