Page MenuHomeFreeBSD

Correctly determine the real executable in crunched binaries
ClosedPublic

Authored by arichardson on Aug 7 2020, 5:20 PM.
Tags
None
Referenced Files
F103023150: D25998.diff
Tue, Nov 19, 10:58 PM
Unknown Object (File)
Fri, Nov 15, 3:59 AM
Unknown Object (File)
Oct 19 2024, 2:55 PM
Unknown Object (File)
Oct 3 2024, 9:10 AM
Unknown Object (File)
Oct 2 2024, 6:18 PM
Unknown Object (File)
Sep 27 2024, 5:44 AM
Unknown Object (File)
Sep 6 2024, 2:29 AM
Unknown Object (File)
Sep 5 2024, 7:12 PM
Subscribers

Details

Summary

This should fix cases like su setting argv[0] to _su for /bin/sh.
Previously cheribsdbox (a crunched tool we use in CheriBSD to reduce the
size of our minimal disk images to allow loading them onto FPGAs without
waiting forever for the transfer) would complain about _su not being
compiled in, but now that we also look at AT_EXECPATH it correctly
invokes the sh tool.

Not we use use AT_EXECPATH instead of the KERN_PROC_PATHNAME sysctl to get
the crunchgen binary name since it seems like KERN_PROC_PATHNAME just
returns the last cached path for a given hardlink.
When using su, instead of invoking /bin/csh it would invoke the last
used hardlink to cheribsdbox. This caused weird test failures when running
tests due to id being executed instead of echo:

$ id # id is a hardlink to /bin/cheribsdbox
$ su postgres -c 'echo 1' # su is also a hardlink
uid=1001(postgres) gid=1001(postgres) groups=1001(postgres)

Obtained from: CheriBSD

Test Plan

similar version of this patch has been in use in CheriBSD for a long time.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

arichardson created this revision.

Seems OK to me

usr.sbin/crunch/crunchgen/crunched_main.c
122 ↗(On Diff #75581)

The :\n seems odd, I would expect no newline

This revision is now accepted and ready to land.Aug 10 2020, 3:05 PM
usr.sbin/crunch/crunchgen/crunched_main.c
122 ↗(On Diff #75581)

The :\n seems odd, I would expect no newline

That was a bug caused by me converting the CheriBSD fprintf() to use warnc(). Will fix.

Re-indenting should be done in a separate commit if at all.

usr.sbin/crunch/crunchgen/crunched_main.c
108 ↗(On Diff #75581)

Fair point from @brooks about indentation, I glossed over it.
I'd argue that we should fix crunched_main.c whitespace and other style(9) issues first then apply this change on top.

Re-indenting should be done in a separate commit if at all.

I did it since I was touching almost every line anyway. I'll submit a new review that re-indents first.

IMO you could just commit the reindent first without going through phabricator

arichardson marked 2 inline comments as done.
  • rebased on top of reformatting commit
  • address review comments
  • added my copyright since most of the file has been rewritten
This revision now requires review to proceed.Aug 12 2020, 3:57 PM
usr.sbin/crunch/crunchgen/crunched_main.c
156 ↗(On Diff #75733)

The : shouldn't be required at all. The manpage says:

The err(), errc(), verr(), verrc(), warn(), warnc(), vwarn(), and
vwarnc() functions append an error message obtained from strerror(3)
based on a supplied error code value or the global variable errno,
preceded by another colon and space unless the fmt argument is NULL.
This revision is now accepted and ready to land.Aug 14 2020, 11:43 PM