Skip to content

Add a configure probe for setenv() and enable PERL_USE_SAFE_PUTENV on the platforms that have it #19514

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

Merged
merged 8 commits into from
May 28, 2022

Conversation

xenu
Copy link
Member

@xenu xenu commented Mar 10, 2022

Fixes #19399

@xenu
Copy link
Member Author

xenu commented Mar 10, 2022

PS. Apart from the usual platforms, I've tested this change on AIX and HP-UX. There were no problems.

xenu added a commit to xenu/metaconfig that referenced this pull request Mar 10, 2022
@leonerd
Copy link
Contributor

leonerd commented Apr 8, 2022

Looks like the test failure was due to the trailing NUL byte in $0, which we fixed a while ago.

Can you rebase this on current blead and try again?

It's too close to a 5.36 release to put this in now, but it would be nice to get it lined up ready to start in 5.37.1.

@demerphq
Copy link
Collaborator

demerphq commented Apr 8, 2022

See #19504 (comment) for details on the null byte issue with $0.

@xenu
Copy link
Member Author

xenu commented Apr 19, 2022

Looks like the test failure was due to the trailing NUL byte in $0, which we fixed a while ago.

The new test has actually exposed a real problem in my branch. The cause of the failure was that $0 was getting truncated. Perl doesn't overwrite environ when PERL_USE_SAFE_PUTENV is defined, and because of that, the maximum length of $0 was limited to the length of argv.

I have updated the branch with a more comprehensive fix and also some refactoring, see the commit messages for more details. The non-PERL_USE_SAFE_PUTENV code path was completely removed.

Copy link
Contributor

@Tux Tux left a comment

Choose a reason for hiding this comment

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

For the Configure part, I see that this probe already exists upstream, but has not been included because it isnt/wasnt used. If I would add it it metaconfig.h and regenerate Configure and friends, it'll include the original documentation too.

@Tux
Copy link
Contributor

Tux commented Apr 19, 2022

FWIW, here is the part of the original code that is already available:

?S:d_setenv:
?S:     This variable conditionally defines the HAS_SETENV
?S:     symbol, which indicates to the C program that setenv()
?S:     is available to change or add an environment variable.
?S:.
?C:HAS_SETENV:
?C:     This symbol is defined when setenv() is available to change or
?C:     add an environment variable.
?C:.
?H:#$d_setenv HAS_SETENV
?H:.
?LINT: set d_setenv
: do we have setenv?
$cat >try.c <<EOC
#$i_stdlib I_STDLIB
#ifdef I_STDLIB
#include <stdlib.h>
#endif
int main (void) {
    return setenv ("foo", "bar", 1);
    }
EOC
cyn=setenv
set d_setenv
eval $trylink

(somewhat modified to fit)
Note that this actually treis to compile its use instead of just checking for its existence.

@xenu
Copy link
Member Author

xenu commented Apr 19, 2022

I don't quite understand. As far as I can tell, there are no setenv probes in the Perl's metaconfig fork. Do you mean that the probe should be copied from the upstream dist repository?

@Tux
Copy link
Contributor

Tux commented Apr 20, 2022

There are no setenv probes in the perl-part of metaconfig, but there is one in the upstream part of what is used for meta/dist
If the above code (also) works for you, it is easier for me to add that probe than to add a new probe that differs.

@xenu
Copy link
Member Author

xenu commented Apr 20, 2022

Sure, that works too.

xenu added 4 commits May 29, 2022 00:32
- Early return is much better than wrapping the whole function with if().

- Remove pointless (void) casts.

- Move variable declarations closer to their first use.

- Normalize indentation.
This allows us to overwrite the original environ when $0 is being set,
which means that enabling PERL_USE_SAFE_PUTENV will no longer decrease
the maximum length of $0 on some platforms.
xenu added 4 commits May 29, 2022 00:52
Now environ isn't owned by Perl and calling setenv/putenv in XS code
will no longer result in memory corruption.

Fixes Perl#19399
environ is managed by the C runtime, while we are using the system APIs
directly.
The purpose of PL_origenviron is to preserve the earliest known value
of environ, which is a global. All interpreters should share it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants