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 ↗

(On Diff #112275)

What is ID?

llvm/lib/Object/WindowsResource.cpp

74 ↗

(On Diff #112275)

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.)

137 ↗

(On Diff #112275)

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 ↗

(On Diff #112275)

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

137 ↗

(On Diff #112275)

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

137 ↗

(On Diff #112275)

Please add that as a comment.

[ecbeckmann]

Add comment

llvm/trunk/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/trunk/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>()) {
+      // Check if the .res file contains no entries.  In this case we don't have
+      // to throw an error but can rather just return without parsing anything.
+      // This applies for files which have a valid PE header magic and the
+      // mandatory empty null resource entry.  Files which do not fit this
+      // criteria would have already been filtered out by
+      // WindowsResource::createWindowsResource().
+      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;