diff --git a/ChangeLog.txt b/ChangeLog.txt index f5e4a1f7..bbbef2cb 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -1,4 +1,5 @@ Broker: +- Fix possible crash under heavy network load. Closes #463241. - Fix possible crash when using pattern ACLs. - Fix problems parsing config strings with multiple leading spaces. Closes #462154. diff --git a/lib/mosquitto_internal.h b/lib/mosquitto_internal.h index 76b7cfa8..b288969c 100644 --- a/lib/mosquitto_internal.h +++ b/lib/mosquitto_internal.h @@ -123,7 +123,7 @@ struct _mosquitto_packet{ uint32_t pos; uint16_t mid; uint8_t command; - uint8_t remaining_count; + int8_t remaining_count; }; struct mosquitto_message_all{ diff --git a/lib/net_mosq.c b/lib/net_mosq.c index 34c1edbe..f43f3199 100644 --- a/lib/net_mosq.c +++ b/lib/net_mosq.c @@ -935,15 +935,24 @@ int _mosquitto_packet_read(struct mosquitto *mosq) } } } - if(mosq->in_packet.remaining_count == 0){ + /* remaining_count is the number of bytes that the remaining_length + * parameter occupied in this incoming packet. We don't use it here as such + * (it is used when allocating an outgoing packet), but we must be able to + * determine whether all of the remaining_length parameter has been read. + * remaining_count has three states here: + * 0 means that we haven't read any remaining_length bytes + * <0 means we have read some remaining_length bytes but haven't finished + * >0 means we have finished reading the remaining_length bytes. + */ + if(mosq->in_packet.remaining_count <= 0){ do{ read_length = _mosquitto_net_read(mosq, &byte, 1); if(read_length == 1){ - mosq->in_packet.remaining_count++; + mosq->in_packet.remaining_count--; /* Max 4 bytes length for remaining length as defined by protocol. * Anything more likely means a broken/malicious client. */ - if(mosq->in_packet.remaining_count > 4) return MOSQ_ERR_PROTOCOL; + if(mosq->in_packet.remaining_count < -4) return MOSQ_ERR_PROTOCOL; #if defined(WITH_BROKER) && defined(WITH_SYS_TREE) g_bytes_received++; @@ -967,6 +976,9 @@ int _mosquitto_packet_read(struct mosquitto *mosq) } } }while((byte & 128) != 0); + /* We have finished reading remaining_length, so make remaining_count + * positive. */ + mosq->in_packet.remaining_count *= -1; if(mosq->in_packet.remaining_length > 0){ mosq->in_packet.payload = _mosquitto_malloc(mosq->in_packet.remaining_length*sizeof(uint8_t)); diff --git a/src/websockets.c b/src/websockets.c index cf2b5e70..13d2b87e 100644 --- a/src/websockets.c +++ b/src/websockets.c @@ -270,7 +270,7 @@ static int callback_mqtt(struct libwebsocket_context *context, return -1; } } - if(mosq->in_packet.remaining_count == 0){ + if(mosq->in_packet.remaining_count <= 0){ do{ if(pos == len){ return 0; @@ -278,17 +278,18 @@ static int callback_mqtt(struct libwebsocket_context *context, byte = buf[pos]; pos++; - mosq->in_packet.remaining_count++; + mosq->in_packet.remaining_count--; /* Max 4 bytes length for remaining length as defined by protocol. * Anything more likely means a broken/malicious client. */ - if(mosq->in_packet.remaining_count > 4){ + if(mosq->in_packet.remaining_count < -4){ return -1; } mosq->in_packet.remaining_length += (byte & 127) * mosq->in_packet.remaining_mult; mosq->in_packet.remaining_mult *= 128; }while((byte & 128) != 0); + mosq->in_packet.remaining_count *= -1; if(mosq->in_packet.remaining_length > 0){ mosq->in_packet.payload = _mosquitto_malloc(mosq->in_packet.remaining_length*sizeof(uint8_t));