Opened 6 years ago

Closed 4 months ago

#8415 closed enhancement (implemented)

Use event_set_mem_functions

Reported by: nickm Owned by: rl1987
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy backend libevent malloc
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

We should use event_set_mem_functions so that Libevent also uses our memory allocation functions, as we do for openssl when we call CRYPTO_set_mem_ex_functions.

Child Tickets

Change History (25)

comment:1 Changed 6 years ago by nickm

Hm. My "as we do for openssl when we call CRYPTO_set_mem_ex_functions" seems to have been inaccurate; we apparently only do that when we have dmalloc enabled.

Maybe we should do that unconditionally too?

comment:2 Changed 6 years ago by arma

Fine with me either way. (What's the advantage to doing it unconditionally?)

comment:3 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.???

comment:4 Changed 2 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:5 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:6 Changed 20 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:7 Changed 20 months ago by nickm

Keywords: easy backend libevent malloc added
Severity: Normal

comment:8 Changed 14 months ago by aruna1234

When we use event_set_mem_functions Libevent also uses our memory allocation functions, but why is it necessary?

comment:9 Changed 14 months ago by teor

Keywords: tor-relay removed

So that when we use debugging allocators, or special allocators, we also debug or modify the allocations that Libevent makes on our behalf.

We also want to do something similar with our rust code, so it's allocations are made with the same allocator as the rest of Tor.

comment:10 Changed 14 months ago by aruna1234

and how can I do it?

comment:11 Changed 14 months ago by teor

Hi aruna1234,

Please focus on one or two tickets, finish those tickets, and then try a few more tickets.
We can't help you with a lot of tickets at the same time. It's too confusing for everyone.

comment:12 Changed 13 months ago by aruna1234

okay. I'll take care.

comment:13 Changed 4 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:14 Changed 4 months ago by rl1987

Status: acceptedneeds_review

comment:15 in reply to:  1 Changed 4 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.5.x-final
Status: needs_reviewneeds_revision

Thanks for this patch. I think we can take it in 0.3.5.

Do you plan on doing the CRYPTO_set_mem_ex_functions() call as well?

Replying to nickm:

Hm. My "as we do for openssl when we call CRYPTO_set_mem_ex_functions" seems to have been inaccurate; we apparently only do that when we have dmalloc enabled.

Maybe we should do that unconditionally too?

comment:16 Changed 4 months ago by rl1987

It seems that CRYPTO_set_mem_ex_functions is no longer present in modern OpenSSL. I added a call to CRYPTO_set_mem_functions in fd9d51c5fce9d2a3539c2f18665c3af1a8941d58.

However, on my Vagrant VM (which has OpenSSL 1.1.0f) there are following compiler warnings:

src/core/mainloop/main.c: In function ‘tor_run_main’:
src/core/mainloop/main.c:4240:28: warning: passing argument 1 of ‘CRYPTO_set_mem_functions’ from incompatible pointer type [-Wincompatible-pointer-types]
   CRYPTO_set_mem_functions(tor_malloc_,
                            ^~~~~~~~~~~
In file included from /usr/include/openssl/bn.h:33:0,
                 from /usr/include/openssl/engine.h:23,
                 from ./src/lib/crypt_ops/crypto_openssl_mgt.h:17,
                 from ./src/lib/crypt_ops/crypto_curve25519.h:15,
                 from ./src/lib/crypt_ops/crypto_ed25519.h:14,
                 from ./src/core/or/channel.h:15,
                 from src/core/mainloop/main.c:56:
/usr/include/openssl/crypto.h:262:5: note: expected ‘void * (*)(size_t,  const char *, int) {aka void * (*)(long unsigned int,  const char *, int)}’ but argument is of type ‘void * (*)(size_t) {aka void * (*)(long unsigned int)}’
 int CRYPTO_set_mem_functions(
     ^~~~~~~~~~~~~~~~~~~~~~~~
src/core/mainloop/main.c:4241:28: warning: passing argument 2 of ‘CRYPTO_set_mem_functions’ from incompatible pointer type [-Wincompatible-pointer-types]
                            tor_realloc_,
                            ^~~~~~~~~~~~
In file included from /usr/include/openssl/bn.h:33:0,
                 from /usr/include/openssl/engine.h:23,
                 from ./src/lib/crypt_ops/crypto_openssl_mgt.h:17,
                 from ./src/lib/crypt_ops/crypto_curve25519.h:15,
                 from ./src/lib/crypt_ops/crypto_ed25519.h:14,
                 from ./src/core/or/channel.h:15,
                 from src/core/mainloop/main.c:56:
/usr/include/openssl/crypto.h:262:5: note: expected ‘void * (*)(void *, size_t,  const char *, int) {aka void * (*)(void *, long unsigned int,  const char *, int)}’ but argument is of type ‘void * (*)(void *, size_t) {aka void * (*)(void *, long unsigned int)}’
 int CRYPTO_set_mem_functions(
     ^~~~~~~~~~~~~~~~~~~~~~~~
src/core/mainloop/main.c:4242:28: warning: passing argument 3 of ‘CRYPTO_set_mem_functions’ from incompatible pointer type [-Wincompatible-pointer-types]
                            tor_free_);
                            ^~~~~~~~~
In file included from /usr/include/openssl/bn.h:33:0,
                 from /usr/include/openssl/engine.h:23,
                 from ./src/lib/crypt_ops/crypto_openssl_mgt.h:17,
                 from ./src/lib/crypt_ops/crypto_curve25519.h:15,
                 from ./src/lib/crypt_ops/crypto_ed25519.h:14,
                 from ./src/core/or/channel.h:15,
                 from src/core/mainloop/main.c:56:
/usr/include/openssl/crypto.h:262:5: note: expected ‘void (*)(void *, const char *, int)’ but argument is of type ‘void (*)(void *)’
 int CRYPTO_set_mem_functions(
     ^~~~~~~~~~~~~~~~~~~~~~~~

Do we disregard these? It compiles without warnings on macOS (OpenSSL 1.1.0i) and on CI. OpenSSL declares their memory management functions with char * arguments, which is weird.

comment:17 Changed 4 months ago by teor

We can't ignore warnings, they make our CI and development environments fail. (We set --enable-gcc-warnings, which turns all warnings into errors.)

I think your box might have gcc 8, which recently started warning about function casts. I'm surprised that the Appveyor CI didn't fail on this code, because it also has gcc 8. (But it has existing failures, see #27389.)

I don't think it's safe to pass a different number of arguments than the caller expects:

               expected ‘void * (*)(size_t,  const char *, int) {aka void * (*)(long unsigned int,  const char *, int)}’
but argument is of type ‘void * (*)(size_t) {aka void * (*)(long unsigned int)}’

I suspect that the extra arguments are the location in the code of the function call (__FILE__, __LINE__). Check the OpenSSL documentation for the callback.

Since it's a callback, the file and line are less useful. And we have backtraces on most systems.

You could see if tor has another malloc function that takes file and line. If not, write a wrapper that ignores them, and calls tor_malloc().

comment:18 in reply to:  17 Changed 4 months ago by rl1987

Status: needs_revisionneeds_review

Replying to teor:

If not, write a wrapper that ignores them, and calls tor_malloc().

Did so in 39954c0765cf64d38e6ec64b66a9e016b75e5ee1.

comment:19 Changed 4 months ago by rl1987

Status: needs_reviewneeds_revision

Now it started failing on CI.

comment:20 Changed 4 months ago by rl1987

I suggest we back out of doing the OpenSSL part here, as CRYPTO_set_mem_functions and related function pointer types had some API changes over the last few years and it's rather problematic to get all API incarnations covered. I tried for some time now, and still don't have a patch that compiles without warnings universally.

I propose we cherry-pick the first commit (3fcdd33b0e3eb0360d168bcf98caffe215ffb3ac) I submitted for review to master.

comment:21 Changed 4 months ago by rl1987

Status: needs_revisionneeds_review

comment:22 Changed 4 months ago by rl1987

Opened #27593 for OpenSSL part.

comment:23 Changed 4 months ago by dgoulet

Reviewer: dgoulet

comment:24 Changed 4 months ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;

Some part of me would love a better abstraction (maybe a single central init function) so we can have one place where we initialize "libevent". For instance, the libevent RNG is setup in multiple steps in different places, and now that memory setup. I guess ordering matters maybe when we initialize libevent?...

Anyway, for quick fix up, this seems fine.

comment:25 Changed 4 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

lgtm; merging.

Note: See TracTickets for help on using tickets.