Re: [PHP-CVS] cvs: php-src(PHP_5_3) /main php_open_temporary_file.c

2009-06-24 Thread Johannes Schlüter 
Hi Ilia,

On Wed, 2009-06-24 at 12:21 +, Ilia Alshanetsky wrote:  
   MFB: Fixed bug #48465 (sys_get_temp_dir() possibly inconsistent when using
   TMPDIR).

First: I'd like to remind about what Lukas wrote as we planned to
release 5.3.0 with as little source for trouble as possible:

   If issues are found/fixed please send the patches to
internals for review.

Others didn't do that either but at least asked for review on IRC or
private mail ...

Would be nice if all could follow a stricter review process till 5.3 is
tagged (see the mail Lukas will send in a few minutes to internals, too)

Seondly:

 @@ -200,7 +200,14 @@
   {
   char* s = getenv(TMPDIR);
   if (s) {
 - temporary_directory = strdup(s);
 + int len = strlen(s);
 +
 + if (s[len - 1] == DEFAULT_SLASH) {
 + temporary_directory = zend_strndup(s, len - 1);

This seems to be broken in case TMPDIR=.

johannes

 + } else {
 + temporary_directory = zend_strndup(s, len);
 + }
 +
   return temporary_directory;
   }
   }
 
 
 


-- 
PHP CVS Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-CVS] cvs: php-src(PHP_5_3) /main php_open_temporary_file.c

2009-06-24 Thread Ilia Alshanetsky 

Will keep in mind for the future, do you want me to revert the change?


Ilia Alshanetsky




On 24-Jun-09, at 9:14 AM, Johannes Schlüter wrote:


Hi Ilia,

On Wed, 2009-06-24 at 12:21 +, Ilia Alshanetsky wrote:
 MFB: Fixed bug #48465 (sys_get_temp_dir() possibly inconsistent  
when using

 TMPDIR).


First: I'd like to remind about what Lukas wrote as we planned to
release 5.3.0 with as little source for trouble as possible:

  If issues are found/fixed please send the patches to
   internals for review.

Others didn't do that either but at least asked for review on IRC or
private mail ...

Would be nice if all could follow a stricter review process till 5.3  
is
tagged (see the mail Lukas will send in a few minutes to internals,  
too)


Seondly:


@@ -200,7 +200,14 @@
{
char* s = getenv(TMPDIR);
if (s) {
-   temporary_directory = strdup(s);
+   int len = strlen(s);
+
+   if (s[len - 1] == DEFAULT_SLASH) {
+   temporary_directory = zend_strndup(s, len - 1);


This seems to be broken in case TMPDIR=.

johannes


+   } else {
+   temporary_directory = zend_strndup(s, len);
+   }
+
return temporary_directory;
}
}






--
PHP CVS Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php




--
PHP CVS Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-CVS] cvs: php-src(PHP_5_3) /main php_open_temporary_file.c

2009-06-24 Thread Lukas Kahwe Smith 


On 24.06.2009, at 18:34, Ilia Alshanetsky wrote:


Will keep in mind for the future, do you want me to revert the change?


The patch looks safe. However if you would have asked before  
commiting, Johannes would have said no, since its simply not critical  
enough to bring any risk to the release process at this point. Then  
again, this fixes a bug, its in CVS, so it seems like children finger  
slapping to ask for a revert.


We have to trust that people do not intentionally go against what we  
ask and we have to also hope that people realize that this is a  
sensitive for the 5_3 branch and then check posts from the RM's before  
committing. I guess we should maybe define some subject prefix to give  
such posts additional visible weight and maybe I should more clearly  
state the current commit policy for 5_3 on some static webpage/wiki.  
But I also think this is not the right moment to come up with such  
policies. After we get 5.3.0 stable out expect an RFC that will  
hopefully outline how to improve things so that the release process is  
able to produce higher quality code with less hassle for contributors.


Back to the patch at hand. Like I said, the patch looks safe, however  
the devil is in the detail as with many seemlingly simply things. As  
Johannes has pointed out already, there is the question of what  
happens if TMPDIR=.


So please do revert this patch.

regards,
Lukas



On 24-Jun-09, at 9:14 AM, Johannes Schlüter wrote:


Hi Ilia,

On Wed, 2009-06-24 at 12:21 +, Ilia Alshanetsky wrote:
MFB: Fixed bug #48465 (sys_get_temp_dir() possibly inconsistent  
when using

TMPDIR).


First: I'd like to remind about what Lukas wrote as we planned to
release 5.3.0 with as little source for trouble as possible:

 If issues are found/fixed please send the patches to
  internals for review.

Others didn't do that either but at least asked for review on IRC or
private mail ...

Would be nice if all could follow a stricter review process till  
5.3 is
tagged (see the mail Lukas will send in a few minutes to internals,  
too)


Seondly:


@@ -200,7 +200,14 @@
{
char* s = getenv(TMPDIR);
if (s) {
-   temporary_directory = strdup(s);
+   int len = strlen(s);
+
+   if (s[len - 1] == DEFAULT_SLASH) {
+   temporary_directory = zend_strndup(s, len - 1);


This seems to be broken in case TMPDIR=.

johannes


+   } else {
+   temporary_directory = zend_strndup(s, len);
+   }
+
return temporary_directory;
}
}






--
PHP CVS Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php




--
PHP CVS Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php



Lukas Kahwe Smith
m...@pooteeweet.org




--
PHP CVS Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php