Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Bugfixes #365

Closed
wants to merge 18 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

evoloshchuk commented Jul 1, 2011

Some additional fixes, together with fixes from facebook#239

Contributor

evoloshchuk commented Jul 5, 2011

Thanks for the explanation!

lcastelli and others added some commits Oct 12, 2010

Fix array_keys($array, null) to make it equivalent to PHP.
Currently in Hiphop "array_keys($data)" and "array_keys($data, null)"
are equivalent, as the optional second parameter has a default value of
null. In PHP instead they are different, with the second one in
particular returning only the keys from $data where the corresponding
value is null.

The following patch adds a special "not_given_variant" global variable
which can be used to distinguish the case where a parameter is provided
with the value null from the case where no parameter is provided at
all.
Use better escaping for identifiers.
There are currently a few different implementations of string escaping
with the purpose of generating valid C++ identifiers, which are used in
different places. Some of them don't correctly escape all the
characters which are invalid in identifiers, and can also generate the
same identifier out of different original strings, which could result
in collisions. For example these problems affect the compiled templates
generated by Smarty, which use characters like % or ^ in the filenames.

The best implementation for identifier escaping seems to be
CodeGenerator::formatLabel. This patch extracts that function to Util
and removes the other implementations. To improve readability, some
shorter escape codes have also been introduced for common characters
(for example "." -> "$_"). The patch also removes the LabelEscape
option for simplicity and correctness, since guaranteeing that no
collisions are possible becomes more complicated if the escaping string
is not fixed in advance.
Fix PID file race condition
There is a slight race condition regarding the creation/removal of the
PID file when a new hiphop instance is started using the takeover
functionality. In particular in certain cases it's possible that the
old instance removes the PID file after the new instance has already
created the new PID file. In this case the old instance will actually
remove the PID file of the new instance, resulting in a situation where
Hiphop is running but there's no PID file on the file system.

This fixes the problem by removing the PID file synchronously before
the new instance creates the new PID file.
Work-around GCC compiler bug
The same problem described in a previous patch
3610aa4 has been observed in the new
ObjectAllocatorInitSetup<T> function and how it calls
set<...>.insert(). Like in the previous case, there are situations
where the caller thinks the insert() function has been applied the
return value optimization, and so it passes as the first parameter the
address of where it wants the return value to be stored. The function
itself is instead not expecting any additional return value parameter,
and thus uses all the parameters incorrectly causing seg faults or
incorrect behaviors. See the description of the other patch for some
more information.

This patch moves the insert function call outside of the function
template, which seems effective at working around the problem.
Add missing check for NULL in VariableTable::getVariablePrefix
The last 'if' in the method didn't check for 'sym' being set.
Bug in object unserialization.
In case when object has a private property, wich results in primitive
typed property in generated c++, unserialization works incorrectly.

It happens because the current way of unserialization fails to get
a Variant* for such property (see HASH_REALPROP_TYPED_NAMSTR),
thus new public dynamic one with the same name is created.

The fix initializes the private variables through ClassInfo::SetArray/o_setArray

Example:

<?php
class A {
  private $x = true;
  public function isX() {
    return $this->x;
  }
  public function f() {
    $this->x = false;
  }
}
$a = new A();
$a->f();
$s = serialize($a);
$b = unserialize($s);
var_dump($b->isX());
?>

