Page MenuHomePhabricator

Fix bug 34051 by handling empty .res files gracefully.

Authored by ecbeckmann on Aug 22 2017, 5:39 PM.



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

Diff Detail


Event Timeline

ecbeckmann created this revision.Aug 22 2017, 5:39 PM

Remove unnecessary braces

ruiu added inline comments.Aug 22 2017, 6:21 PM
92 ↗(On Diff #112275)

What is ID?

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 marked 2 inline comments as done.

Make factory function instead of having constructor fail.

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.

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 accepted this revision.Aug 23 2017, 5:24 PM


137 ↗(On Diff #112275)

Please add that as a comment.

This revision is now accepted and ready to land.Aug 23 2017, 5:24 PM

Add comment

This revision was automatically updated to reflect the committed changes.