| Message ID | 20201209160645.15561-1-cotequeiroz@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Petr Štetiar |
| Headers | show |
| Series | [ustream] ustream-openssl: fix bio memory leak | expand |
Eneas U de Queiroz <cotequeiroz@gmail.com> [2020-12-09 13:06:45]: Hi, > Using the patch by Pan Chen as inspiration, this avoids a memory leak by > using a global BIO_METHOD pointer that doesn't ordinarily need to be > freed. this sounds weird, how is global pointer avoiding memory leaks? :-) > CC: Pan Chen <serial115200@outlook.com> > > Signed-off-by: Eneas U de Queiroz <cotequeiroz@gmail.com> > > --- > Run-tested with a WRT-3200ACM, running uclient_fetch and uhttpd. > I have not run it with valgrind or any other debugger. how do you otherwise verify the correctness? :-) FYI this is my work in progress[1]. 1. https://gitlab.com/ynezz/openwrt-ustream-ssl/-/commit/807ce1de752e021802a563783dfa580950746a0c Cheers, Petr
On Wed, Dec 09, 2020 at 05:44:48PM +0100, Petr Štetiar wrote: > Eneas U de Queiroz <cotequeiroz@gmail.com> [2020-12-09 13:06:45]: > > Hi, > > > Using the patch by Pan Chen as inspiration, this avoids a memory leak by > > using a global BIO_METHOD pointer that doesn't ordinarily need to be > > freed. > > this sounds weird, how is global pointer avoiding memory leaks? :-) Well, it moves it from "definitely lost" to "still reachable" when looking at it with valgrind. We will still have to free it as well, and that could be done just like in the original patch. > > > CC: Pan Chen <serial115200@outlook.com> > > > > Signed-off-by: Eneas U de Queiroz <cotequeiroz@gmail.com> > > > > --- > > Run-tested with a WRT-3200ACM, running uclient_fetch and uhttpd. > > I have not run it with valgrind or any other debugger. > > how do you otherwise verify the correctness? :-) FYI this is my work in progress[1]. > > 1. https://gitlab.com/ynezz/openwrt-ustream-ssl/-/commit/807ce1de752e021802a563783dfa580950746a0c > > Cheers, > > Petr > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
On Wed, Dec 9, 2020 at 1:45 PM Petr Štetiar <ynezz@true.cz> wrote: > > Eneas U de Queiroz <cotequeiroz@gmail.com> [2020-12-09 13:06:45]: > > Hi, > > > Using the patch by Pan Chen as inspiration, this avoids a memory leak by > > using a global BIO_METHOD pointer that doesn't ordinarily need to be > > freed. > > this sounds weird, how is global pointer avoiding memory leaks? :-) BIO_METHOD was made opaque by openssl 1.1.0. It's just a table of methods, and it does change. Before that, one would just fill the struct without having to make any calls. I am the one responsible for introducing the bug in 34b0b80 [ustream-ssl: add openssl-1.1.0 compatibility]. The old, openssl 1.0 code was just: static BIO_METHOD methods_ustream = { 100 | BIO_TYPE_SOURCE_SINK, "ustream", s_ustream_write, s_ustream_read, s_ustream_puts, s_ustream_gets, s_ustream_ctrl, s_ustream_new, s_ustream_free, NULL, }; So the answer to your question is because you only allocate the table if methods_ustream is NULL, and it will point to the created table then. The table won't change during the lifetime of the process, and will get freed only when the process ends. We could free it in s_ustream_free, but only to have to create it again with the same data the next time ustream_bio_new is called. I wouldn't do it, but if you'd rather, I can add it in a v2. > > > CC: Pan Chen <serial115200@outlook.com> > > > > Signed-off-by: Eneas U de Queiroz <cotequeiroz@gmail.com> > > > > --- > > Run-tested with a WRT-3200ACM, running uclient_fetch and uhttpd. > > I have not run it with valgrind or any other debugger. > > how do you otherwise verify the correctness? :-) FYI this is my work in progress[1]. > > 1. https://gitlab.com/ynezz/openwrt-ustream-ssl/-/commit/807ce1de752e021802a563783dfa580950746a0c As for testing I don't have valgrind running, so I wasn't able to do it; but someone else can. That's why I made sure to point it out. As for the WIP, you're perhaps doing too much work. Cheers, Eneas
On Wed, Dec 9, 2020 at 1:58 PM Daniel Golle <daniel@makrotopia.org> wrote: > > On Wed, Dec 09, 2020 at 05:44:48PM +0100, Petr Štetiar wrote: > > Eneas U de Queiroz <cotequeiroz@gmail.com> [2020-12-09 13:06:45]: > > > > Hi, > > > > > Using the patch by Pan Chen as inspiration, this avoids a memory leak by > > > using a global BIO_METHOD pointer that doesn't ordinarily need to be > > > freed. > > > > this sounds weird, how is global pointer avoiding memory leaks? :-) > > Well, it moves it from "definitely lost" to "still reachable" when > looking at it with valgrind. We will still have to free it as well, > and that could be done just like in the original patch. > See my reply to Petr. I'm not sure if valgrind will be completely pleased with my approach. I'm not an expert with valgrind, but it seems to not like that I left it in the heap to be cleaned up by the process end, but that is my intention. As long as I am not allocating memory again--it will only be created when methods_ustream is NULL, which is when it is initialized, and there's nowhere else in code that touches it. Note the const in the definition of: BIO * BIO_new(const BIO_METHOD *type); Cheers, Eneas
Eneas U de Queiroz <cotequeiroz@gmail.com> [2020-12-09 14:39:06]: Hi, > So the answer to your question is because you only allocate the table if > methods_ustream is NULL, and it will point to the created table then. I was referencing the missing freeing of allocated resources. > We could free it in s_ustream_free, but only to have to create it again > with the same data the next time ustream_bio_new is called. I wouldn't do > it, but if you'd rather, I can add it in a v2. Is this micro optimization worth it? You're adding global variable in the library, you're breaking API layer etc. I'm not supposed to study how is it implemented _now_, because it will likely change with the next release (either OpenSSL or wolfSSL) and it might be source of regressions. The API boundary is given so I'm just trying to use it as designed and as seen in the docs/examples/tests etc. And there is always new/free combo. > As for the WIP, you're perhaps doing too much work. I'm spending time on this mainly because of FS#3465, perhaps mbedTLS has similar issues[1]. In the end I would like to have uclient/ustream-ssl CI tested (all 3 SSL libs combinations), with static analyzers, various sanitizers and Valgrind. So I have to fix all the issues those tools expose. Maybe it's too much work, but given the constraints (no globals, follow API), it's currently simplest working solution, but not fully tested yet. BTW I'm not discouraging you from v2, I've rejected the v1 patch, because it doesn't fix the memory leak as advertised in the subject :-) Thanks! 1. https://patchwork.ozlabs.org/project/openwrt/patch/trinity-0c56705d-7e2c-482a-a0b5-a3230d3e75b2-1533383113431@3c-app-gmx-bs62/ Cheers, Petr
Hi Petr On Wed, Dec 9, 2020 at 6:59 PM Petr Štetiar <ynezz@true.cz> wrote: > > Eneas U de Queiroz <cotequeiroz@gmail.com> [2020-12-09 14:39:06]: > > Hi, > > > So the answer to your question is because you only allocate the table if > > methods_ustream is NULL, and it will point to the created table then. > > I was referencing the missing freeing of allocated resources. > > > We could free it in s_ustream_free, but only to have to create it again > > with the same data the next time ustream_bio_new is called. I wouldn't do > > it, but if you'd rather, I can add it in a v2. > > Is this micro optimization worth it? You're adding global variable in the > library, you're breaking API layer etc. I'm not supposed to study how is it > implemented _now_, because it will likely change with the next release (either > OpenSSL or wolfSSL) and it might be source of regressions. The API boundary is > given so I'm just trying to use it as designed and as seen in the > docs/examples/tests etc. And there is always new/free combo. > The purpose of BIO_METHOD struct is to hold a table of methods for a BIO object to use. In our case, it remains constant for the lifetime of the process. So, the maximum usable lifetime of methods_ustream is up to the lifetime of the program--it does not mean that we can't set a shorter lifetime. In an ideal world, we would free the resource when the library is cleaned up/deinitialized, but we don't have a function for that. So a possible lifetime we can use is the lifetime of the BIO object using it. One thing we need to be aware of is use after free. We pass the pointer to the BIO_new, and we must be sure that openssl will not access that memory after we free it. This would be after we call BIO_free. The thing is, we aren't making that call. so we are leaking that resource as well. That one can't have the lifetime of the program, its lifetime is no larger than the underlying SSL connection, apparently. So we need to take care of that first. After tackling BIO_free, my suggestion would be to determine where the method table variable should go, and where to call BIO_meth_new and BIO_meth_free. I would add it to a defined struct ustream_ssl_ctx--which is now just used with a cast to SSL_CTX--and would create and free the object in __ustream_ssl_context_new and __ustream_ssl_context_free, which would give it a possibly larger lifetime than the ssl_session or the BIO object. > > As for the WIP, you're perhaps doing too much work. I was corrected by my own previous point. > I'm spending time on this mainly because of FS#3465, perhaps mbedTLS has > similar issues[1]. In the end I would like to have uclient/ustream-ssl CI > tested (all 3 SSL libs combinations), with static analyzers, various > sanitizers and Valgrind. So I have to fix all the issues those tools expose. > > Maybe it's too much work, but given the constraints (no globals, follow API), > it's currently simplest working solution, but not fully tested yet. > > BTW I'm not discouraging you from v2, I've rejected the v1 patch, because it > doesn't fix the memory leak as advertised in the subject :-) Thanks! We should coordinate efforts. You're the boss, so tell me what you want me to do, if anything. Cheers, Eneas
Eneas U de Queiroz <cotequeiroz@gmail.com> [2020-12-10 11:29:36]: Hi, > access that memory after we free it. This would be after we call > BIO_free. The thing is, we aren't making that call. so we are leaking > that resource as well. IIRC this is solved by OpenSSL internal reference counting and executed in SSL_free() or SSL_shutdown(). > After tackling BIO_free, my suggestion would be to determine where the > method table variable should go, and where to call BIO_meth_new and > BIO_meth_free. I would add it to a defined struct > ustream_ssl_ctx--which is now just used with a cast to SSL_CTX--and IIRC I've tried that approach already(this WIP solution is like 3rd iteration), but that struct is opaque. > would create and free the object in __ustream_ssl_context_new and > __ustream_ssl_context_free, which would give it a possibly larger > lifetime than the ssl_session or the BIO object. AFAIK that's exactly what I'm doing in my current solution. > We should coordinate efforts. You're the boss, so tell me what you want me > to do, if anything. I didn't wanted to sound like the boss and I apologize if that was the case, sorry. I've just send out some patches for uclient/ustream-ssl, so I would be grateful if you could review and test those changes on your device(s), ideally on all three SSL libs and client/server setup. Thanks! Cheers, Petr
Hi Petr On Thu, Dec 10, 2020 at 12:57 PM Petr Štetiar <ynezz@true.cz> wrote: > > After tackling BIO_free, my suggestion would be to determine where the > > method table variable should go, and where to call BIO_meth_new and > > BIO_meth_free. I would add it to a defined struct > > ustream_ssl_ctx--which is now just used with a cast to SSL_CTX--and > > IIRC I've tried that approach already(this WIP solution is like 3rd > iteration), but that struct is opaque. I meant the ustream_ssl_ctx structure, which is an ustream internal structure. For openssl, we're just using a straight cast to the openssl's SSL_CTX struct, so that's why it is opaque, while for mbedtls, it is a defined struct. What I meant was to actually define a ustream_ssl_ctx structure for openssl, just as ustream-mbedtls does, with the BIO_methods and the SSL_CTX as members. > > would create and free the object in __ustream_ssl_context_new and > > __ustream_ssl_context_free, which would give it a possibly larger > > lifetime than the ssl_session or the BIO object. > > AFAIK that's exactly what I'm doing in my current solution. You're doing it at the SSL struct. You can have multiple SSL structs under the same SSL_CTX struct. In a server, for example, you will have one SSL_CTX object, which accepts connections, creating a new SSL structure for each connection. You know I'm just madly fighting for every CPU cycle of performance optimization I can get. ;-) If you look at it from an organization and tidiness POV, you can argue that the BIO methods structure should be placed along with the BIO, which is with the SSL structure. I'll let you pick your side. > > We should coordinate efforts. You're the boss, so tell me what you want me > > to do, if anything. > > I didn't wanted to sound like the boss and I apologize if that was the case, > sorry. I apologize for the bad choice of words. Someone has to take the lead, and that was a rather ill-fated attempt to make it clear that I would follow your lead, and had nothing to do with your tone or anything you had done. > I've just send out some patches for uclient/ustream-ssl, so I would be > grateful if you could review and test those changes on your device(s), ideally > on all three SSL libs and client/server setup. Thanks! I'll do that over the weekend. I'm updating openssl to 1.1.1i, which fixes high severity CVE-2020-1971. I haven't sent it yet because I want to test it first, and I'm low on testing resources right now. I'll probably test openssl tonight, then tackle ustream-ssl. Cheers, Eneas
diff --git a/ustream-io-openssl.c b/ustream-io-openssl.c index 606ed4a..26b3ed5 100644 --- a/ustream-io-openssl.c +++ b/ustream-io-openssl.c @@ -116,20 +116,23 @@ static long s_ustream_ctrl(BIO *b, int cmd, long num, void *ptr) }; } +static BIO_METHOD *methods_ustream = NULL; + static BIO *ustream_bio_new(struct ustream *s) { BIO *bio; - BIO_METHOD *methods_ustream; - - methods_ustream = BIO_meth_new(100 | BIO_TYPE_SOURCE_SINK, "ustream"); - BIO_meth_set_write(methods_ustream, s_ustream_write); - BIO_meth_set_read(methods_ustream, s_ustream_read); - BIO_meth_set_puts(methods_ustream, s_ustream_puts); - BIO_meth_set_gets(methods_ustream, s_ustream_gets); - BIO_meth_set_ctrl(methods_ustream, s_ustream_ctrl); - BIO_meth_set_create(methods_ustream, s_ustream_new); - BIO_meth_set_destroy(methods_ustream, s_ustream_free); + if (methods_ustream == NULL) { + methods_ustream = BIO_meth_new(100 | BIO_TYPE_SOURCE_SINK, + "ustream"); + BIO_meth_set_write(methods_ustream, s_ustream_write); + BIO_meth_set_read(methods_ustream, s_ustream_read); + BIO_meth_set_puts(methods_ustream, s_ustream_puts); + BIO_meth_set_gets(methods_ustream, s_ustream_gets); + BIO_meth_set_ctrl(methods_ustream, s_ustream_ctrl); + BIO_meth_set_create(methods_ustream, s_ustream_new); + BIO_meth_set_destroy(methods_ustream, s_ustream_free); + } bio = BIO_new(methods_ustream); BIO_set_data(bio, s);
Using the patch by Pan Chen as inspiration, this avoids a memory leak by using a global BIO_METHOD pointer that doesn't ordinarily need to be freed. CC: Pan Chen <serial115200@outlook.com> Signed-off-by: Eneas U de Queiroz <cotequeiroz@gmail.com> --- Run-tested with a WRT-3200ACM, running uclient_fetch and uhttpd. I have not run it with valgrind or any other debugger.