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.

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

x509.c: fix memory leak on self-test #2106

Merged
merged 1 commit into from Apr 8, 2019

Conversation

Projects
None yet
6 participants
@JunhwanPark
Copy link
Contributor

commented Oct 17, 2018

When the time on the target board is not set, the X.509 verification step self-test may fail. When I checked the memory before and after this test, I could see that there was a leak in some memory.

Similar to other self tests, this PR changes using the goto command so that buffer free can be performed before returning.

@RonEld

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

@JunhwanPark Thank you for your contribution and for your interest in Mbed TLS!
In addition, unfortunately our policy is to not accept contributions, without a Contributor’s Licence Agreement (CLA) signed or authorised by yourself or your employer.

If this is a personal contribution, the easiest way to do this is if you create an mbed account and accept this click through agreement. Alternatively, you can find a slightly different agreement to sign here, which can be signed and returned to us, and is applicable if you don't want to create an mbed account or alternatively if this is a corporate contribution.

Thanks for your understanding and again, thanks for the contribution!

Show resolved Hide resolved library/x509.c Outdated
@hanno-arm
Copy link
Contributor

left a comment

Thanks @JunhwanPark for spotting and fixing this bug!

Overall, I agree with the proposed change, but the following needs to be addressed before merge:

  1. There is a potential free-without-initialization in the code.
  2. The PR needs a ChangeLog entry in the Bugfix section, including credits for yourself. Suggestion: Fix potential memory leak in X.509 self test. Found and fixed by YOUR_NAME in #2106.

@hanno-arm hanno-arm added needs: work and removed needs: review labels Oct 17, 2018

@hanno-arm hanno-arm self-assigned this Oct 17, 2018

@JunhwanPark JunhwanPark force-pushed the JunhwanPark:development branch from b0271bf to 6a33ebe Oct 17, 2018

JunhwanPark added a commit to JunhwanPark/mbedtls that referenced this pull request Oct 17, 2018

x509.c: Fix potential memory leak in X.509 self test
Found and fixed by Junhwan Park in ARMmbed#2106.

Signed-off-by: Junhwan Park <junhwan.park@samsung.com>
@JunhwanPark

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

Hi @hanno-arm ,
Thanks for review. I re-updated.

@JunhwanPark JunhwanPark force-pushed the JunhwanPark:development branch from 6a33ebe to ea0ed48 Oct 17, 2018

JunhwanPark added a commit to JunhwanPark/mbedtls that referenced this pull request Oct 17, 2018

@JunhwanPark

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

Hi @RonEld ,
I joined a Mbed Portal. And, I sent e-mail.
Thank you for information.

@RonEld

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

Hi @JunhwanPark
Thank you for your cooperation!
If you joined the portal, I think it would be faster for you if you accept the click through agreement
Is this contribution part of your work, and submitted on behalf of your company? If so, we will need to get a corporate CLA accepted from someone authorized to accept it in your organization.
Thanks again for your understanding and your interest in Mbed TLS!

@@ -1019,7 +1019,8 @@ int mbedtls_x509_self_test( int verbose )
if( verbose != 0 )
mbedtls_printf( "failed\n" );

return( ret );
mbedtls_x509_crt_free( &clicert );

This comment has been minimized.

Copy link
@hanno-arm

hanno-arm Oct 17, 2018

Contributor

It is more in line with the remainder of the library to do a bulk initialization at the beginning. I'd therefore suggest to move the initialization call mbedtls_x509_crt_init( &cacert ); to L1013.

@JunhwanPark JunhwanPark force-pushed the JunhwanPark:development branch from ea0ed48 to 7455c1b Oct 17, 2018

JunhwanPark added a commit to JunhwanPark/mbedtls that referenced this pull request Oct 17, 2018

x509.c: Fix potential memory leak in X.509 self test
Found and fixed by Junhwan Park in ARMmbed#2106.

Signed-off-by: Junhwan Park <semoking@naver.com>
@JunhwanPark

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

@RonEld
I do not want to submit on behalf of the company. I want to do it for my personal interest. :)
I also agreed to the site you gave me.

@hanno-arm
You are right. I changed cacert and clicert to initialize at the same time.
Thank you.

@hanno-arm

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

@JunhwanPark Could you please add a ChangeLog entry? The code looks fine, now.

@JunhwanPark JunhwanPark force-pushed the JunhwanPark:development branch from 7455c1b to a89313a Oct 17, 2018

JunhwanPark added a commit to JunhwanPark/mbedtls that referenced this pull request Oct 17, 2018

x509.c: Fix potential memory leak in X.509 self test
Found and fixed by Junhwan Park in ARMmbed#2106.

Signed-off-by: Junhwan Park <semoking@naver.com>
@JunhwanPark

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

@hanno-arm
I added a changelog. Thanks for the details.

@hanno-arm
Copy link
Contributor

left a comment

Looks good to me - thanks again @JunhwanPark!

@hanno-arm hanno-arm requested a review from k-stachowiak Oct 17, 2018

@RonEld RonEld added CLA valid and removed CLA requested labels Oct 17, 2018

@RonEld RonEld removed the needs: work label Oct 18, 2018

@RonEld RonEld removed the request for review from k-stachowiak Oct 18, 2018

@sbutcher-arm

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

retest

@JunhwanPark

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

Hi @hanno-arm , Should I resolve this? Or do you fix the conflict while rebass?

x509.c: Fix potential memory leak in X.509 self test
Found and fixed by Junhwan Park in #2106.

Signed-off-by: Junhwan Park <semoking@naver.com>

@JunhwanPark JunhwanPark dismissed stale reviews from RonEld and hanno-arm via 39bdab7 Nov 10, 2018

@JunhwanPark JunhwanPark force-pushed the JunhwanPark:development branch from a89313a to 39bdab7 Nov 10, 2018

@JunhwanPark

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2018

Oh. I resolved complict.. but I have dissmissed your approve.. sorry.

@RonEld

RonEld approved these changes Dec 6, 2018

@RonEld RonEld added needs: review and removed ready for merge labels Dec 6, 2018

@sbutcher-arm

This comment has been minimized.

Copy link
Collaborator

commented Jan 24, 2019

Passed reviews and CI, so ready for merge. Also looks like it needs back porting (which can be done by the team), so marking as needs back ports.

This PR can be used to merge with the main branch and mbedtls-2.16, but we should still have another PR for mbedtls-2.7.

@JunhwanPark

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

@sbutcher-arm Thank you!

RonEld added a commit to RonEld/mbedtls that referenced this pull request Mar 11, 2019

x509.c: Fix potential memory leak in X.509 self test
Found and fixed by Junhwan Park in ARMmbed#2106.

Signed-off-by: Junhwan Park <semoking@naver.com>

RonEld added a commit to RonEld/mbedtls that referenced this pull request Mar 11, 2019

x509.c: Fix potential memory leak in X.509 self test
Found and fixed by Junhwan Park in ARMmbed#2106.

Signed-off-by: Junhwan Park <semoking@naver.com>
@RonEld

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

@JunhwanPark I have backported this PR on your behalf to mbedtls-2.7 andmbedtls-2.16

@RonEld

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

The backports have been fully reviewed and approved, so removing the "needs backports" label

cc @Patater

@JunhwanPark

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

@RonEld I have forgotten this PR for a while. :) Thanks for creating Backport.

@Patater Patater merged commit 39bdab7 into ARMmbed:development Apr 8, 2019

2 of 3 checks passed

continuous-integration/jenkins/pr-head The build of this commit was aborted
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.