will give "(bool)true", instead of expeced "(bool)false".
Updating string idl
Catching up with latest changes by running 'EXT=string make -C idl update'
Provide lcfirst.
Add implementation of php 5.3 function lcfirst,
which makes a string's first character lowercase.
`
Bug in json_decode
In following cases the behavior is different from php:

json_decode("test") gives string(4) "test" instead of NULL
json_decode("'test'") gives string(6) "'test'" instead of NULL

The fix makes it work the same way php does.
Bug in htmlspecialchars/htmlentities
For strings that contain \0 the behavior is different from php:

htmlspecialchars("test\0test") gives string(4) "test" instead of string(9) "test\0test"
htmlentities("test\0test") gives string(4) "test" instead of string(9) "test\0test"

The fix makes it work the same way php does.
Fix for Option::IdPrefix
After recent changes using ___ instead of $$ when HPHP_OSS is enabled does not seem to have much sense,
because $$ is used for all internal classes constants (see 06e5270).
Moreover, it results in compilation error when trying to compile for example
<?php var_dump(XMLReader::NONE); ?>
since it tries to access constant as q_XMLReader___NONE, while it's defined as q_XMLReader$$NONE.
Fix for type mismatch
The following php code

<?php
function f($x) {
	$c = 0;
	$c = sqrt($x);
	$a[] = $b = ($x < 0 ? $c : -$c);
}
?>

after compilation with --opts "deadcode" will result in an error

operands to ?: have different types ‘double’ and ‘HPHP::Variant’

This fix tries to fix this by en extra type infer for this case,
which was not needed before due work of fixExpectedType.
Introduced with d7b9fa4
Fix for code generation when PregenerateCPP=true.
After introduction of property tables the current way of c++ files generation
when clustering is requested together with PregenerateCPP=true does not work.

It happens because after code is pregenerated the classes scope information
gets lost, i.e. not available for code generator when it builds property tables.

The fix saves the classes scope when pregeneration is done and restore it back
to code generator when that pregenerated code used, making it possible to build
property tables after.
Fix stackLimit calculation for an edge case
When you run compiled binary from fork or exec on linux RLIMIT_STACK might get
lost in some cases. This makes the current calculation wrong, which results in
false negative "infinite recursion detected" for any program.

The fix uses old way of estimation of stackLimit when RLIMIT_STACK is not
available.
Bug in accessing constant from eval statements.
The following code
<?php
   define('A', 'test');
   eval("var_dump(A);");
?>
will result in
HipHop Notice:  Use of undefined constant A - assumed 'A' in string on line 1
string(1) "A"
when compiled with eval enabled.

The bug was introduced with 1a09996.

The fix provide gives it another try to find constant in dynamic table
before issuing an error.
Bug in using error-control operator together with constants.
The following code
<?php
   var_dump(@A);
   var_dump(B);
?>
when compiled will result in
HipHop Notice:  Use of undefined constant A - assumed 'A' in _bug2.php on line 3
string(1) "A"
HipHop Notice:  Use of undefined constant B - assumed 'B' in _bug2.php on line 4
string(1) "B"
while only second notice is expected.

The bug was introduced with 4a92854.

The fix makes it always use Silencer for constants with error-control operator.
Bug in handling non-static call as a static in some edge cases.
The following example will result in a segmenation fault.

<?php
   class A {
      public function f($x) {
         return $x;
      }
   }
   class B {
      function __construct() {
         array_map(array('A', 'f'), array(0,1,3));
      }
   }
   $b = new B();
?>

The problem seems to be in the way array_map works. It creates and initialize
cmp for callback once for all calls. Then during the first call the dummy
object is created and the reference to corresponding ObjectData is saved within
mcp. This dummy object is destroyed right after the call is done, but the mcp
still has that reference, which is used for next calls, which causes problem.
RuntimeOption::ThreadLoopDocumentsSleepSeconds
When implementing daemons or cronjobs as a ThreadLoopDocument
the approach of making them responsible for sleeping between calls is a bit fragile.
It easily becomes error prone in case of many documents
and might result in one thread consuming all the resources.

A RuntimeOption::ThreadLoopDocumentsSleepSeconds was added
to specify default sleep time for all loop documents.

ctc commented on 2d605db Apr 21, 2012

rl.rlim_cur is an unsigned int - and is compared with -1 (signed) ?

Contributor

ptarjan commented May 13, 2013

We're closing out all bugs older than 2 months. http://www.hiphop-php.com/wp/?p=575

If this is still an issue, please re-open it, and in order of goodness:

  1. Give detailed repro steps
  2. Write a test case in hphp/tests/quick (run it with hphp/tests/run) and send the pull request
  3. Fix it in a pull request

@ptarjan ptarjan closed this May 13, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment