Fix off-by-one error in TarWriter.

[ruiu]

Summary

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.

[zturner]

llvm/lib/Support/TarWriter.cpp

128

I don't think we need std::string here. Can't we just use StringRef again? Also, how about just returning Optional<std::pair<StringRef, StringRef>>?

130–131

return std::make_pair(StringRef(), Path);

134

What about this Path?

<total of 95 a's>/aaaaa/bb

in this case we have 95 + 1 + 5 = 101 characters, followed by a /, followed by bb.

Does this path conform to Ustar header? You can split it as (<total of 95 a's>, "aaaaa/bb") but then you have a / in the name. Is this ok?

[ruiu]

• updated as per Zach's comments

[ruiu]

llvm/lib/Support/TarWriter.cpp

128

I actually tried to use Optional<std::pair<StringRef, StringRef>> but it looked like it is a bit too complicated type for this little helper function.

134

That's OK. Updated the comment a bit.

[zturner]

Can you add the following tests?

std::string(154, 'x') + "/" + std::string(100, 'y');
std::string(150, 'x') + "/" + std::string(20, 'y') + "/" + std::string(20, 'z');

Can you also add a test that if we can't use a Ustar header, that we can still produce a valid Tar file by using a Pax header?

llvm/lib/Support/TarWriter.cpp

128

Might as well make them StringRef& instead.

[ruiu]

• Address review comments.

[alekseyshl]

UBsan is not happy about TarWriter.cpp: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/8197, can you check it out?

llvm/lib/Support/TarWriter.cpp

static void writePaxHeader(raw_fd_ostream &OS, StringRef Path) {
   computeChecksum(Hdr);
 
   // Write them down.
   OS << StringRef(reinterpret_cast<char *>(&Hdr), sizeof(Hdr));
   OS << PaxAttr;
   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};
-  size_t Sep = Path.rfind('/', sizeof(UstarHeader::Prefix) + 1);
-  if (Sep == StringRef::npos)
-    return {"", Path};
-  return {Path.substr(0, Sep), Path.substr(Sep + 1)};
+// Path fits in a Ustar header if
+//
+// - Path is 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.
+//
+// If Path fits in a Ustar header, updates Prefix and Name and returns true.
+// Otherwise, returns false.
+static bool splitUstar(StringRef Path, std::string &Prefix, std::string &Name) {
+  if (Path.size() < sizeof(UstarHeader::Name)) {
+    Name = Path;
+    return true;
   }
 
-// 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);
+  size_t Sep = Path.rfind('/', sizeof(UstarHeader::Prefix) + 1);
+  if (Sep == StringRef::npos)
+    return false;
+  if (Path.size() - Sep - 1 >= sizeof(UstarHeader::Name))
+    return false;
+
+  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);
   snprintf(Hdr.Size, sizeof(Hdr.Size), "%011zo", Size);
   memcpy(Hdr.Prefix, Prefix.data(), Prefix.size());
   computeChecksum(Hdr);
   OS << StringRef(reinterpret_cast<char *>(&Hdr), sizeof(Hdr));
 }
 
 // Creates a TarWriter instance and returns it.
 Expected<std::unique_ptr<TarWriter>> TarWriter::create(StringRef OutputPath,
                                                        StringRef BaseDir) {
   int FD;
   if (std::error_code EC = openFileForWrite(OutputPath, FD, sys::fs::F_None))
     return make_error<StringError>("cannot open " + OutputPath, EC);
   return std::unique_ptr<TarWriter>(new TarWriter(FD, BaseDir));
 }
 
 TarWriter::TarWriter(int FD, StringRef BaseDir)
     : OS(FD, /*shouldClose=*/true, /*unbuffered=*/false), BaseDir(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);
+
+  std::string Prefix;
+  std::string 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;
   pad(OS);
 
   // POSIX requires tar archives end with two null blocks.
   // Here, we write the terminator and then seek back, so that
   // the file being output is terminated correctly at any moment.
   uint64_t Pos = OS.tell();
   OS << std::string(BlockSize * 2, '\0');
   OS.seek(Pos);
   OS.flush();
 }

llvm/unittests/Support/TarWriterTest.cpp

TEST_F(TarWriterTest, Basics) {
   UstarHeader Hdr = create("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));
+  UstarHeader Hdr1 =
+      create("", std::string(154, 'x') + "/" + std::string(99, 'y'));
+  EXPECT_EQ("/" + std::string(154, 'x'), StringRef(Hdr1.Prefix));
+  EXPECT_EQ(std::string(99, 'y'), StringRef(Hdr1.Name));
+
+  UstarHeader Hdr2 =
+      create("", std::string(155, 'x') + "/" + std::string(99, 'y'));
+  EXPECT_EQ("", StringRef(Hdr2.Prefix));
+  EXPECT_EQ("", StringRef(Hdr2.Name));
+
+  UstarHeader Hdr3 =
+      create("", std::string(155, 'x') + "/" + std::string(100, 'y'));
+  EXPECT_EQ("", StringRef(Hdr3.Prefix));
+  EXPECT_EQ("", StringRef(Hdr3.Name));
 }
 }