Add CRYPTO_get_alloc_counts. by richsalz · Pull Request #4467 · openssl/openssl

richsalz · 2017-10-05T01:18:50Z

No description provided.

paulidale · 2017-10-05T03:42:42Z

Should these be public? I'm not seeing an obvious need.

For memory allocation failure testing, it would be sufficient for the test suite to define its own local malloc/realloc/free that keep the counts without polluting the public interface.

You might also want to consider a reset/zero the counters capability.

If they are going to be public, they really should use atomic add operations for thread safety.

richsalz · 2017-10-05T11:48:39Z

Yes it can be done that way. Or it can be put under MEM_DEBUG ifdef if you prefer. And we don't guarantee thread-safety of our malloc so the guarantee isn't changed. Want it ifdef'd?

paulidale · 2017-10-05T12:15:53Z

pthreads(7) claims, under Posix at least, that malloc and friends are thread safe. That is unlikely to be true for all implementations on all platforms.

I'm somewhat ambivalent about conditioning them using MEM_DEBUG or including the counting implementation in test. It just doesn't feel like making them a public and supported API is right, I really am having difficulty coming up with a meaningful use case outside of memory stressing.

I still think a reset the counters command would be worthwhile -- this could be rolled into the query making it a query and zero operation.

I'm happy to go either way on the atomics -- the tests will have control over what happens and should be able to avoid nastinesses. Memory stress testing won't (initially) be hunting for concurrency issues.

paulidale · 2017-10-05T12:17:13Z

PS: I do like this idea from a stability and leak catching viewpoint.

richsalz · 2017-10-05T17:31:32Z

I think it makes sense, given the likely use-case, to make this only visible during MEM_DEBUG.
I don't (yet?) see a need to clear the counts.
Additional commit (to be squashed) that uses atomic_add (so have to be ints not long) and locks.

dot-asm · 2017-10-05T19:46:01Z

doc/man3/OPENSSL_malloc.pod

"two two". One "two" is redundant.

And what's with period after available?

dot-asm · 2017-10-05T19:49:02Z

doc/man3/OPENSSL_malloc.pod

Is it actually right spot for inserting this? I mean it's inserted between strongly related sentences, one that says that there are environment variables and one that describes just mentioned variables...

I'd swap value for pointer here. The pointer is NULL, the value is undefined.

I'm not sure. @dot-asm has a point but it isn't unreadable as it stands.

How about splitting so that the added function is mentioned and described first and then the addition of the two environment variable after?

If the library is built with the C option, an addition function is defined...

If the library is built with the C option, two additional environment variable modify the operation...

paulidale · 2017-10-05T20:52:55Z

doc/man3/OPENSSL_malloc.pod

values not value

dot-asm · 2017-10-06T06:46:18Z

crypto/mem.c

You do realize that CRYPTO_atomic_add(&x, 1, &dummy, malloc_lock) doesn't use malloc_lock on contemporary platforms. I mean the referred lock doesn't prevent rest of the code from modifying values. On contemporary platforms. In other words if accurate counters are all-important, then one shouldn't use CRYPTO_atomic_add.

It's the best we have, isn't it? Is there any other portable choice for atomic increment?

It's the best we have, isn't it?

What do you mean? What I'm trying to say is that maybe it's not right application for CRYPTO_atomic_add, in which case it doesn't matter if it's best or not. And if you can't use it, then you've obliged to do it the "usual way", i.e. lock, counter++, unlock. If we insist of atomic, then there would have to be atomic_compare_exchange, that would zero corresponding counter. So that there could be only one CRYPTO_get_alloc_counts caller and would be its responsibility to accumulate values if it calls CRYPTO_get_alloc_counts multiple times. Alternatively you can always say that in MEM_DEBUG scenario there will one call at the end, after everything is done, so it doesn't matter if it's locked or not...

I am not worried about multiple threads doing simultaneous allocation operations and the counts being off by a few. If someone has a better definition of INCREMENT for me to put in this PR, great.

I find it jarring as a reader when the formal thread-safety rules are not adhered to (at least without a comment attempting to explain the rule violation). Not mixing direct accesses to a variable and CRYPTO_atomic_add is one such rule.

so you like the code as-is, @kaduk?

so you like the code as-is

I can't quite tell what you're actually asking about, so I'll be a little flippant and note that there are some parts of the OpenSSL master source tree that I like. The current state of this pull request falls under the "jarring to read" category, though.

