Add support for over-long filepaths via GNU extension and fix hardlinks

[Bodigrim]

This is #50 rebased, all credit due to @hasufell.

[hasufell]

@Bodigrim do you want a review for this? It's quite a long time ago I wrote this.

[Bodigrim]

@hasufell do you remember how you tested this branch long ago?

[hasufell]

@hasufell do you remember how you tested this branch long ago?

Pretty sure I used actual tar archives. I can try to give this a go maybe.

[hasufell]

Quite interestingly htar hangs on all operations indefinitely for me. I sprinkled traces all over the place but could not detect a loop. The codebase abuses laziness heavily, so it's hard to find.

[hasufell]

So, to test long filepaths, I changed the arbitrary instance like so:

diff --git a/test/Codec/Archive/Tar/Types/Tests.hs b/test/Codec/Archive/Tar/Types/Tests.hs
index d2acc43..912bebb 100644
--- a/test/Codec/Archive/Tar/Types/Tests.hs
+++ b/test/Codec/Archive/Tar/Types/Tests.hs
@@ -48,7 +48,7 @@ instance Arbitrary TarPath where
   arbitrary = either error id
             . toTarPath False
             . FilePath.Posix.joinPath
-          <$> listOf1ToN (255 `div` 5)
+          <$> listOf1ToN 500
                          (elements (map (replicate 4) "abcd"))

   shrink = map (either error id . toTarPath False)

And then discovered that commit 0af50df seems to break the code. If I revert it, the test suite passes.

[hasufell]

And then discovered that commit 0af50df seems to break the code. If I revert it, the test suite passes

I was able to fix it this way:

diff --git a/test/Codec/Archive/Tar/Types/Tests.hs b/test/Codec/Archive/Tar/Types/Tests.hs
index d2acc43..134b141 100644
--- a/test/Codec/Archive/Tar/Types/Tests.hs
+++ b/test/Codec/Archive/Tar/Types/Tests.hs
@@ -45,19 +45,26 @@ instance Arbitrary Entry where
       | perms' <- shrinkIntegral perms ]

 instance Arbitrary TarPath where
-  arbitrary = either error id
-            . toTarPath False
+  arbitrary = fromTarPathResult
+            . toTarPath' False
             . FilePath.Posix.joinPath
-          <$> listOf1ToN (255 `div` 5)
+          <$> listOf1ToN 500
                          (elements (map (replicate 4) "abcd"))

-  shrink = map (either error id . toTarPath False)
+  shrink = map (fromTarPathResult . toTarPath' False)
          . map FilePath.Posix.joinPath
          . filter (not . null)
          . shrinkList shrinkNothing
          . FilePath.Posix.splitPath
          . fromTarPathToPosixPath

+-- errors only on FileNameEmpty, because long filenames are supported
+fromTarPathResult :: ToTarPathResult -> TarPath
+fromTarPathResult FileNameEmpty = error "File name empty"
+fromTarPathResult (FileNameOK tp) = tp
+fromTarPathResult (FileNameTooLong tp) = tp
+
+
 instance Arbitrary LinkTarget where
   arbitrary = maybe (error "link target too large") id
             . toLinkTarget

[hasufell]

This actually makes me question the code. See the tests here:

tar/test/Codec/Archive/Tar/Tests.hs

Lines 25 to 42 in 7d0517d

	prop_write_read_ustar :: [Entry] -> Bool
	prop_write_read_ustar entries =
	foldr Next Done entries' == read (write entries')
	where
	entries' = [ e { entryFormat = UstarFormat } | e <- entries ]
	prop_write_read_gnu :: [Entry] -> Bool
	prop_write_read_gnu entries =
	foldr Next Done entries' == read (write entries')
	where
	entries' = [ e { entryFormat = GnuFormat } | e <- entries ]
	prop_write_read_v7 :: [Entry] -> Bool
	prop_write_read_v7 entries =
	foldr Next Done entries' == read (write entries')
	where
	entries' = [ limitToV7FormatCompat e { entryFormat = V7Format }
	| e <- entries ]

I'm not sure we would expect long filenames to work under non-GNU formats?

[hasufell]

ustar and v7 formats don't support it: https://www.gnu.org/software/tar/manual/html_chapter/Formats.html

My tests show that tar cli utility is permissive when it comes to unpacking/listing, but rejects writing such archives when you specify --format=ustar for example.

It doesn't seem we allow great control to the users over the format, but the following patch might be an improvement in strictness anyway (and will also make ustar and v7 property tests fail with long filenames, which I would expect):

diff --git a/Codec/Archive/Tar/Write.hs b/Codec/Archive/Tar/Write.hs
index 0d76e14..f3dcf5e 100644
--- a/Codec/Archive/Tar/Write.hs
+++ b/Codec/Archive/Tar/Write.hs
@@ -38,6 +38,8 @@ write es = LBS.concat $ map putEntry es ++ [LBS.replicate (512*2) 0]
 putEntry :: Entry -> LBS.ByteString
 putEntry entry = case entryContent entry of
   NormalFile       content size -> LBS.concat [ header, content, padding size ]
+  OtherEntryType 'L' _ _
+    | entryFormat entry /= GnuFormat -> error "Long filename support is a GNU extension"
   OtherEntryType _ content size -> LBS.concat [ header, content, padding size ]
   _                             -> header
   where

[hasufell]

And sorry to bother for more, but I think we should also support

/* Identifies the next file on the tape as having a long linkname. */
#define GNUTYPE_LONGLINK 'K'

https://www.gnu.org/software/tar/manual/html_node/Standard.html

Otherwise it seems rather inconsistent. I can write a patch for that.

[Bodigrim]

Otherwise it seems rather inconsistent. I can write a patch for that.

Sure, that would be most appreciated.

Shall we merge this then? I tested it on examples from #49, seems doing well.

[hasufell]

Shall we merge this then?

I'd rather have it cleaned up a bit more and add tests with adjusted arbitrary instance? There are also a few other things I'm starting to notice. E.g. handling of unknown filetypes isn't entirely correct. From the spec:

The typeflag field specifies the type of file archived. If a particular implementation does not recognize or permit the specified type, the file will be extracted as if it were a regular file. As this action occurs, tar issues a warning to the standard error.

[Bodigrim]

I've added a test to check that pack/unpack roundtrips with long file names + restricted long names to GnuFormat. It would be nice to test symlinks, but I'd rather do it in a separate PR later.

[hasufell]

It would be nice to test symlinks, but I'd rather do it in a separate PR later

I'm already done, just adding tests. Should I push to this PR or create another one?

[hasufell]

https://github.com/haskell/tar/pull/80/files

[Bodigrim]

Hmm, something feels off: both

cabal run htar -- --list test/data/long.tar

and even

tar --list test/data/long.tar

just freeze on my machine. And yet the test suite succeeds. How is it possible?..

[hasufell]

htar needs an explicit --file= parameter, otherwise it expects stdin.

Not sure why tar wouldn't be able to read it. I can check.

[Bodigrim]

I'm on MacOS, so it's bsdtar.

$ tar --version
bsdtar 3.5.3 - libarchive 3.5.3 zlib/1.2.11 liblzma/5.0.5 bz2lib/1.0.8

It might not support GNU extensions well indeed...

[Bodigrim]

Nevermind, tar needs f as well. All good.

[hasufell]

Btw, now I remember when I debug printed filepaths, that they had a trailing NUL char in unpack.

Most filepath operations rely on withCString to implicitly truncate, but that may no longer be true when using OsPath at some point.

It may make sense to sanitize them after extraction from the headers.

[Bodigrim]

Let's go!