[PATCH] D62718: [llvm-objcopy] Change output file permissions to be the same as the input file

Jordan Rupprecht via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 4 16:05:13 PDT 2019


rupprecht requested changes to this revision.
rupprecht added a comment.
This revision now requires changes to proceed.

Thanks for noticing this bug! I think the issue is a little more subtle (I didn't notice it until I really started digging into it), see my last comment especially. I can also include that one in the bug.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/same-permissions.test:3
+# RUN: llvm-objcopy %p/Inputs/alloc-symtab.o %t
+# RUN: stat --printf=%a %p/Inputs/alloc-symtab.o > %t1
+# RUN: stat --printf=%a %t > %t2
----------------
Another (probably) more portable option is `ls -l | cut -f 1 -d ' '`. But if you can get `stat --format` working (and remove the `system-linux` requirement), that's fine too.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:209
   sys::fs::file_status Stat;
-  if (Config.PreserveDates)
+  if (Config.InputFilename != "-")
     if (auto EC = sys::fs::status(Config.InputFilename, Stat))
----------------
We should actually have a check for PreserveDates w/ `-` as a filename (for input or output) that gives a more useful error (like `--preserve-dates requires a file`):

```
$ llvm-objcopy -p /tmp/foo.o /tmp/bar.o
$ llvm-objcopy -p - /tmp/bar.o < /tmp/foo.o
llvm-objcopy: error: '-': No such file or directory
```

Doesn't have to be this patch tho.

Side note: GNU objcopy appears to treat `-` as literal filenames.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:240
       return E;
     if (!Config.SplitDWO.empty())
       if (Error E = restoreDateOnFile(Config.SplitDWO, Stat))
----------------
We should copy the same permissions to the split dwo file too

Instead of creating a separate block below as you now have, I think it makes sense to refactor this a tiny bit, e.g. change `restoreDateOnFile` to a more general `restoreStatOnFile` method (that takes a `PreserveDates` arg)

Actually... see my comment below, I think setting based on an initial stat is not actually the right way to go


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:244
   }
+  if (Config.OutputFilename != "-")
+    if (std::error_code E = 
----------------
abrachet wrote:
> jhenderson wrote:
> > Nit: this should probably have a blank line before it.
> > 
> > Also, are there any test cases that have llvm-objcopy write to stdout/read from stdin? If not, it would probably be worth adding one, possibly in a separate patch.
> A quick grep would suggest there are only tests which use - for stdout and none of these are explicitly testing this. Good idea! I have added a test case here https://reviews.llvm.org/D62817
> 
> Also, do you have an opinion on how permissions should be decided when reading from stdin? I think 0664 | executableFile() ? 0111 : 0. So we just default to 0664 and then add execute bits if the header says it is executable.
> I think 0664 | executableFile() ? 0111 : 0
I think what we'd want is 0666 for object files, 0777 for executables, but respecting the umask, which I *think* means you either need to figure out whether it's an executable when you create the FileBuffer (and let the OS take care of handling the umask), or manually handle the umask when restoring stat.

example w/ GNU objcopy:

```
$ umask 027
$ chmod 0777 /tmp/foo.o
$ objcopy /tmp/foo.o /tmp/bar.o
$ stat --format='%A %n' /tmp/foo.o /tmp/bar.o
-rwxrwxrwx /tmp/foo.o
-rwxr-x--- /tmp/bar.o
```

Even though the input file has full r/w/x permissions, the output has the umask applied


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:247
+      sys::fs::setPermissions(Config.OutputFilename, Stat.permissions()))
+      return errorCodeToError(E);
 
----------------
This should additionally be wrapped in a file error w/ `Config.OutputFilename` as the filename


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62718/new/

https://reviews.llvm.org/D62718





More information about the llvm-commits mailing list