Details
- Reviewers
kevans brooks kib emaste - Commits
- rGd8e841a9aa1f: libc: treat execvpe as a week symbol
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 63634 Build 60518: arc lint + arc unit
Event Timeline
See https://github.com/rizsotto/Bear/issues/557 for the issue that prompted this and particularly the comment https://github.com/rizsotto/Bear/issues/557#issuecomment-2776259708
I think that the proper fix in line with the existing code is to use namespace.h to mangle execvpe reference in libc.
The bear problem still fixed but we are unable to intercept the execvpe call as it is replaced with _execvpe.
I think you need to bring the __weak_reference line back from the original change.
Using _execvpe is consistent, but perhaps it should be __libc_execvpe. The general _<foo> model is a remnant of libc_r and should perhaps be removed at some point.
| lib/libc/gen/Symbol.map | ||
|---|---|---|
| 460 ↗ | (On Diff #153445) | You can't remove previously exported public symbols. |
| lib/libc/gen/exec.c | ||
| 301 | Remove | |
| lib/libc/include/libc_private.h | ||
| 358 | Should not be required. | |
| lib/libc/include/namespace.h | ||
| 64–65 ↗ | (On Diff #153445) | White space is wrong and touching the execve line is bug. More importantly, you're missing a matching un-namespace.h change. |
You still need to add a an #undef execvpe line to un-namespace.h and then you shouldn't need the libc_private.h entry. The point of namespace.h/un-namespace.h is that the declarations are impacted by the macro so the execvpe declaration in unistd.h becomes an __libc_execvpe declaration which you can then use.
| lib/libc/include/namespace.h | ||
|---|---|---|
| 65 ↗ | (On Diff #153542) | If you changed the original symbol name, why the namespacing is still kept in the patch? |
I think it is for consistency as all standard C library call point to the actual implementation in namespace.h?
No, you do not namespace execvpe, instead you use explicit internal symbol. So I do not think there is a point in changing namespace.h.
Sorry for the churn, but I think @kib is right and this should be declared in libc_private.h and not namespaced.
If someone want to help me merge this patch. Please use the following commit on github:
https://github.com/aokblast/freebsd-src/commit/fd54c46f55ebd500bd7bf2380bacd818b4784059
| lib/libc/include/libc_private.h | ||
|---|---|---|
| 358 | I think this should be above __libc_sigaction with no line break so they are alphabetical (modulo prefix) | |
Also update the github branch.
https://github.com/freebsd/freebsd-src/commit/57b03648208dde96d64c5a8a4b7bf204bdffcfdc.patch
| lib/libc/include/libc_private.h | ||
|---|---|---|
| 358 | Sorry, I use the same location as https://github.com/freebsd/freebsd-src/commit/8ccd0b876e67fda6249f294ff484798cc1e1569f#diff-aca98fe8d8694a2cf3df0ece911f284ef0a281ee095de3621d2a9337c16a5226. Fix it right now. | |
LGTM with @brooks's comment incorporated. I can grab the change from GitHub, fold in that change, and push.
there are some typo Sponrored->Sponsored in commit message. @emaste Could you please help me to fix it when committing?
Yeah, I already spotted and corrected that. I was editing the message to add kib to the reviewed-by list before noticing that it was already there.