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

Fix bug GH-11246 cli/get_set_process_title initialisation failure on MacOS #11247

Closed
wants to merge 3 commits into from

Conversation

lucasnetau
Copy link
Contributor

Fixes #11246, when executing on MacOS it was found that the environ area my not be contiguous.

Checking the original Postgres implementation https://github.com/postgres/postgres/blob/master/src/backend/utils/misc/ps_status.c the PHP code fails if either argv or environ is non-contiguous whereas the original code only fails if argv is ever non-contiguous.

Additionally the end_of_area variable is changed even if we hit a non-contiguous environ section, another divergence from the original code

Fail to clobber_error only when the argv is a non-contiguous area
Don't increment the end_of_error if a non-contiguous area is encountered in environ
@lucasnetau

This comment was marked as outdated.

@lucasnetau
Copy link
Contributor Author

lucasnetau commented May 16, 2023

A separate patch is needed to apply to PHP8.2 and master due to commit b468d6f which change int to bool and inverted some logic with a variable rename. If this PR is accepted should I raise a second one against PHP8.2?

@lucasnetau
Copy link
Contributor Author

@Girgias do you have any idea why MACOS_DEBUG_NTS is failing? I cannot replicate locally, however I am building on macOS 13, whereas CI is using macOS 11

@Girgias
Copy link
Member

Girgias commented May 20, 2023

Sorry I don't, and I don't have access to a MacOS machine. Maybe @bwoebi knows as IIRC they have access to such a machine.

@lucasnetau
Copy link
Contributor Author

Issue found using ASAN in the runner. I've pushed a commit to this PR and #11273

@lucasnetau
Copy link
Contributor Author

@Girgias could you please approve the workflow again (and for PR #11273) ? I'm not sure who to add as a reviewer for CLI, no one listed under Code Owners

@lucasnetau
Copy link
Contributor Author

WINDOWS_X64_NTS failed with a transient network issue

fatal: unable to access 'https://github.com/php/php-sdk-binary-tools.git/': Failed to connect to github.com port 443 after 21079 ms: Couldn't connect to server

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Please verify if my understanding is correct.

environ points to the strings after argv, and should be contiguous at the start of the program if the environment hasn't been modified yet, or new environment variables were added. We assume string that aren't contiguous don't belong to the initial environment strings and may not be used for the process title (ps_buffer_size). So it seems the space for the title on macOS might be slightly limited, but by how much is not clear without testing.

It also seems that the strdup below for strings that aren't part of ps_buffer_size is useless because it may not be written over by set_process_title, but we shouldn't change that in a patch.

Your patch looks good to me. It would be interesting to know why the environment strings are non-contiguous on macOS, and whether these strings live somewhere else or whether there's some empty space between environment strings. I'll test this later today on my MacBook.

sapi/cli/ps_title.c Outdated Show resolved Hide resolved
@lucasnetau
Copy link
Contributor Author

@iluuu1994, I tried to find out before but came up with nothing when building my own small c program, your comment made me go back and take a look with some debugging in the php-src, here is what I found:

On macOS when the environ is non-contiguous there is an additional variable seen injected after the expected last _ variable when executing from a bash prompt, that variable being __CF_USER_TEXT_ENCODING.

Going through the Apple technote (https://developer.apple.com/library/archive/technotes/tn2228/_index.html) __CF_USER_TEXT_ENCODING is being injected by Core Foundation. It only appears when linked against that with -framework CoreServices

0x7ff7b6351ed0: _=sapi/cli/php
**NCONTIG** 0x6040000012d0: __CF_USER_TEXT_ENCODING=0x1F5:0:15

So the risk of limited space appears to be no less than any other platform.

A quick reproduction that needs to be compiled with -framework CoreServices flag

#include <unistd.h>
#include <stdio.h>
#include <string.h>

extern char **environ;

int main(int argc, char *argv[])
{
        char* end_of_area = NULL;
        int non_contiguous_area = 0;
        int i;

        /*
         * check for contiguous argv strings
         */
        for (i = 0; (non_contiguous_area == 0) && (i < argc); i++)
        {
            if (i != 0 && end_of_area + 1 != argv[i])
                non_contiguous_area = 1;
            end_of_area = argv[i] + strlen(argv[i]);
        }

        for (i = 0; environ[i] != NULL; i++)
        {
            
            if (end_of_area + 1 == environ[i]) {
            	end_of_area = environ[i] + strlen(environ[i]);
                printf("%p: %s\n", environ[i], environ[i]);
            } else {
                printf("**NCONTIG** %p: %s\n", environ[i], environ[i]);
            }
        }

}

@iluuu1994 iluuu1994 closed this in c6ae7a5 May 31, 2023
@iluuu1994
Copy link
Member

@lucasnetau Thanks for the explanation! This looks good to me then, thanks for the patch!

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

Successfully merging this pull request may close these issues.

None yet

3 participants