Page MenuHomePhabricator

[Object] add isExecutableObject member function
AbandonedPublic

Authored by abrachet on Jun 3 2019, 8:29 PM.

Details

Summary

added object::ObjectFile::isExecutableObject() which checks from an ObjectFiles header whether it is marked as executable.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2019, 8:29 PM

Hi @abrachet - you forgot to include context with this diff, so it's somewhat harder to review as is. Please update with context.

What is the purpose of this function? Do you have a use-case for it yet?

You should also add some unit tests for this. I don't think it would be too hard, but you might need to create some new unit test files.

llvm/include/llvm/BinaryFormat/XCOFF.h
25

Missing trailing full stop.

llvm/lib/Object/WasmObjectFile.cpp
1540

Could this just be !isRelocatableObject() && !isSharedObject()? I don't know anything about wasm though, so this might not make sense.

sbc100 added inline comments.Jun 4 2019, 9:31 AM
llvm/lib/Object/WasmObjectFile.cpp
1540

Yes that sounds correct to me.

abrachet updated this revision to Diff 203038.Jun 4 2019, 3:22 PM
abrachet marked 3 inline comments as done.Jun 4 2019, 3:26 PM

I felt this would be a useful change because in llvm-objcopy I am planning on using isExecutableObject() when deciding on output file permissions. I also think the change gives more continuity in the ObjectFile class, which already has the isRelocatableObject() method.

The code change looks fine now, but there's still no context in the diff.

Are you going to add unit tests too?

rupprecht added inline comments.Jun 6 2019, 9:51 AM
llvm/include/llvm/BinaryFormat/XCOFF.h
33

This is 0x0100, not 0x1000

abrachet updated this revision to Diff 203504.Jun 7 2019, 1:05 AM

Added test case

abrachet marked 2 inline comments as done.Jun 7 2019, 1:07 AM
abrachet added inline comments.
llvm/include/llvm/BinaryFormat/XCOFF.h
33

Good catch had an extra 0 at the end there, thanks!

rupprecht added inline comments.Jun 7 2019, 9:38 AM
llvm/unittests/Object/CMakeLists.txt
10

nit: I'd put this in a file called ELFObjectFileTest.cpp. Usually unit test files have a 1:1 correspondence with the file they're testing, and individual test cases for the methods/scenarios they're testing.

