Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

programs/cp: Fix pointer cast warning #356

Merged

Conversation

neverpanic
Copy link
Member

execve() expects the args to be in writable memory, so copy them using strdup(3).

execve() expects the args to be in writable memory, so copy them using
strdup(3).
@neverpanic neverpanic requested a review from jmroot November 18, 2024 19:21
@neverpanic neverpanic merged commit 3861815 into macports:master Dec 6, 2024
5 checks passed
@neverpanic neverpanic deleted the cal-fix-pointer-cast-warning branch December 6, 2024 21:13
@saagarjha
Copy link
Contributor

(Note: drive-by comment)

execve() expects the args to be in writable memory,

It doesn't, actually: https://pubs.opengroup.org/onlinepubs/9799919799/ (scroll down to the "Rationale" section). I would suggest just marking the variables as char * and letting it be rather than heap allocating them for no reason.

@neverpanic
Copy link
Member Author

execve() expects the args to be in writable memory,

It doesn't, actually: https://pubs.opengroup.org/onlinepubs/9799919799/ (scroll down to the "Rationale" section). I would suggest just marking the variables as char * and letting it be rather than heap allocating them for no reason.

Thanks, that's helpful. However, the macOS headers declare execve in /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/unistd.h as:

int  execve(const char * __file, char * const * __argv, char * const * __envp) __WATCHOS_PROHIBITED __TVOS_PROHIBITED;

This matches the third row in the table of the rationale section of https://pubs.opengroup.org/onlinepubs/9799919799/functions/exec.html. We also don't actually know whether Apple's implementation of execve(2) conforms to this spec — AFAIK Apple doesn't make any claims about macOS' compliance to POSIX.1-2024, and the manpage doesn't clarify either whether args can be in read-only memory, or whether it conforms to the standard.

So we either need to live with the compiler warning, silence the compiler warning, or add a cast. The allocation fixes the warning regardless of what Apple's implementation does, and the cost of two small allocations compared to the following execve(2) is negligible.

@raimue
Copy link
Member

raimue commented Dec 7, 2024

Couldn't you also allocate the static strings on the stack with char clone_arg[] = "-c";?

char *cp_path = strdup("/bin/cp");
char *clone_arg = strdup("-c");
char **new_argv = malloc(sizeof(char *) * (argc+2));
if (cp_path && clone_arg && new_argv) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting all into one condition will not work well with the perror() below. The errno could have been overwritten by the next library function.

This comment was marked as off-topic.

@saagarjha
Copy link
Contributor

This matches the third row in the table of the rationale section of https://pubs.opengroup.org/onlinepubs/9799919799/functions/exec.html.

That's the source, for a char *const[] destination we need to check the columns. That accepts char *[] as expected. Also, macOS is one of the few certified UNIX OSes, which means that it actually does need to conform to the standards. Not the 2024 version of course, but this one for SUSv3: https://pubs.opengroup.org/onlinepubs/009695399/functions/exec.html. It has the same language in it.

neverpanic added a commit that referenced this pull request Dec 8, 2024
We can use the stack instead. Thanks @raimue for spotting that.

See #356 (comment).
@neverpanic
Copy link
Member Author

Couldn't you also allocate the static strings on the stack with char clone_arg[] = "-c";?

Good catch, in 1b43b56.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants