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.

Sign up for GitHub

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

Jump to bottom

Fixed critical segfault, memory leak and default keepalive #54

Merged
merged 4 commits into from
Oct 22, 2016
Merged

Fixed critical segfault, memory leak and default keepalive #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
511f8ba
fixed segfault in PHP7 in message destruct
argakon Oct 18, 2016
cb35d7b
change default keepalive 0 -> 60 in connect function, as written in docs
argakon Oct 18, 2016
4141a12
fixed memory leaks
argakon Oct 18, 2016
773e6c4
fixed another memory leak, produced by mosquitto_message_copy in mess…
argakon Oct 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions mosquitto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ PHP_METHOD(Mosquitto_Client, connect)
mosquitto_strlen_type host_len, interface_len;
int retval;
zend_long port = 1883;
zend_long keepalive = 0;
zend_long keepalive = 60;

PHP_MOSQUITTO_ERROR_HANDLING();
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s|lls!",
Expand Down Expand Up @@ -1147,7 +1147,12 @@ PHP_MOSQUITTO_API void php_mosquitto_message_callback(struct mosquitto *mosq, vo

object_init_ex(message_zval, mosquitto_ce_message);
message_object = mosquitto_message_object_from_zend_object(Z_OBJ_P(message_zval));
mosquitto_message_copy(&message_object->message, message);
message_object->message.mid = message->mid;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caused a leak? Was it logged by PHP's memory leak detection or was it something you found in Valgrind or similar?

Copy link
Contributor Author

@argakon argakon Oct 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.
At first i used 2 simple scripts. One for subscribing and second for publishing 100000 messages. I run this scripts and after that watch in htop linux utility at memory column. In htop memory always grow.
After that i run subscribe script in valgrind and saw the problem. Leak was in PHP5 and PHP7.

setCredentials("esp", "esp"); $mqtt->onConnect(function($rc, $message) use ($topic, $mqtt) { echo 'Connected' . PHP_EOL; $mqtt->subscribe($topic, 0); }); $mqtt->onMessage(function($message) { var_dump(memory_get_usage()); var_dump($message); }); $mqtt->connect('192.168.5.1', 1883, 60); $mqtt->loopForever(100); ?> setCredentials("esp", "esp"); $iterations = 100000; $i = 0; echo 'Start testing' . PHP_EOL; $mqtt->connect("192.168.5.1", 1883, 60); while(true) { $arr = [ "test_i" => 123, "test_f" => 123.23, "test_s" => "test" . rand(1, 200000) ]; $s = $mqtt->publish("meteo/test", json_encode($arr)); usleep(1000); $i++; echo "Iteration: {$i}" . PHP_EOL; if ($i >= $iterations) break; } $mqtt->disconnect(); ?>

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, yes, I see now that I've looked at how that function is implemented:
https://github.com/eclipse/mosquitto/blob/master/lib/messages_mosq.c#L59

Good catch, thanks!

message_object->message.qos = message->qos;
message_object->message.retain = message->retain;
message_object->message.topic = message->topic;
message_object->message.payload = message->payload;
Copy link
Owner

@mgdm mgdm Oct 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're going to have to estrdup() the topic and something similar for the payload, as Mosquitto will free the memory after the callback completes. I can do that though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you wish. I'am not a C developer, my commit is a just quick fix.

message_object->message.payloadlen = message->payloadlen;

object->message_callback.params = params;
object->message_callback.param_count = 1;
Expand Down
5 changes: 4 additions & 1 deletion mosquitto_message.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,9 +487,12 @@ void php_mosquitto_message_add_property(HashTable *h, const char *name, size_t n
static void mosquitto_message_object_destroy(zend_object *object TSRMLS_DC)
{
mosquitto_message_object *message = mosquitto_message_object_from_zend_object(object);
#ifdef ZEND_ENGINE_3
zend_object_std_dtor(object);
#else
zend_hash_destroy(message->std.properties);
FREE_HASHTABLE(message->std.properties);

#endif
if (message->owned_topic == 1) {
efree(message->message.topic);
}
Expand Down