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 ↗

(On Diff #116249)

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 ↗

(On Diff #116249)

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

134 ↗

(On Diff #116249)

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 ↗

(On Diff #116249)

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 ↗

(On Diff #116249)

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 ↗

(On Diff #116255)

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/trunk/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 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;
   }
 
-// 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);
+
+  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;
   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/trunk/unittests/Support/TarWriterTest.cpp

 //===- llvm/unittest/Support/TarWriterTest.cpp ----------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/TarWriter.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "gtest/gtest.h"
+#include <vector>
 
 using namespace llvm;
 namespace {
 
 struct UstarHeader {
   char Name[100];
   char Mode[8];
   char Uid[8];
struct UstarHeader {
   char DevMajor[8];
   char DevMinor[8];
   char Prefix[155];
   char Pad[12];
 };
 
 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 =
       sys::fs::createTemporaryFile("TarWriterTest", "tar", Path);
   EXPECT_FALSE((bool)EC);
 
   // Create a tar file.
   Expected<std::unique_ptr<TarWriter>> TarOrErr = TarWriter::create(Path, Base);
   EXPECT_TRUE((bool)TarOrErr);
   std::unique_ptr<TarWriter> Tar = std::move(*TarOrErr);
   Tar->append(Filename, "contents");
   Tar.reset();
 
   // Read the tar file.
   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')));
 }
 }