llvm/unittests/Object/IsExecutableTest.cpp
31 ↗(On Diff #203504)

It'd be good to parameterize (i.e. with a loop) this on other ELF types too: ET_NONE, ET_DYN, ET_CORE, and maybe a few non-enum types (pick an arbitrary number between ET_LOPROC and ET_HIPROC).

Note: within a loop, you can use stream logging w/ ASSERT_*/EXPECT_* macros to print the type value if the assertion fails (since gtest won't print it otherwise), e.g.

for (...) {
  Header.e_type = ElfType;
  ...
  EXPECT_FALSE(ObjFileOrErr.get()->isExecutableObject()) << " for " << ElfType;
}
38–39 ↗(On Diff #203504)

For ASSERT (which aborts the test case) vs EXPECT (which causes failure but continues with the test) in gtest, you usually want to use ASSERT only when the condition being false will cause fatal errors later, and EXPECT when the test can continue to check other things.

ASSERT_TRUE(!!ObjFileOrErr) is a good check because calling ObjFileOrErr.get()->isExecutableObject() on the next line would (probably) cause the test to crash anyway (I think ObjFileOrErr.get() would be nullptr in that case?)
However, ASSERT_FALSE(ObjFileOrErr.get()->isExecutableObject()) should be EXPECT_FALSE instead -- it's not a fatal error, and you might want to see what the result is when you change the type to ET_EXEC. (e.g. maybe that will also fail because the check is backwards, or maybe it will pass because isExecutableObject() always returns true, etc.)

abrachet updated this revision to Diff 203672.Jun 7 2019, 11:03 PM
abrachet marked an inline comment as done.

Updated test case

jhenderson added inline comments.Jun 10 2019, 3:06 AM
llvm/unittests/Object/ObjectFileTest.cpp
18–19

Do you need both these headers? Note that in LLVM, the standard is to only include the headers that are actually required.

48

Nit: capital letter at start of comment.

49–52

You probably only need one of these, but you should also add a value between LOOS and HIOS.

You should probably even add an unused value from the standard range (e.g. ET_CORE + 1), or similar.

110

',' -> '.'

111

Change the rest of the comment to:
"They don't make a copy, so there's no reason to call ObjectFile::createObjectFile every time."

121–124

Sorry, I don't understand this?

163–164

Delete the bit after the last comma.

166

Add "files" after "MachO". Add a full stop at the end of this line.

168–169

Delete the last sentence. I don't think it needs explaining in the test.

180

Why the double '!'? Does ASSERT_TRUE not work directly on ObjectFileOrErr?

abrachet marked 4 inline comments as done.Jun 11 2019, 12:12 AM
abrachet added inline comments.
llvm/unittests/Object/ObjectFileTest.cpp
18–19

I do use both std::function and std::vector. I suppose that std::vector was not 100% required, but it makes things somewhat cleaner I would say. Would you have a strong preference that I take it out?

121–124

I probably over complicated things with this entire function. A lot of work to not copy and paste. The idea behind these lines was that although most likely it would be tested in the upper loop, this explicitly tests for example if a ObjectFile::isExecutableObject() is true when ET_EXEC is set as its type. I say this isn't mandatory because ET_EXEC would have already been tested going through the loop on 112 as it is in ELF_ETypes.

180

Seems like gtest doesn't play well with operator bool? I'm not really sure but it didn't work for me without the manual bool conversion. They explicitly mention this in the ErrorOr unit tests, actually: https://github.com/llvm/llvm-project/blob/master/llvm/unittests/Support/ErrorOrTest.cpp#L23

jhenderson added inline comments.Jun 11 2019, 2:58 AM
llvm/unittests/Object/ObjectFileTest.cpp
18–19

See https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible. So, if one of the headers includes them, my understanding is that you don't need to explicitly mention them in the source too.

121–124

Yes, exactly, you don't need this block, because you are already testing it above. I don't think you need a separate test that does the same thing.

Sometimes a little copy-pasting might make it easier to read, just be careful how much. In this case, I don't think you need to.

abrachet updated this revision to Diff 204321.Jun 12 2019, 10:10 AM
abrachet marked 9 inline comments as done.
abrachet added inline comments.
llvm/unittests/Object/ObjectFileTest.cpp
49–52

Which LOOS and HIOS should I use in the context of e_type?

abrachet marked an inline comment as done.Jun 12 2019, 10:28 AM
abrachet added inline comments.
llvm/unittests/Object/ObjectFileTest.cpp
18–19

I don’t personally interpret this in the same way you do. It seems to me that is mainly targeting when a forward declaring would suffice which is not the case here. I’m sure that one of the headers that I have included itself includes vector, but the compile time performance cost mentioned in that standard I would guess isn’t concerned with the cost of an ifdef. Also, if one of those files no longer needs vector then the build breaks. I wrote a lot more than is representative of how much I care. I’m not putting my foot down or anything, it’ll gladly remove the include if you still want

rupprecht added inline comments.Jun 12 2019, 1:00 PM
llvm/unittests/Object/ObjectFileTest.cpp
24

For these create*Header methods, it looks like these can all create their own header variables instead of making the caller pass a blank one in?

93–118

This is a bit too clever; it looks great but it makes it harder to read. Tests, even more than code, should be simple to read to make sure the test is actually doing the right thing. Otherwise, if it gets too complex, you have to start writing tests for the test itself :)

I think a good way to make it simpler would be to break it apart, e.g. one that reinterprets to an ObjectFile, and break apart the true/false types into different loops (avoiding the need for KnownTrue), like:

template <typename header_t>
static void createObjectFile(header_t Header, file_magic magic = file_magic::unknown) {
  StringRef Bytes(reinterpret_cast<const char *>(&Header), sizeof(Header));
  auto ObjFileOrErr =
      ObjectFile::createObjectFile(MemoryBufferRef(Bytes, ""), magic);
  ASSERT_TRUE(!!ObjFileOrErr) << "invalid object file format";

  return *ObjFileOrErr.get();
}

TEST(IsExecutableObject, ELF) {
  ELF64LE::EHdr Header = createGenericELFHeader();
  ObjectFile Obj = createObjectFile(Header);
  for (auto ElfType : { ELF::ET_NONE, ELF::ET_REL, ... }) {
    Obj.Header = ElfType;
    EXPECT_FALSE(Obj.isExecutable());
  }
  // Same for EXPECT_TRUE
}

Depending on how you refactor, you might also want to put these into a test fixture (i.e. create a test class and use TEST_F), though I'm not sure there's any data to share (the usual reason for creating a test fixture).

jhenderson added inline comments.Jun 13 2019, 1:41 AM
llvm/unittests/Object/ObjectFileTest.cpp
18–19

I agree with basically everything you just said, but ultimately I'm going on what common practice seems to be. I really don't care that much, and would happily back bringing this up on the mailing list as an inconsistency in how the coding standard is applied etc, with a suggestion to either change the standard or seeking agreement to start pushing more concretely for it.

Also, if one of those files no longer needs vector then the build breaks.

This is not really an issue to worry about, since people should build their code before committing anyway.

49–52

Any arbitrary value between ET_LOOS and ET_HIOS.

Reference in case you need it: http://www.sco.com/developers/gabi/latest/ch4.eheader.html

110

Missing trailing full stop.

abrachet updated this revision to Diff 204588.Jun 13 2019, 11:17 AM

Made tests simpler.

abrachet marked 11 inline comments as done.Jun 13 2019, 11:22 AM
abrachet added inline comments.
llvm/unittests/Object/ObjectFileTest.cpp
49–52

Thanks, I was confused because these values were not in BinaryFormat/ELF.h, I have added them.

93–118

Yup the classic saved a few minutes of copy and pasting for a couple of hours fighting the compiler. You should have seen how bad it was before :)

jhenderson added inline comments.Jun 14 2019, 3:01 AM
llvm/unittests/Object/ObjectFileTest.cpp
77

Don't use first person in comments. If you feel you need a comment, rephrase this as "Omit a scope..." etc, or "This macro omits a scope creating..."

It would also be easier to read this comment if you surround code snippets with quotation marks.

80

What do they return? What's stopping your function returning something and then asserting in the code on that? E.g:

std::string my_function(bool b) {
  if (b)
    return "";
  return "something went wrong";
}

TEST(a_test) {
  auto result = my_function(somecall());
  ASSERT_TRUE(result.empty()) << result;
  ...
}
rupprecht added inline comments.Jun 14 2019, 11:44 AM
llvm/unittests/Object/ObjectFileTest.cpp
81

Why a macro? Can you create a templated method instead?

abrachet updated this revision to Diff 204894.Jun 14 2019, 10:00 PM
abrachet marked 2 inline comments as done.

Use function instead of macro in unit test.

abrachet updated this revision to Diff 204895.Jun 14 2019, 10:02 PM

Added period at the end of comment.

abrachet updated this revision to Diff 204897.Jun 14 2019, 10:07 PM
abrachet marked 3 inline comments as done.

Ran clang-format

jhenderson accepted this revision.Jun 17 2019, 9:23 AM

Couple of minor nits, but otherwise LGTM.

llvm/unittests/Object/ObjectFileTest.cpp
78

Can Header be a const &? Also, I think header_t should follow standard LLVM type naming (i.e. UpperCamelCase, e.g. HeaderType).

This revision is now accepted and ready to land.Jun 17 2019, 9:23 AM

(just the one comment, then lgtm)

llvm/unittests/Object/ObjectFileTest.cpp
84

This should just be returned, i.e. the method should have a signature:

template <typename HeaderType>
std::unique_ptr<ObjectFile> createObjectFile(const HeaderType &Header, file_magic Magic = file_magic::unknown)

(+1 to using the name suggestion from James)

abrachet marked an inline comment as done.Jun 17 2019, 11:40 AM
abrachet added inline comments.
llvm/unittests/Object/ObjectFileTest.cpp
84

If I returned the unique_ptr then I couldn’t have the ASSERT_TRUE which returns void on error. I would like to keep it, but it isn’t totally necessary I suppose.

rupprecht accepted this revision.Jun 17 2019, 11:51 AM
rupprecht marked an inline comment as done.

LGTM just with the naming nit from James (header_t&->const HeaderType&)

llvm/unittests/Object/ObjectFileTest.cpp
84

Oh, yeah. I knew there was a weird gotcha with ASSERT_TRUE, but couldn't remember specifically. :(

Using a test fixture would workaround this issue; e.g. the test class would hold an empty std::unique_pt<ObjectFile> ObjPtr that the test method would initialize (and would not assign as an out-parameter as you have here). But I think this is fine.

abrachet updated this revision to Diff 205220.Jun 17 2019, 5:10 PM

Refactored unit test.

abrachet marked 3 inline comments as done.Jun 17 2019, 5:11 PM
abrachet added inline comments.
llvm/unittests/Object/ObjectFileTest.cpp
84

How does this look now?

jhenderson added inline comments.Jun 18 2019, 3:36 AM
llvm/unittests/Object/ObjectFileTest.cpp
47

You've marked this as private (implicitly) here, but it's protected in the base class. Is that intended?

63

Do you really need to make the constructor protected?

152

I wonder if it would make more sense to fold this into the test case above? i.e. the check becomes:

if (Type == MachO::MH_EXECUTE) {
  EXPECT_TRUE(ObjPtr->isExecutableObject());
  EXPECT_FALSE(ObjPtr->isRelocatableObject());
} else if (Type == MachO::MH_OBJECT) {
  EXPECT_FALSE(ObjPtr->isExecutableObject());
  EXPECT_TRUE(ObjPtr->isRelocatableObject());
} else { // Don't know if this is really necessary or not, but you get the idea
  EXPECT_FALSE(ObjPtr->isExecutableObject());
  EXPECT_FALSE(ObjPtr->isRelocatableObject());
}

On the other hand, if you think the separate test-cases is easier to read, I'm okay with it.

Same comment applies to ELF.

MaskRay added inline comments.
llvm/include/llvm/Object/ELFObjectFile.h
1203

Some people will be surprised if ET_DYN is not considered as executable object.

Position-Independent Executables use ET_DYN.

abrachet marked 4 inline comments as done.Jun 18 2019, 9:44 AM
abrachet added inline comments.
llvm/include/llvm/Object/ELFObjectFile.h
1203

$ llvm-readelf which llvm-readelf -h
...
Type: DYN (Shared object file)

Oh yeah, good catch, thank you. How would you write this method? Does an executable always have e_entry as non zero? Is there ever a case where execve(2) will not fail if e_entry is 0?

Also, should I worry about this? https://github.com/freebsd/freebsd/blob/master/sys/kern/imgact_elf.c#L1135 I don't know enough about ELF yet on this one. Thanks.

llvm/unittests/Object/ObjectFileTest.cpp
47

Well createGenericHeader is only ever called by SetUp in TestBase, it isn't necessary to be called outside of the class.

152

Hmm, I think that the current way is better for when a failure occurs because the output will be more clear on which test it failed.

abrachet marked an inline comment as done.Jun 18 2019, 10:45 AM

Would it make sense to add something like this? It might catch a case I haven't thought of yet on obscure systems (assuming we end up changing this to do more than just checking e_type and e_entry, otherwise this won't catch much), assuming build bots are running tests on such systems.

