Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:18794
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Message-ID: <432534E4.9020602@lerdorf.com>
Date: Mon, 12 Sep 2005 15:57:24 +0800
User-Agent: Mozilla Thunderbird 1.0.6 (Macintosh/20050716)
MIME-Version: 1.0
To: internals <internals@lists.php.net>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: ref fix revisited
From: rasmus@lerdorf.com (Rasmus Lerdorf)

Guys, could we take a look at making the ref to temp var fix a bit
narrower?  Currently we try to catch it at call-time.  This means that
something like:

  current(explode(' ','a b'))

as per bug #34468 doesn't work.  Now, I think there is a secondary bug
here.  I see no reason for current() to take a by-ref, so this
particular one could be easily fixed.  But there are many other cases
where a function legitimately takes a by-ref and doesn't necessarily
write to it or the write is a secondary action not required for the code
to work.  Could we not catch this on the write instead of on the call?
The memory problem happens on the write.  Or perhaps better, an E_NOTICE
or E_STRICT on the call and an E_FATAL on the write.  The current
E_FATAL on the call seems out of whack.

Gallery, for example, broke in a rather subtle way in their
gallery_remote2.php script which meant the various client-side tools
like iphototogallery and others got a cryptic "no album at URL" error
message.  I had to break out ethereal to track it down to a couple of
functions where read-only function args were marked as by-ref.  So they
didn't actually have a memory corruption problem yet the E_FATAL killed
them.

SquirrelMail has code like this all over the place:

   $value = strtolower(array_shift(split('/\w/',trim($value))));

Here array_shift() does of course change the arg, so that is a potential
problem.  And yes, that's a dumb way to do this, but people write code
like this.  In some of these array manipulation calls, which seems to
account for a number of the BC problems we are having, we could check
for a non-ref and behave slightly differently.  In the case of
array_shift() we could return the first arg and throw a notice.  Same
would go for reset(), end(), next(), prev() and probably a few others.

-Rasmus