You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benno Evers <be...@mesosphere.com> on 2019/11/22 16:20:21 UTC
Review Request 71805: Fixed memory leak in openssl verification
function.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71805/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Greg Mann.
Repository: mesos
Description
-------
When the hostname validation scheme was set to 'openssl',
the `openssl::verify()` function would return without
freeing a previously allocated `X509*` object.
To fix the leak, a long-standing TODO to switch to
RAII-based memory management for the certificate was
resolved.
Diffs
-----
3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3
Diff: https://reviews.apache.org/r/71805/diff/1/
Testing
-------
`make check`
Thanks,
Benno Evers
Re: Review Request 71805: Fixed memory leak in openssl verification
function.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Nov. 22, 2019, 5:33 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Line 843 (original), 843 (patched)
> > <https://reviews.apache.org/r/71805/diff/1/?file=2176773#file2176773line843>
> >
> > This says "if this call succeeds", but I guess it should say in all cases?
> >
> > I guess it's not possible to just have it on the stack.. :(
>
> Benno Evers wrote:
> I think "if this call succeeds" is correct: If it doesn't then `SSL_get_peer_certificate()` returns a null pointer, and `std::unique_ptr` guarantees that it will only call the deleter when the contained value is non-null.
>
> You're right that it's not possible to have it on the stack, at least there's no way that I know of.
ohhh, I had read "this call" as the function we're in, rather than the function we're calling
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71805/#review218765
-----------------------------------------------------------
On Nov. 22, 2019, 4:20 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71805/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2019, 4:20 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When the hostname validation scheme was set to 'openssl',
> the `openssl::verify()` function would return without
> freeing a previously allocated `X509*` object.
>
> To fix the leak, a long-standing TODO to switch to
> RAII-based memory management for the certificate was
> resolved.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3
>
>
> Diff: https://reviews.apache.org/r/71805/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71805: Fixed memory leak in openssl verification
function.
Posted by Benno Evers <be...@mesosphere.com>.
> On Nov. 22, 2019, 5:33 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Line 843 (original), 843 (patched)
> > <https://reviews.apache.org/r/71805/diff/1/?file=2176773#file2176773line843>
> >
> > This says "if this call succeeds", but I guess it should say in all cases?
> >
> > I guess it's not possible to just have it on the stack.. :(
I think "if this call succeeds" is correct: If it doesn't then `SSL_get_peer_certificate()` returns a null pointer, and `std::unique_ptr` guarantees that it will only call the deleter when the contained value is non-null.
You're right that it's not possible to have it on the stack, at least there's no way that I know of.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71805/#review218765
-----------------------------------------------------------
On Nov. 22, 2019, 4:20 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71805/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2019, 4:20 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When the hostname validation scheme was set to 'openssl',
> the `openssl::verify()` function would return without
> freeing a previously allocated `X509*` object.
>
> To fix the leak, a long-standing TODO to switch to
> RAII-based memory management for the certificate was
> resolved.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3
>
>
> Diff: https://reviews.apache.org/r/71805/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71805: Fixed memory leak in openssl verification
function.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71805/#review218765
-----------------------------------------------------------
Ship it!
Good find! Can you file a MESOS ticket for this with affected versions tagged?
3rdparty/libprocess/src/openssl.cpp
Line 843 (original), 843 (patched)
<https://reviews.apache.org/r/71805/#comment306631>
This says "if this call succeeds", but I guess it should say in all cases?
I guess it's not possible to just have it on the stack.. :(
- Benjamin Mahler
On Nov. 22, 2019, 4:20 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71805/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2019, 4:20 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When the hostname validation scheme was set to 'openssl',
> the `openssl::verify()` function would return without
> freeing a previously allocated `X509*` object.
>
> To fix the leak, a long-standing TODO to switch to
> RAII-based memory management for the certificate was
> resolved.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3
>
>
> Diff: https://reviews.apache.org/r/71805/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71805: Fixed memory leak in openssl verification
function.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71805/
-----------------------------------------------------------
(Updated Nov. 22, 2019, 10:53 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann.
Bugs: MESOS-10041
https://issues.apache.org/jira/browse/MESOS-10041
Repository: mesos
Description
-------
When the hostname validation scheme was set to 'openssl',
the `openssl::verify()` function would return without
freeing a previously allocated `X509*` object.
To fix the leak, a long-standing TODO to switch to
RAII-based memory management for the certificate was
resolved.
Diffs
-----
3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3
Diff: https://reviews.apache.org/r/71805/diff/1/
Testing
-------
`make check`
Thanks,
Benno Evers
Re: Review Request 71805: Fixed memory leak in openssl verification
function.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71805/#review218763
-----------------------------------------------------------
3rdparty/libprocess/src/openssl.cpp
Line 866 (original), 866 (patched)
<https://reviews.apache.org/r/71805/#comment306629>
FYI, since it isn't touched in the review: Here was the location of the original leak.
- Benno Evers
On Nov. 22, 2019, 4:20 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71805/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2019, 4:20 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When the hostname validation scheme was set to 'openssl',
> the `openssl::verify()` function would return without
> freeing a previously allocated `X509*` object.
>
> To fix the leak, a long-standing TODO to switch to
> RAII-based memory management for the certificate was
> resolved.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3
>
>
> Diff: https://reviews.apache.org/r/71805/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71805: Fixed memory leak in openssl verification
function.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71805/#review218777
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On Nov. 22, 2019, 4:20 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71805/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2019, 4:20 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When the hostname validation scheme was set to 'openssl',
> the `openssl::verify()` function would return without
> freeing a previously allocated `X509*` object.
>
> To fix the leak, a long-standing TODO to switch to
> RAII-based memory management for the certificate was
> resolved.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/openssl.cpp bd05866950e1043d9585a7c5fdc7b2147a233fd3
>
>
> Diff: https://reviews.apache.org/r/71805/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benno Evers
>
>