Page MenuHomeFreeBSD

xinstall: fix invocation of llvm-strip
ClosedPublic

Authored by mhorne on Jun 2 2021, 3:14 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sep 28 2024, 11:20 AM
Unknown Object (File)
Sep 28 2024, 11:18 AM
Unknown Object (File)
Sep 28 2024, 9:24 AM
Unknown Object (File)
Sep 9 2024, 8:30 PM
Unknown Object (File)
Sep 8 2024, 4:18 AM
Unknown Object (File)
Sep 7 2024, 7:42 PM
Unknown Object (File)
Sep 7 2024, 1:14 PM
Unknown Object (File)
Sep 2 2024, 2:18 PM
Subscribers

Details

Summary

When executing strip(1), '--' is passed as an argument to explicitly
terminate the getopt(3) loop. The option parsing in llvm-strip doesn't
support this however, so setting XSTRIPBIN=llvm-strip results in an
unsupported argument error. llvm-strip(1) is otherwise
commandline-compatible with our strip(1), so just use the documented
argument format that is common to both.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mhorne requested review of this revision.Jun 2 2021, 3:14 PM

Surprised I haven't seen this, pretty sure I've built with XSTRIPBIN=llvm-strip before.

This revision is now accepted and ready to land.Jun 2 2021, 5:13 PM

Looking at git blame it seems like the "--" was added in D25551, so I probably tested before that was committed.

Surprised I haven't seen this, pretty sure I've built with XSTRIPBIN=llvm-strip before.

I should clarify that this doesn't cause the build to fail, only that the warnings are quickly printed and the binaries will be left un-stripped. It becomes more obvious when running make -s XSTRIPBIN=llvm-strip buildworld.

The -- was added so that source pathnames starting with - would not break. I guess destination pathnames starting with - already did not work before D25551, so that was not a regression. This could be fixed differently by prepending ./ if the name starts with -.

Handle from_name beginning with '-'.

This revision now requires review to proceed.Jun 3 2021, 4:58 PM
jilles requested changes to this revision.Jun 8 2021, 9:22 PM
jilles added inline comments.
usr.bin/xinstall/xinstall.c
1336–1338

The extra semicolon means it will return here always, and never run strip with the modified path.

Also, snprintf() will not fail if the buffer is too small. Instead, it will write a truncated string and return the full length.

An alternative to a fixed-size buffer could be heap allocation using asprintf(); this is not limited to a fixed length, but the resulting string needs to be freed afterward.

This revision now requires changes to proceed.Jun 8 2021, 9:22 PM

I agree that snprintf() should be avoided here, as well as stack abuse. Please consider using asprintf() that allocated memory on heap all by itself dealing with space requirements automatically, just free the pointer just after posix_spawnp().

Use asprintf() to allocate _from_name.

Thank you both for the review. I was rushing a bit with the previous revision, and it shows. I've tested the success case for installing and stripping a file beginning with '-'. I'll look into adding a test for this case to install_test.sh.

usr.bin/xinstall/xinstall.c
1318

Perhaps name this variable something like prefixed_from_name.

1352

Calling free(NULL) need not be prevented.

Address jilles' comments.

usr.bin/xinstall/xinstall.c
1336

I'll note that this now goes slightly over 80 columns, but I found a linebreak here to be less legible.

Any remaining concerns with this change?

This revision was not accepted when it landed; it landed in state Needs Review.Aug 30 2021, 2:57 PM
This revision was automatically updated to reflect the committed changes.