Prevent no unique connection bridge name #446

Closed
Tifaifai opened this Issue May 12, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@Tifaifai
Contributor

Tifaifai commented May 12, 2017

Hello,

In case of no unique connection bridge name in mosquitto.conf, mosquitto crash with a segmentation fault.

example:

connection test
address localhost:1884
topic # both testbrod1/ testbrod1/

connection test
address localhost:1884
topic # both testbrod2/ testbrod2/

We know that it's not a good practice (same name for connection) but the reaction of mosquitto can be better.
A first bad solution is prevent this bug with rename the bridge by default
in conf.c, line 928

cur_bridge = &(config->bridges[config->bridge_count-1]);
memset(cur_bridge, 0, sizeof(struct _mqtt3_bridge));
cur_bridge->name = _mosquitto_strdup(token);
if(!cur_bridge->name){
	_mosquitto_log_printf(NULL, MOSQ_LOG_ERR, "Error: Out of memory.");
	return MOSQ_ERR_NOMEM;
}

replace by:

cur_bridge = &(config->bridges[config->bridge_count-1]);
memset(cur_bridge, 0, sizeof(struct _mqtt3_bridge));

sprintf(token, "%s%d", token, config->bridge_count);

cur_bridge->name = _mosquitto_strdup(token);
if(!cur_bridge->name){
	_mosquitto_log_printf(NULL, MOSQ_LOG_ERR, "Error: Out of memory.");
	return MOSQ_ERR_NOMEM;
}

Have you a better solution for prevent segmentation fault and crash ? because we don't understand this problem (no use bridges[i]->name after).

THX

@Tifaifai

This comment has been minimized.

Show comment
Hide comment
@Tifaifai

Tifaifai May 12, 2017

Contributor

Hello,
We can propose you a new better solution :)
In src/bridge.cin function int mqtt3_bridge_new
Replace:

if(!bridge->remote_clientid){
	if(!gethostname(hostname, 256)){
		len = strlen(hostname) + strlen(bridge->name) + 2;
		id = _mosquitto_malloc(len);
		if(!id){
			return MOSQ_ERR_NOMEM;
		}
		snprintf(id, len, "%s.%s", hostname, bridge->name);
	}else{
		return 1;
	}
bridge->remote_clientid = id;

by:

if(!bridge->remote_clientid){
	if(!gethostname(hostname, 256)){
		len = strlen(hostname) + strlen(bridge->name) + 2 + 1 + snprintf(NULL, 0, "%d", db->bridge_count+1);
		id = _mosquitto_malloc(len);
		if(!id){
			return MOSQ_ERR_NOMEM;
		}
		snprintf(id, len, "%s.%s.%d", hostname, bridge->name, db->bridge_count+1);
	}else{
		return 1;
	}
bridge->remote_clientid = id;

What do you think about it ? Correction ;-) ?

C-U

Contributor

Tifaifai commented May 12, 2017

Hello,
We can propose you a new better solution :)
In src/bridge.cin function int mqtt3_bridge_new
Replace:

if(!bridge->remote_clientid){
	if(!gethostname(hostname, 256)){
		len = strlen(hostname) + strlen(bridge->name) + 2;
		id = _mosquitto_malloc(len);
		if(!id){
			return MOSQ_ERR_NOMEM;
		}
		snprintf(id, len, "%s.%s", hostname, bridge->name);
	}else{
		return 1;
	}
bridge->remote_clientid = id;

by:

if(!bridge->remote_clientid){
	if(!gethostname(hostname, 256)){
		len = strlen(hostname) + strlen(bridge->name) + 2 + 1 + snprintf(NULL, 0, "%d", db->bridge_count+1);
		id = _mosquitto_malloc(len);
		if(!id){
			return MOSQ_ERR_NOMEM;
		}
		snprintf(id, len, "%s.%s.%d", hostname, bridge->name, db->bridge_count+1);
	}else{
		return 1;
	}
bridge->remote_clientid = id;

What do you think about it ? Correction ;-) ?

C-U

ralight added a commit that referenced this issue May 12, 2017

@ralight

This comment has been minimized.

Show comment
Hide comment
@ralight

ralight May 12, 2017

Contributor

Thanks for reporting this, I see the crash as well. I've fixed it in a different way by not allowing duplicates.

Contributor

ralight commented May 12, 2017

Thanks for reporting this, I see the crash as well. I've fixed it in a different way by not allowing duplicates.

@ralight ralight closed this May 12, 2017

Tifaifai added a commit to Tifaifai/mosquitto that referenced this issue May 15, 2017

Tifaifai added a commit to Tifaifai/mosquitto that referenced this issue May 15, 2017

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