if (!sys::fs::exists("/usr/bin/true") {
    // Try other paths?
    // If it doesn't exist then just return.
}

// Ensure it isn't a symlink, I think we don't deal with these currently?

auto ObjFile = createObjectFile(Path);
ASSERT_TRUE(ObjFile->isExecutableObject());

Also, for more reference this is how elf executables are loaded by execve in Linux I believe. Obviously there is more to check than just e_type, e_entry or program headers but how much is reasonable? Is it worth commenting in ObjectFile::isExecutableObject() that a false return may not necessarily mean the OS can't load the object as an executable?

MaskRay added inline comments.Jun 18 2019, 11:24 PM
llvm/include/llvm/Object/ELFObjectFile.h
1203

Do you have a use case (not a unittest) of this member function?

If you have one, let's see if relocatable/executable/shared should be trichotomy or not.

Trichotomy doesn't work in ELF. One may argue a PIE doesn't have PT_INTERP but a DSO doesn't. However, a static pie doesn't have PT_INTERP, either. Its difference from a DSO is if DT_DEBUG exists. However, DT_DEBUG can be disabled in lld with -z rodynamic (Fuchsia people do so)...

Also, should I worry about this? https://github.com/freebsd/freebsd/blob/master/sys/kern/imgact_elf.c#L1135

We should not be bothered by FreeBSD Brandinfo

abrachet marked an inline comment as done.Jun 18 2019, 11:33 PM
abrachet added inline comments.
llvm/include/llvm/Object/ELFObjectFile.h
1203

Do you have a use case (not a unittest) of this member function?

Yes, D62718 here I have a change which tries to mirror gnu objcopy's handling of output file permissions which is dependent on the file being executable or not.

Thanks for the help.

MaskRay added inline comments.Jun 19 2019, 1:17 AM
llvm/include/llvm/Object/ELFObjectFile.h
1203

Replied on D62718. The idea to use isExecutableObject() to set file permissions looks very bad to me. That would bring more issues than it tried to solve. It is a good example that copying every corner case of GNU objcopy is harmful.

If D62718 is the only use case, I suggest we wait until a more reasonable use case arises.

abrachet abandoned this revision.Jun 19 2019, 4:59 PM

This method is no longer needed, D62718 will be going in a new direction.