Fixing other potential memory corruption #155

huguesBouvier · 2018-04-07T01:25:00Z

Fixing other potential memory corruption bugs.
Also added a function to free up mutexes created by mqtt init and shadow init

Passed unit tests and integration tests

Fix memory corruption

gordonwang0 · 2018-04-07T16:46:51Z

samples/linux/shadow_sample/aws_iot_config.h

@@ -23,7 +23,7 @@

 // Get from console
 // =================================================
-#define AWS_IOT_MQTT_HOST              "" ///< Customer specific MQTT HOST. The same will be used for Thing Shadow
+#define AWS_IOT_MQTT_HOST              "a3qggb6vsdlf5s.iot.us-west-2.amazonaws.com" ///< Customer specific MQTT HOST. The same will be used for Thing Shadow


Endpoint should be removed. Changes to this file shouldn't be committed.

Correct, fixed

gordonwang0 · 2018-04-07T16:51:24Z

src/aws_iot_mqtt_client.c

+        rc = aws_iot_thread_mutex_destroy(&(pClient->clientData.state_change_mutex));
+    }
+
+    if ((&(pClient->clientData.state_change_mutex) != NULL)&&(rc == SUCCESS))


This should check if tls_read_mutex isn't NULL.

How about also setting pClient->clientData.tls_read_mutex to NULL after destroying it?

This would be a bug, it is a structure.

gordonwang0 · 2018-04-07T16:51:52Z

src/aws_iot_mqtt_client.c

+        rc = aws_iot_thread_mutex_destroy(&(pClient->clientData.tls_read_mutex));
+    }
+
+    if ((&(pClient->clientData.state_change_mutex) != NULL)&&(rc == SUCCESS))


This should check if tls_write_mutex isn't NULL.

How about also setting pClient->clientData.tls_write_mutex to NULL after destroying it?

This would be a bug, it is a structure.

gordonwang0 · 2018-04-07T16:55:36Z

src/aws_iot_mqtt_client.c

