From 72618921adee29022038dd5fbd130f922d5078bc Mon Sep 17 00:00:00 2001 From: Martijn Otto Date: Fri, 20 May 2016 11:16:12 +0200 Subject: Fixed segfault during Value::~Value --- include/array.h | 2 +- include/class.h | 34 +++++++++++++++++++++++++--------- include/classbase.h | 2 +- include/object.h | 2 +- include/value.h | 2 +- zend/callable.h | 22 ++++++++++++++++------ zend/classimpl.cpp | 27 ++++++++------------------- zend/value.cpp | 41 +++++++++++++++-------------------------- 8 files changed, 68 insertions(+), 64 deletions(-) diff --git a/include/array.h b/include/array.h index 126cd58..925e902 100644 --- a/include/array.h +++ b/include/array.h @@ -79,7 +79,7 @@ public: * Change the internal type of the variable * @param Type */ - virtual Value &setType(Type type) override + virtual Value &setType(Type type) & override { // throw exception if things are going wrong if (type != Type::Array) throw FatalError("Changing type of a fixed array variable"); diff --git a/include/class.h b/include/class.h index e92dfc3..371595e 100644 --- a/include/class.h +++ b/include/class.h @@ -53,7 +53,7 @@ public: /** * Destructor */ - virtual ~Class() {} + virtual ~Class() = default; /** * Add a regular method to the class @@ -461,8 +461,12 @@ private: // cast to actual object T *obj = (T *)base; - // pass on - return Value(obj->__toString()).setType(Type::String); + // retrieve the casted value and convert it if necessary + auto result = obj->__toString(); + result.setType(Type::String); + + // return the converted result + return result; } /** @@ -475,8 +479,12 @@ private: // cast to actual object T *obj = (T *)base; - // pass on - return Value(obj->__toInteger()).setType(Type::Numeric); + // retrieve the casted value and convert it if necessary + auto result = obj->__toInteger(); + result.setType(Type::Numeric); + + // return the converted result + return result; } /** @@ -489,8 +497,12 @@ private: // cast to actual object T *obj = (T *)base; - // pass on - return Value(obj->__toFloat()).setType(Type::Float); + // retrieve the casted value and convert it if necessary + auto result = obj->__toFloat(); + result.setType(Type::Float); + + // return the converted result + return result; } /** @@ -503,8 +515,12 @@ private: // cast to actual object T *obj = (T *)base; - // pass on - return Value(obj->__toBool()).setType(Type::Bool); + // retrieve the casted value and convert it if necessary + auto result = obj->__toBool(); + result.setType(Type::Bool); + + // return the converted result + return result; } /** diff --git a/include/classbase.h b/include/classbase.h index 0db0c35..a9542d7 100644 --- a/include/classbase.h +++ b/include/classbase.h @@ -90,7 +90,7 @@ public: /** * Destructor */ - virtual ~ClassBase() {} + virtual ~ClassBase() = default; /** * Construct a new instance of the object, or to clone the object diff --git a/include/object.h b/include/object.h index 745b7c6..56d7e83 100644 --- a/include/object.h +++ b/include/object.h @@ -106,7 +106,7 @@ public: * Change the internal type of the variable * @param Type */ - virtual Value &setType(Type type) override + virtual Value &setType(Type type) & override { // throw exception if things are going wrong if (type != Type::Object) throw FatalError("Changing type of a fixed object variable"); diff --git a/include/value.h b/include/value.h index afd532d..cd32568 100644 --- a/include/value.h +++ b/include/value.h @@ -361,7 +361,7 @@ public: * Change the internal type of the variable * @param Type */ - virtual Value &setType(Type type); + virtual Value &setType(Type type) &; /** * Make a clone of the value with the same type diff --git a/zend/callable.h b/zend/callable.h index f6a172f..e875d0a 100644 --- a/zend/callable.h +++ b/zend/callable.h @@ -148,12 +148,22 @@ protected: if (arg.type() == Type::Object) info->class_name = arg.classname(); else info->class_name = nullptr; - // since php 5.4 there is a type-hint, but we only support arrays, objects and callables - switch (arg.type()) { - case Type::Array: info->type_hint = IS_ARRAY; break; - case Type::Callable: info->type_hint = IS_CALLABLE; break; - case Type::Object: info->type_hint = IS_OBJECT; break; - default: info->type_hint = IS_NULL; break; + // set the correct type-hint + switch (arg.type()) + { + case Type::Undefined: info->type_hint = IS_UNDEF; break; // undefined means we'll accept any type + case Type::Null: info->type_hint = IS_UNDEF; break; // this is likely an error, what good would accepting NULL be? accept anything + case Type::False: info->type_hint = _IS_BOOL; break; // accept true as well ;) + case Type::True: info->type_hint = _IS_BOOL; break; // accept false as well + case Type::Bool: info->type_hint = _IS_BOOL; break; // any bool will do, true, false, the options are limitless + case Type::Numeric: info->type_hint = IS_LONG; break; // accept integers here + case Type::Float: info->type_hint = IS_DOUBLE; break; // floating-point values welcome too + case Type::String: info->type_hint = IS_STRING; break; // accept strings, should auto-cast objects with __toString as well + case Type::Array: info->type_hint = IS_ARRAY; break; // array of anything (individual members cannot be restricted) + case Type::Object: info->type_hint = IS_OBJECT; break; // must be an object of the given classname + case Type::Callable: info->type_hint = IS_CALLABLE; break; // anything that can be invoked + default: info->type_hint = IS_UNDEF; break; // if not specified we allow anything + } // from PHP 5.6 and onwards, an is_variadic property can be set, this diff --git a/zend/classimpl.cpp b/zend/classimpl.cpp index 7ced789..94dc236 100644 --- a/zend/classimpl.cpp +++ b/zend/classimpl.cpp @@ -455,38 +455,27 @@ int ClassImpl::cast(zval *val, zval *retval, int type TSRMLS_DC) // we need the C++ class meta-information object ClassBase *meta = self(entry)->_base; - // retval is not yet initialized --- and again feelings of disbelief, - // frustration, wonder and anger come up when you see that there are not two - // functions in the Zend engine that have a comparable API - // - // this function was removed, because it was supposedly no longer necessary - // can we get away with removing it here too? - // INIT_PZVAL(retval); - // when the magic function it not implemented, an exception will be thrown, // and the extension may throw a Php::Exception try { - // the result zval - zval *result = nullptr; + // the result value + Value result; // check type switch ((Type)type) { - case Type::Numeric: result = meta->callToInteger(object).detach(true); break; - case Type::Float: result = meta->callToFloat(object).detach(true); break; - case Type::Bool: result = meta->callToBool(object).detach(true); break; - case Type::String: result = meta->callToString(object).detach(true); break; - default: throw NotImplemented(); break; + case Type::Numeric: result = meta->callToInteger(object); break; + case Type::Float: result = meta->callToFloat(object); break; + case Type::Bool: result = meta->callToBool(object); break; + case Type::String: result = meta->callToString(object); break; + default: throw NotImplemented(); break; } // @todo do we turn into endless conversion if the __toString object returns 'this' ?? // (and if it does: who cares? If the extension programmer is stupid, why do we have to suffer?) - // is the original parameter overwritten? - if (val == retval) zval_dtor(val); - // overwrite the result - ZVAL_ZVAL(retval, result, 1, 1); + ZVAL_DUP(retval, result._val); // done return SUCCESS; diff --git a/zend/value.cpp b/zend/value.cpp index 9be4b24..4aaec4c 100644 --- a/zend/value.cpp +++ b/zend/value.cpp @@ -143,7 +143,7 @@ Value::Value(double value) : _val(new zval) Value::Value(struct _zval_struct *val, bool ref) : _val(new zval) { // copy the value over (and add reference if relevant) - ZVAL_COPY(_val, val); + ZVAL_DUP(_val, val); // not refcounted? then there is nothing we can do here // @todo: we should be able to force a reference somehow @@ -240,28 +240,22 @@ Value::~Value() // ignore if moved if (!_val) return; - // copy to local variable - auto val = _val; - - // reset local - _val = nullptr; - // are we not a refcounted variable? - if (!Z_REFCOUNTED_P(val)) + if (!Z_REFCOUNTED_P(_val)) { // we can simply delete it - delete val; + delete _val; } else { - // if there were two references or less, we're going to remove a reference - // and only one reference will remain, the object will then impossible be - // a reference - if (Z_REFCOUNT_P(val) <= 2) ZVAL_UNREF(val); - - // destruct the zval (this function will decrement the reference counter, - // and only destruct if there are no other references left) - zval_ptr_dtor(val); + // remove the reference that we have + Z_DELREF_P(_val); + + // one would assume we would need to explicitly delete + // _val here too, since we allocated it ourselves, this + // turns out to lead to double-free problems, we should + // check whether this is true in all cases and whether + // this leads to memory leaks here... } } @@ -353,9 +347,6 @@ Value &Value::operator=(Value &&value) _NOEXCEPT } else { - // we need the tsrm_ls variable - TSRMLS_FETCH(); - // the last and only reference to the other object was // removed, we no longer need it delete value._val; @@ -366,9 +357,8 @@ Value &Value::operator=(Value &&value) _NOEXCEPT } else { - // destruct the zval (this function will decrement the reference counter, - // and only destruct if there are no other references left) - if (_val) zval_ptr_dtor(_val); + // first destruct ourselves properly + this->~Value(); // just copy the zval completely _val = value._val; @@ -1007,7 +997,7 @@ Type Value::type() const * @param type * @return Value */ -Value &Value::setType(Type type) +Value &Value::setType(Type type) & { // skip if nothing changes if (this->type() == type) return *this; @@ -1035,7 +1025,6 @@ Value &Value::setType(Type type) case Type::ConstantAST: throw FatalError{ "Constant types can not be assigned to a PHP-CPP library variable" }; break; case Type::Callable: throw FatalError{ "Callable types can not be assigned to a PHP-CPP library variable" }; break; case Type::Reference: throw FatalError{ "Reference types cannot be assigned to a PHP-CPP library variable" }; break; - } // done @@ -1154,7 +1143,7 @@ Value Value::clone() const Value output; // copy the value over to the output - ZVAL_COPY_VALUE(output._val, _val); + ZVAL_DUP(output._val, _val); // done return output; -- cgit v1.2.3