Fix off-by-one error in TarWriter.

The tar format originally supported up to 99 byte filename. The two
extensions are proposed later: Ustar or PAX.

In the UStar extension, a pathanme is split at a '/' and its "prefix"
and "suffix" are stored in different locations in the tar header. Since
"prefix" can be up to 155 byte, it can represent up to 254 byte
filename (but exact limit depends on the location of '/' character in
a pathname.)

Our TarWriter first attempt to use UStar extension and then fallback to
PAX extension.

But there's a bug in UStar header creation. "Suffix" part must be a NUL-
terminated string, but we didn't handle it correctly. As a result, if
your filename just 100 characters long, the last character was droppped.

This patch fixes the issue.

Differential Revision: https://reviews.llvm.org/D38149

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@314349 91177308-0d34-0410-b5e6-96231b3b80d8

lib/Support/TarWriter.cpp

@@ -116,34 +116,36 @@ static void writePaxHeader(raw_fd_ostream &OS, StringRef Path) {
   pad(OS);
 }
 
-// In the Ustar header, a path can be split at any '/' to store
-// a path into UstarHeader::Name and UstarHeader::Prefix. This
-// function splits a given path for that purpose.
-static std::pair<StringRef, StringRef> splitPath(StringRef Path) {
-  if (Path.size() <= sizeof(UstarHeader::Name))
-    return {"", Path};
+// Path fits in a Ustar header if
+//
+// - Path is less than 100 characters long, or
+// - Path is in the form of "<prefix>/<name>" where <prefix> is less
+//   than or equal to 155 characters long and <name> is less than 100
+//   characters long. Both <prefix> and <name> can contain extra '/'.
+//
+// If Path fits in a Ustar header, updates Prefix and Name and returns true.
+// Otherwise, returns false.
+static bool splitUstar(StringRef Path, StringRef &Prefix, StringRef &Name) {
+  if (Path.size() < sizeof(UstarHeader::Name)) {
+    Name = Path;
+    return true;
+  }
+
   size_t Sep = Path.rfind('/', sizeof(UstarHeader::Prefix) + 1);
   if (Sep == StringRef::npos)
-    return {"", Path};
-  return {Path.substr(0, Sep), Path.substr(Sep + 1)};
-}
+    return false;
+  if (Path.size() - Sep - 1 >= sizeof(UstarHeader::Name))
+    return false;
 
-// Returns true if a given path can be stored to a Ustar header
-// without the PAX extension.
-static bool fitsInUstar(StringRef Path) {
-  StringRef Prefix;
-  StringRef Name;
-  std::tie(Prefix, Name) = splitPath(Path);
-  return Name.size() <= sizeof(UstarHeader::Name);
+  Prefix = Path.substr(0, Sep);
+  Name = Path.substr(Sep + 1);
+  return true;
 }
 
 // The PAX header is an extended format, so a PAX header needs
 // to be followed by a "real" header.
-static void writeUstarHeader(raw_fd_ostream &OS, StringRef Path, size_t Size) {
-  StringRef Prefix;
-  StringRef Name;
-  std::tie(Prefix, Name) = splitPath(Path);
-
+static void writeUstarHeader(raw_fd_ostream &OS, StringRef Prefix,
+                             StringRef Name, size_t Size) {
   UstarHeader Hdr = makeUstarHeader();
   memcpy(Hdr.Name, Name.data(), Name.size());
   memcpy(Hdr.Mode, "0000664", 8);
@@ -168,12 +170,15 @@ TarWriter::TarWriter(int FD, StringRef BaseDir)
 // Append a given file to an archive.
 void TarWriter::append(StringRef Path, StringRef Data) {
   // Write Path and Data.
-  std::string S = BaseDir + "/" + sys::path::convert_to_slash(Path) + "\0";
-  if (fitsInUstar(S)) {
-    writeUstarHeader(OS, S, Data.size());
+  std::string Fullpath = BaseDir + "/" + sys::path::convert_to_slash(Path);
+
+  StringRef Prefix;
+  StringRef Name;
+  if (splitUstar(Fullpath, Prefix, Name)) {
+    writeUstarHeader(OS, Prefix, Name, Data.size());
   } else {
-    writePaxHeader(OS, S);
-    writeUstarHeader(OS, "", Data.size());
+    writePaxHeader(OS, Fullpath);
+    writeUstarHeader(OS, "", "", Data.size());
   }
 
   OS << Data;

unittests/Support/TarWriterTest.cpp

@@ -11,6 +11,7 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "gtest/gtest.h"
+#include <vector>
 
 using namespace llvm;
 namespace {
@@ -37,7 +38,7 @@ struct UstarHeader {
 
 class TarWriterTest : public ::testing::Test {};
 
-static UstarHeader create(StringRef Base, StringRef Filename) {
+static std::vector<uint8_t> createTar(StringRef Base, StringRef Filename) {
   // Create a temporary file.
   SmallString<128> Path;
   std::error_code EC =
@@ -55,34 +56,68 @@ static UstarHeader create(StringRef Base, StringRef Filename) {
   ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr = MemoryBuffer::getFile(Path);
   EXPECT_TRUE((bool)MBOrErr);
   std::unique_ptr<MemoryBuffer> MB = std::move(*MBOrErr);
+  std::vector<uint8_t> Buf((const uint8_t *)MB->getBufferStart(),
+                           (const uint8_t *)MB->getBufferEnd());
+
+  // Windows does not allow us to remove a mmap'ed files, so
+  // unmap first and then remove the temporary file.
+  MB = nullptr;
   sys::fs::remove(Path);
-  return *reinterpret_cast<const UstarHeader *>(MB->getBufferStart());
+
+  return Buf;
+}
+
+static UstarHeader createUstar(StringRef Base, StringRef Filename) {
+  std::vector<uint8_t> Buf = createTar(Base, Filename);
+  EXPECT_TRUE(Buf.size() >= sizeof(UstarHeader));
+  return *reinterpret_cast<const UstarHeader *>(Buf.data());
 }
 
 TEST_F(TarWriterTest, Basics) {
-  UstarHeader Hdr = create("base", "file");
+  UstarHeader Hdr = createUstar("base", "file");
   EXPECT_EQ("ustar", StringRef(Hdr.Magic));
   EXPECT_EQ("00", StringRef(Hdr.Version, 2));
   EXPECT_EQ("base/file", StringRef(Hdr.Name));
   EXPECT_EQ("00000000010", StringRef(Hdr.Size));
 }
 
 TEST_F(TarWriterTest, LongFilename) {
-  UstarHeader Hdr1 = create(
-      "012345678", std::string(99, 'x') + "/" + std::string(44, 'x') + "/foo");
-  EXPECT_EQ("foo", StringRef(Hdr1.Name));
-  EXPECT_EQ("012345678/" + std::string(99, 'x') + "/" + std::string(44, 'x'),
-            StringRef(Hdr1.Prefix));
-
-  UstarHeader Hdr2 = create(
-      "012345678", std::string(99, 'x') + "/" + std::string(45, 'x') + "/foo");
-  EXPECT_EQ("foo", StringRef(Hdr2.Name));
-  EXPECT_EQ("012345678/" + std::string(99, 'x') + "/" + std::string(45, 'x'),
-            StringRef(Hdr2.Prefix));
-
-  UstarHeader Hdr3 = create(
-      "012345678", std::string(99, 'x') + "/" + std::string(46, 'x') + "/foo");
-  EXPECT_EQ(std::string(46, 'x') + "/foo", StringRef(Hdr3.Name));
-  EXPECT_EQ("012345678/" + std::string(99, 'x'), StringRef(Hdr3.Prefix));
+  std::string x154(154, 'x');
+  std::string x155(155, 'x');
+  std::string y99(99, 'y');
+  std::string y100(100, 'y');
+
+  UstarHeader Hdr1 = createUstar("", x154 + "/" + y99);
+  EXPECT_EQ("/" + x154, StringRef(Hdr1.Prefix));
+  EXPECT_EQ(y99, StringRef(Hdr1.Name));
+
+  UstarHeader Hdr2 = createUstar("", x155 + "/" + y99);
+  EXPECT_EQ("", StringRef(Hdr2.Prefix));
+  EXPECT_EQ("", StringRef(Hdr2.Name));
+
+  UstarHeader Hdr3 = createUstar("", x154 + "/" + y100);
+  EXPECT_EQ("", StringRef(Hdr3.Prefix));
+  EXPECT_EQ("", StringRef(Hdr3.Name));
+
+  UstarHeader Hdr4 = createUstar("", x155 + "/" + y100);
+  EXPECT_EQ("", StringRef(Hdr4.Prefix));
+  EXPECT_EQ("", StringRef(Hdr4.Name));
+
+  std::string yz = "yyyyyyyyyyyyyyyyyyyy/zzzzzzzzzzzzzzzzzzzz";
+  UstarHeader Hdr5 = createUstar("", x154 + "/" + yz);
+  EXPECT_EQ("/" + x154, StringRef(Hdr5.Prefix));
+  EXPECT_EQ(yz, StringRef(Hdr5.Name));
+}
+
+TEST_F(TarWriterTest, Pax) {
+  std::vector<uint8_t> Buf = createTar("", std::string(200, 'x'));
+  EXPECT_TRUE(Buf.size() >= 1024);
+
+  auto *Hdr = reinterpret_cast<const UstarHeader *>(Buf.data());
+  EXPECT_EQ("", StringRef(Hdr->Prefix));
+  EXPECT_EQ("", StringRef(Hdr->Name));
+
+  StringRef Pax = StringRef((char *)(Buf.data() + 512), 512);
+  EXPECT_TRUE(Pax.startswith("211 path=/" + std::string(200, 'x')));
 }
 }