Fix bug 34051 by handling empty .res files gracefully.

[ecbeckmann]

Summary

Previously, llvm-cvtres crashes on .res files which are empty except for
the null header. This allows the library to simply pass over them.

[ecbeckmann]

Remove unnecessary braces

[ruiu]

llvm/include/llvm/Object/WindowsResource.h

92

What is ID?

llvm/lib/Object/WindowsResource.cpp

68

I'm not sure if I understand this code correctly. Does this swallow Err if a new error occurs?

If you can change this part of code, a better approach to create an instance of some class whose constructor can fail is to provide a factory function and returns an Error or a new instance from the factory function.

For example, instead of providing a ctor that can fail, you could provide a static member function:

static ErrorOr<ResourceEntryRef> ResourceEntryRef::create(BinaryStreamRef Ref,
  const WindowsResource *Owner);

which in turn call the ctor only when all conditions are met (thus the ctor is free from failure.)

136

Does this ignore errors? Crash bug needs to be fixed, but I wonder if an empty file should be considered as a valid input.

[ecbeckmann]

Make factory function instead of having constructor fail.

llvm/include/llvm/Object/WindowsResource.h

92

Ah you're right it's not necessary. Normally subclasses of ErrorInfo need to have this defined or else the linker won't be happy but I guess that only applies to direct non-virtual subclasses.

llvm/lib/Object/WindowsResource.cpp

136

Not an empty file, it still contains PE header magic and an empty, null entry. So it's a valid .res file that just doesn't contain any records, which I could see being valid in some cases. Also, the original cvtres does not crash/throw error on empty .res.

A completely empty file would fail upon WindowsResource::createWindowsResource().

[ruiu]

LGTM

llvm/lib/Object/WindowsResource.cpp

136

Please add that as a comment.

[ecbeckmann]

Add comment

llvm/include/llvm/Object/WindowsResource.h

 struct WinResHeaderSuffix {
   support::ulittle32_t DataVersion;
   support::ulittle16_t MemoryFlags;
   support::ulittle16_t Language;
   support::ulittle32_t Version;
   support::ulittle32_t Characteristics;
 };
 
+class EmptyResError : public GenericBinaryError {
+public:
+  EmptyResError(Twine Msg, object_error ECOverride)
+      : GenericBinaryError(Msg, ECOverride) {}
+};
+
 class ResourceEntryRef {
 public:
   Error moveNext(bool &End);
   bool checkTypeString() const { return IsStringType; }
   ArrayRef<UTF16> getTypeString() const { return Type; }
   uint16_t getTypeID() const { return TypeID; }
   bool checkNameString() const { return IsStringName; }
   ArrayRef<UTF16> getNameString() const { return Name; }
   uint16_t getNameID() const { return NameID; }
   uint16_t getLanguage() const { return Suffix->Language; }
   uint16_t getMajorVersion() const { return Suffix->Version >> 16; }
   uint16_t getMinorVersion() const { return Suffix->Version; }
   uint32_t getCharacteristics() const { return Suffix->Characteristics; }
   ArrayRef<uint8_t> getData() const { return Data; }
 
 private:
   friend class WindowsResource;
 
-  ResourceEntryRef(BinaryStreamRef Ref, const WindowsResource *Owner,
-                   Error &Err);
-
+  ResourceEntryRef(BinaryStreamRef Ref, const WindowsResource *Owner);
   Error loadNext();
 
+  static Expected<ResourceEntryRef> create(BinaryStreamRef Ref,
+                                           const WindowsResource *Owner);
+
   BinaryStreamReader Reader;
   bool IsStringType;
   ArrayRef<UTF16> Type;
   uint16_t TypeID;
   bool IsStringName;
   ArrayRef<UTF16> Name;
   uint16_t NameID;
   const WinResHeaderSuffix *Suffix = nullptr;

llvm/lib/Object/WindowsResource.cpp

if (Source.getBufferSize() < WIN_RES_MAGIC_SIZE + WIN_RES_NULL_ENTRY_SIZE)
     return make_error<GenericBinaryError>(
         "File too small to be a resource file",
         object_error::invalid_file_type);
   std::unique_ptr<WindowsResource> Ret(new WindowsResource(Source));
   return std::move(Ret);
 }
 
 Expected<ResourceEntryRef> WindowsResource::getHeadEntry() {
-  Error Err = Error::success();
-  auto Ref = ResourceEntryRef(BinaryStreamRef(BBS), this, Err);
-  if (Err)
-    return std::move(Err);
-  return Ref;
+  if (BBS.getLength() < sizeof(WinResHeaderPrefix) + sizeof(WinResHeaderSuffix))
+    return make_error<EmptyResError>(".res contains no entries",
+                                     object_error::unexpected_eof);
+  return ResourceEntryRef::create(BinaryStreamRef(BBS), this);
 }
 
 ResourceEntryRef::ResourceEntryRef(BinaryStreamRef Ref,
-                                   const WindowsResource *Owner, Error &Err)
-    : Reader(Ref), OwningRes(Owner) {
-  if (loadNext())
-    Err = make_error<GenericBinaryError>("Could not read first entry.\n",
-                                         object_error::unexpected_eof);
+                                   const WindowsResource *Owner)
+    : Reader(Ref), OwningRes(Owner) {}
+
+Expected<ResourceEntryRef>
+ResourceEntryRef::create(BinaryStreamRef BSR, const WindowsResource *Owner) {
+  auto Ref = ResourceEntryRef(BSR, Owner);
+  if (auto E = Ref.loadNext())
+    return std::move(E);
+  return Ref;
 }
 
 Error ResourceEntryRef::moveNext(bool &End) {
   // Reached end of all the entries.
   if (Reader.bytesRemaining() == 0) {
     End = true;
     return Error::success();
   }
Error ResourceEntryRef::loadNext() {
 
   return Error::success();
 }
 
 WindowsResourceParser::WindowsResourceParser() : Root(false) {}
 
 Error WindowsResourceParser::parse(WindowsResource *WR) {
   auto EntryOrErr = WR->getHeadEntry();
-  if (!EntryOrErr)
-    return EntryOrErr.takeError();
+  if (!EntryOrErr) {
+    auto E = EntryOrErr.takeError();
+    if (E.isA<EmptyResError>()) {
+      consumeError(std::move(E));
+      return Error::success();
+    }
+    return E;
+  }
 
   ResourceEntryRef Entry = EntryOrErr.get();
   bool End = false;
   while (!End) {
     Data.push_back(Entry.getData());
 
     bool IsNewTypeString = false;
     bool IsNewNameString = false;