+#ifdef _ENABLE_THREAD_SUPPORT_
+    if ((&(pClient->clientData.state_change_mutex) != NULL)&&(rc == SUCCESS))
+    {
+        rc = aws_iot_thread_mutex_destroy(&(pClient->clientData.state_change_mutex));


How about also setting pClient->clientData.state_change_mutex to NULL after destroying it?

I'm not 100% sure about handling in this way.
(&(pClient->clientData.state_change_mutex) != NULL) checks if the address of variable pClient->clientData.state_change_mutex is not NULL.
Then this address is passed to aws_iot_thread_mutex_destroy() by value; the function can free the content, but cannot modify the parameter passed by value (that is the address).
Even setting pClient->clientData.state_change_mutex to NULL (which I think it's a good idea), its address is not NULL.
And so calling aws_iot_mqtt_free() one more time the condition (&(pClient->clientData.state_change_mutex) != NULL) is true and so aws_iot_thread_mutex_destroy() would be called ahain.

Is this the expected beahvior?

Actually it is a bug, checking NULL is pointless, it is a structure

It seems there is not need to check, destroy will set it to an invalid value so subsequent call will return EINVALID

gordonwang0 · 2018-04-07T17:04:47Z

src/aws_iot_shadow_json.c

@@ -424,13 +462,13 @@ bool isReceivedJsonValid(const char *pJsonDocument) {
 	return true;
 }

-bool extractClientToken(const char *pJsonDocument, char *pExtractedClientToken) {
+bool extractClientToken(const char *pJsonDocument, size_t jsonSize, char *pExtractedClientToken, size_t clientTokenSize) {
 	int32_t tokenCount, i;
 	uint8_t length;


Are you sure uint8_t (up to 255) is always sufficient? How about size_t instead?

It's still uint8_t in the latest version?

gordonwang0 · 2018-04-07T17:06:10Z

tests/unit/src/aws_iot_tests_unit_shadow_action_helper.c

+	ret_val = isReceivedJsonValid(getRequestJson, TEST_JSON_SIZE);
+	CHECK_EQUAL_C_INT(true, ret_val);
+
+	IOT_DEBUG("-->Success - sReceivedJsonValid");


Wrong test name.

gordonwang0 · 2018-04-07T17:06:19Z

tests/unit/src/aws_iot_tests_unit_shadow_action_helper.c

+	bool ret_val;
+	char getRequestJson[TEST_JSON_SIZE];
+
+	IOT_DEBUG("-->Running Shadow Action Tests - sReceivedJsonValid \n");


Wrong test name.

This is still sReceivedJsonValid in the latest version?

gordonwang0 · 2018-04-07T17:06:35Z

tests/unit/src/aws_iot_tests_unit_shadow_action_helper.c

+	ret_val = extractClientToken(getRequestJson, TEST_JSON_SIZE, extractedClientToken, MAX_SIZE_CLIENT_ID_WITH_SEQUENCE );
+	CHECK_EQUAL_C_INT(true, ret_val);
+
+	IOT_DEBUG("-->Success - getAndDeleteRequest");


Wrong test name.

gordonwang0 · 2018-04-07T17:12:50Z

Can you also change aws_iot_mqtt_client.c?

aws-iot-device-sdk-embedded-C/src/aws_iot_mqtt_client.c

Lines 207 to 221 in ba30f68

    
           #ifdef _ENABLE_THREAD_SUPPORT_ 
        
           	pClient->clientData.isBlockOnThreadLockEnabled = pInitParams->isBlockOnThreadLockEnabled; 
        
           	rc = aws_iot_thread_mutex_init(&(pClient->clientData.state_change_mutex)); 
        
           	if(SUCCESS != rc) { 
        
           		FUNC_EXIT_RC(rc); 
        
           	} 
        
           	rc = aws_iot_thread_mutex_init(&(pClient->clientData.tls_read_mutex)); 
        
           	if(SUCCESS != rc) { 
        
           		FUNC_EXIT_RC(rc); 
        
           	} 
        
           	rc = aws_iot_thread_mutex_init(&(pClient->clientData.tls_write_mutex)); 
        
           	if(SUCCESS != rc) { 
        
           		FUNC_EXIT_RC(rc); 
        
           	} 
        
           #endif

In aws_iot_mqtt_init, the mutexes state_change_mutex and tls_read_mutex and tls_write_mutex need to be destroyed before returning if there's an error after their creation to avoid a memory leak.

huguesBouvier · 2018-04-09T17:38:19Z

Ok, I will change that.

Fix from review comments

…ice-sdk-embedded-C

gordonwang0 · 2018-04-09T17:50:52Z

src/aws_iot_mqtt_client.c

+        rc = aws_iot_thread_mutex_destroy(&(pClient->clientData.state_change_mutex));
+    }
+
+    if (rc == SUCCESS)


I think we should still attempt to destroy mutexes, even if the previous call to aws_iot_thread_mutex_destroy failed. We could also print a debug message if a call to aws_iot_thread_mutex_destroy fails.

gordonwang0 · 2018-04-09T17:54:11Z

src/aws_iot_mqtt_client.c

 		FUNC_EXIT_RC(rc);
 	}
 	rc = aws_iot_thread_mutex_init(&(pClient->clientData.tls_write_mutex));
 	if(SUCCESS != rc) {
+		(void)aws_iot_thread_mutex_destroy(&(pClient->clientData.tls_read_mutex));
+		(void)aws_iot_thread_mutex_destroy(&(pClient->clientData.state_change_mutex));
 		FUNC_EXIT_RC(rc);
 	}
 #endif


Line 263 also contains an exit on failure, should also destroy mutexes there.

I am not sure I agree totally with that process. The mqtt free should be called after init anyway. I think we are bringing complexity when not needed.

But if mqtt_init returns something other than SUCCESS, users may not know they need to call mqtt_free. I think it makes sense for init to clean everything up if it fails.

Deleting as much mutexes as possible

adding destroy at the end of function

gordonwang0 · 2018-04-09T18:10:38Z

src/aws_iot_mqtt_client.c

+		{
+			rc = aws_iot_thread_mutex_destroy(&(pClient->clientData.tls_write_mutex));
+		}else{
+			(void)aws_iot_thread_mutex_destroy(&(pClient->clientData.tls_read_mutex));


This line should destroy tls_write_mutex.

Also, is it necessary to re-assign rc? Seems we could just do the following to clean everything up.

if(rc == SUCCESS) /* Check not NULL_VALUE_ERROR. */ { (void)aws_iot_thread_mutex_destroy(&(pClient->clientData.state_change_mutex)); (void)aws_iot_thread_mutex_destroy(&(pClient->clientData.tls_read_mutex)); (void)aws_iot_thread_mutex_destroy(&(pClient->clientData.tls_write_mutex)); }

I though of that but it will not work unless you refactor the function because it has exit points everywhere

gordonwang0 · 2018-04-09T18:13:36Z

src/aws_iot_mqtt_client.c

 		FUNC_EXIT_RC(rc);
 	}
 	rc = aws_iot_thread_mutex_init(&(pClient->clientData.tls_write_mutex));
 	if(SUCCESS != rc) {
+		(void)aws_iot_thread_mutex_destroy(&(pClient->clientData.tls_read_mutex));
+		(void)aws_iot_thread_mutex_destroy(&(pClient->clientData.state_change_mutex));
 		FUNC_EXIT_RC(rc);
 	}
 #endif


But if mqtt_init returns something other than SUCCESS, users may not know they need to call mqtt_free. I think it makes sense for init to clean everything up if it fails.

Fix a forgotten statement

Fixing unit tests

Fixing auto reconnect

Display wrong message when received

Changed string

Fixed dereferencing void pointer

Fix compilation error

Added thing identification to test

Extended string

use strtok_s for thread safety

…ice-sdk-embedded-C

Fix compilation error

Fix bugs from coverity

…ice-sdk-embedded-C

Cosmetic changes from review comment

…ice-sdk-embedded-C

Hugues Bouvier and others added 10 commits April 6, 2018 10:31

Add Jenkinsfile.

1a34c87

Fix memory corruption

a495105

Fix memory corruption

Adding unit test

18acc3c

Create JenkinsFileLoop

fb02c95

Update Jenkinsfile

7c04de6

Fix in unit tests

5119277

Fix

26a70ad

Merge branch 'PullRequest1'

a4244b7

Delete JenkinsFileLoop

fd624c7

Delete Jenkinsfile

1b3d4a3

gordonwang0 mentioned this pull request Apr 7, 2018

Memory leak due to aws_iot_shadow_init() #145

Closed

gordonwang0 reviewed Apr 7, 2018

View reviewed changes

huguesBouvier added 2 commits April 9, 2018 10:43

Fix from review comments

96fad39

Fix from review comments

Merge branch 'master' of https://github.com/huguesBouvier/aws-iot-dev…

c9feb0b

…ice-sdk-embedded-C

gordonwang0 reviewed Apr 9, 2018

View reviewed changes

huguesBouvier added 2 commits April 9, 2018 11:03

Deleting as much mutexes as possible

18178ee

Deleting as much mutexes as possible

adding destroy at the end of function

c8bc22e

adding destroy at the end of function

gordonwang0 reviewed Apr 9, 2018

View reviewed changes

gordonwang0 approved these changes Apr 9, 2018

View reviewed changes

huguesBouvier added 9 commits April 9, 2018 13:48

Fix a forgotten statement

2aed242

Fix a forgotten statement

Create Jenkinsfile

f78a70e

Fixing unit tests

518f608

Fixing unit tests

Fixing auto reconnect

cef33ef

Fixing auto reconnect

Display wrong message when received

8249abe

Display wrong message when received

Changed string

a9804d2

Changed string

Fixed dereferencing void pointer

897e937

Fixed dereferencing void pointer

Improved messaging

e7d3b1c

Ensure pthread_join is being used everywhere

ef91240

huguesBouvier added 11 commits April 11, 2018 12:40

Fix compilation error

dbb18dc

Fix compilation error

Added thing identification to test

3c17afa

Added thing identification to test

Extended string

c30f632

Extended string

Update Jenkinsfile

e367b28

use strtok_s for thread safety

e716e28

use strtok_s for thread safety

Merge branch 'master' of https://github.com/huguesBouvier/aws-iot-dev…

1784c14

…ice-sdk-embedded-C

Fix compilation error

25a7ab4

Fix compilation error

Use strtok_r

3836148

Update Jenkinsfile

e3076ca

Fix bugs from coverity

ff478b2

Fix bugs from coverity

Merge branch 'master' of https://github.com/huguesBouvier/aws-iot-dev…

902a185

…ice-sdk-embedded-C

gordonwang0 approved these changes Apr 13, 2018

View reviewed changes

huguesBouvier added 3 commits April 13, 2018 11:11

Delete Jenkinsfile

8f823b8

Cosmetic changes from review comment

276f4c0

Cosmetic changes from review comment

Merge branch 'master' of https://github.com/huguesBouvier/aws-iot-dev…

5c6685a

…ice-sdk-embedded-C

huguesBouvier merged commit ecc311b into aws:master Apr 13, 2018

gordonwang0 pushed a commit that referenced this pull request Apr 13, 2018

Fixing other potential memory corruption (#155)

760380e

gordonwang0 pushed a commit that referenced this pull request Apr 20, 2018

Fixing other potential memory corruption (#155)

6543589

lt72 mentioned this pull request Apr 23, 2018

SECURITY ISSUE: Buffer overflow in parseStringValue #151

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing other potential memory corruption #155

Fixing other potential memory corruption #155

huguesBouvier commented Apr 7, 2018

gordonwang0 Apr 7, 2018

huguesBouvier Apr 9, 2018

gordonwang0 Apr 7, 2018 •

edited

Loading

huguesBouvier Apr 9, 2018

gordonwang0 Apr 7, 2018 •

edited

Loading

huguesBouvier Apr 9, 2018

gordonwang0 Apr 7, 2018

escherstair Apr 8, 2018

huguesBouvier Apr 9, 2018

huguesBouvier Apr 9, 2018

gordonwang0 Apr 7, 2018

huguesBouvier Apr 9, 2018

gordonwang0 Apr 9, 2018

gordonwang0 Apr 7, 2018

huguesBouvier Apr 9, 2018

gordonwang0 Apr 7, 2018

huguesBouvier Apr 9, 2018

gordonwang0 Apr 9, 2018

gordonwang0 Apr 7, 2018

huguesBouvier Apr 9, 2018

gordonwang0 commented Apr 7, 2018

huguesBouvier commented Apr 9, 2018

gordonwang0 Apr 9, 2018

gordonwang0 Apr 9, 2018

huguesBouvier Apr 9, 2018

gordonwang0 Apr 9, 2018

gordonwang0 Apr 9, 2018

huguesBouvier Apr 9, 2018

gordonwang0 Apr 9, 2018

Fixing other potential memory corruption #155

Fixing other potential memory corruption #155

Conversation

huguesBouvier commented Apr 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gordonwang0 Apr 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gordonwang0 Apr 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gordonwang0 commented Apr 7, 2018

huguesBouvier commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gordonwang0 Apr 7, 2018 •

edited

Loading

gordonwang0 Apr 7, 2018 •

edited

Loading