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

Add suffix to tempnam() #575

Closed
wants to merge 4 commits into from
Closed

Conversation

neufeind
Copy link

Implementation of this RFC:
https://wiki.php.net/rfc/tempnam-suffix

@smalyshev
Copy link
Contributor

This one fails to build with compile error in gd.c.

@neufeind
Copy link
Author

Can't say why gd.c should be a problem here. Worked fine for me. I only had problems with the unittest but didn't find out which mismatch it has in actual/expected output. The output of the testscript itself looks fine to me.

Would be great if somebody could pick this up.

@Tyrael
Copy link
Contributor

Tyrael commented Feb 17, 2014

travis still tells me that this PR breaks the build:
https://travis-ci.org/php/php-src/builds/17938665
I will try to look verify this later yesterday.

@Tyrael
Copy link
Contributor

Tyrael commented Feb 20, 2014

oh, right, you simply missed the php_open_temporary_file() usage in ext/gd.c .
could you please also update that?

@php-pulls
Copy link

*ext/gd/gd.c
2014.02.20. 11:34, "Ferenc Kovacs" notifications@github.com ezt írta:

oh, right, you simply missed the php_open_temporary_file() usage in
ext/gd.c .
could you please also update that?


Reply to this email directly or view it on GitHubhttps://github.com//pull/575#issuecomment-35607450
.

@neufeind
Copy link
Author

had no gd-devel installed, thus gd got disabled during build and I didn't notice it. Thanks for the feedback.

@neufeind
Copy link
Author

Still something broken? This is a neverending story :-(

@Tyrael
Copy link
Contributor

Tyrael commented Feb 20, 2014

Starting program: /Users/tyrael/checkouts/php-src.git/./sapi/cli/php ext/gd/tests/bug41442.php

Program received signal SIGSEGV, Segmentation fault.
0x00007fff929bc812 in ?? ()
(gdb) bt
#0 0x00007fff929bc812 in ?? ()
#1 0x00007fff5fbfd120 in ?? ()
#2 0x000000010024c16f in php_do_open_temporary_file (path=0x0, pfx=0x100495e9b "%s%s%sXXXXXX%s", suffix=0x2101245250 <Address 0x2101245250 out of bounds>, opened_path_p=0x7fff5fbfcba0, tsrm_ls=0x7fff5fbfcca8) at main/php_open_temporary_file.c:161
Backtrace stopped: frame did not save the PC

caused by this line:
if (spprintf(&opened_path, 0, "%s%s%sXXXXXX%s", new_state.cwd, trailing_slash, pfx, suffix) >= MAXPATHLEN) {

I think it is caused by the fact that we allow the NULL pointer to reach all the way here, and spprintf %s expects a null terminated string argument.

@Tyrael
Copy link
Contributor

Tyrael commented Feb 21, 2014

this simple patch solves the segfault, but it would be nice if somebody else could approve this:

diff --git a/main/php_open_temporary_file.c b/main/php_open_temporary_file.c
index 67d158e..6bdd5cb 100644
--- a/main/php_open_temporary_file.c
+++ b/main/php_open_temporary_file.c
@@ -275,6 +275,9 @@ PHPAPI int php_open_temporary_fd_ex(const char *dir, const char *pfx, char **ope
        if (opened_path_p) {
                *opened_path_p = NULL;
        }
+       if (!suffix) {
+               suffix = "";
+       }

        if (!dir || *dir == '\0') {
 def_tmp:

@nikic
Copy link
Member

nikic commented Feb 21, 2014

@Tyrael You need to indent the patch with four spaces or wrap it in ``` to properly format it.

@Tyrael
Copy link
Contributor

Tyrael commented Feb 21, 2014

ah, thanks.

@neufeind
Copy link
Author

Could somebody maybe update the patch and review this? At least I hope we're close ...

@Tyrael
Copy link
Contributor

Tyrael commented Apr 12, 2014

neufeind: only you can update the patch in the pull request.
and unfortunately, we are in feature freeze mode now, so I don't think this can make into 5.6.0 :(

@smalyshev
Copy link
Contributor

Also this has a binary API change in php_open_temporary_file, so unless it's in 5.6.0, it can not be in 5.6 branch at all.

@ghost
Copy link

ghost commented Mar 7, 2015

Can one of the admins verify this patch?

@neufeind
Copy link
Author

neufeind commented Mar 7, 2015

It seems to need some adjustment, but I'm not sure how to continue with it / track that down. A test fails - but how do I debug that?

@Tyrael Tyrael closed this Sep 3, 2015
@Tyrael
Copy link
Contributor

Tyrael commented Sep 3, 2015

if you still want to pursue this, please create a new PR

@neufeind
Copy link
Author

neufeind commented Sep 3, 2015

I still didn't find out how to track down the failing test. The patch itself worked afaik but my tries (as a php-src-newbie) to find help unfortunately lead nowhere :-(

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.

6 participants