What do you think this PR is doing wrong? The values are set and fetched under the malloc_lock, even if some implementations don't use that lock.

The values are set and fetched under the malloc lock

But they are not set under the malloc_lock! INCREMENT(), aka. CRYPTO_atomic_add(&x, 1, &dummy, malloc_lock) expands to dummy = __atomic_add_fetch(&x, 1, __ATOMIC_ACQ_REL) on "most modern" systems, without ever acquiring malloc_lock. So, holding a write lock on malloc_lock does not block any concurrent INCREMENT calls.

richsalz · 2017-10-06T13:21:38Z

updated doc-fix commit pushed. thanks for the feedback so far!

richsalz · 2017-10-06T19:28:59Z

Okay, everything is written in terms of CRYPTO_atomic_add now.

paulidale · 2017-10-08T22:09:11Z

I've reinstated #4414 for consideration. It adds a CRYPTO_atomic_read call that pairs with the atomic add. This would be more useful than having GET add zero which will generate lock prefixes, a basic read doesn't on x64.

paulidale

I'm happy. The documentation change is trivial and not essential. The other outstanding is to use the CRYPRO_atomic_read for GET if that PR is accepted.

paulidale · 2017-10-08T22:20:40Z

doc/man3/OPENSSL_malloc.pod

s/the/that/

I'd say s/the/corresponding/

the corresponding is better again.

paulidale

Pending the atomic read ticket perhaps being first.

dot-asm · 2017-10-09T22:28:41Z

crypto/mem.c

On side note I find malloc_lock name extremely confusing, memdbg_lock would be better...

I can change it if you want. There are to malloc-lock and malloc-lock-long. additional commit tomorrow

richsalz · 2017-10-10T12:51:26Z

Additional commit to rename the lock pushed.

richsalz · 2017-10-10T13:22:45Z

A few changes, needs reconfirm. All commits will be squashed.

dot-asm · 2017-10-10T14:25:29Z

doc/man3/OPENSSL_malloc.pod

This kind of asks for what's German for "correspondingcount"?

kaduk

This seems okay, though it will introduce some churn into whatever solution to the global lock question we end up with. (That is, I am inclined to basically revert the addition of global locks, and if this lands first, 'git revert' will have conflicts. But they should be easily resolvable.)

Use atomic operations for the counters Rename malloc_lock to memdbg_lock Also fix some style errors in mem_dbg.c

richsalz · 2017-10-13T02:17:47Z

Thanks for the feedback and improvements all. Merged.

richsalz added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Oct 5, 2017

richsalz self-assigned this Oct 5, 2017

dot-asm reviewed Oct 5, 2017

View reviewed changes

paulidale reviewed Oct 5, 2017

View reviewed changes

doc/man3/OPENSSL_malloc.pod Outdated

Copy link

Contributor

paulidale Oct 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

values not value

dot-asm reviewed Oct 6, 2017

View reviewed changes

paulidale mentioned this pull request Oct 8, 2017

CRYPTO_atomic_read #4414

Closed

2 tasks

paulidale approved these changes Oct 8, 2017

View reviewed changes

paulidale approved these changes Oct 9, 2017

View reviewed changes

paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Oct 9, 2017

dot-asm reviewed Oct 9, 2017

View reviewed changes

dot-asm reviewed Oct 10, 2017

View reviewed changes

kaduk approved these changes Oct 10, 2017

View reviewed changes

paulidale approved these changes Oct 10, 2017

View reviewed changes

Add CRYPTO_get_alloc_counts.

77da11b

Use atomic operations for the counters Rename malloc_lock to memdbg_lock Also fix some style errors in mem_dbg.c

richsalz closed this Oct 13, 2017

richsalz deleted the count-mallocs branch October 13, 2017 02:17

Uh oh!

Comments

Conversation

richsalz commented Oct 5, 2017

Uh oh!

paulidale commented Oct 5, 2017

Uh oh!

richsalz commented Oct 5, 2017

Uh oh!

paulidale commented Oct 5, 2017

Uh oh!

paulidale commented Oct 5, 2017

Uh oh!

richsalz commented Oct 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richsalz commented Oct 6, 2017

Uh oh!

richsalz commented Oct 6, 2017

Uh oh!

paulidale commented Oct 8, 2017

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richsalz commented Oct 10, 2017

Uh oh!

richsalz commented Oct 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaduk left a comment

Choose a reason for hiding this comment

Uh oh!