Add a lock around the OBJ_NAME table by sfackler · Pull Request #3525 · openssl/openssl

sfackler · 2017-05-23T06:20:33Z

Various initialization functions modify this table, which can cause heap
corruption in the absence of external synchronization.

Fixes #3505

Various initialization functions modify this table, which can cause heap corruption in the absence of external synchronization. Fixes openssl#3505

mattcaswell · 2017-05-24T08:56:27Z

CLA received - thank you!

mattcaswell · 2017-05-24T09:00:47Z

crypto/objects/o_names.c

Too much indentation here! Indent the label one space and the other lines 4 spaces
Also the preferred style is now to not put () around a return expression (where we touch code that has that we remove it).

mattcaswell · 2017-05-24T09:02:40Z

crypto/objects/o_names.c

Remove the () for return expression.

mattcaswell · 2017-05-24T09:05:00Z

crypto/objects/o_names.c

I think a write lock is required here, lh_OBJ_NAME_retrieve() updates internal stats.

FWIW, there's other code which takes a read lock around lh_*_retrieve, like ssl_get_prev_session and int_err_get_item. But there's also performance reasons why you'd probably not want a write lock around those. I would have suggested taking those stats out, but sadly they're public API by way of OPENSSL_LH_stats_bio...

It looks like the tracked statistics are just a couple of counters, so we could swap over to atomic operations and avoid write locking.

Yes, that is a great idea. We have CRYPTO_atomic_add() for that. Do you want to pick that up as part of this PR, or should we defer it to another one?

yeah, agree with @sfackler's suggestion. hope he does it :)

I don't think we can take this PR without the atomic add stuff.

Yep, I'll update tonight.

Updated - it's kind of awful. Does anyone actually use OPENSSL_LH_stats_bio? It'd be great to remove the statistics tracking entirely and have that thing print a message about stats no longer being recorded.

Probably not. It was used by the since removed -stats flag for the errstr command. We removed it, and I've never seen anything notice.

yeah, but ... we tend to be real picky about compatibility. i'd like to turn it off, then remove it. but sigh ...

mattcaswell · 2017-05-24T09:05:48Z

crypto/objects/o_names.c

Too much indentation

sfackler · 2017-05-25T02:38:11Z

Updated!

mattcaswell

Approved subject to one minor nit being fixed.

mattcaswell · 2017-05-26T10:23:12Z

crypto/lhash/lhash_lcl.h

Wrong comment style. Multi-line comments should look like this:

/* * My multi-line * comment */

mattcaswell

Damn...meant to select "approve" not "request changes"....still subject to the one minor nit fix.

kroeckx · 2017-05-26T11:00:14Z

I think the comment style is also not supported by C89? We at least have no code with that style of comment.

sfackler · 2017-05-28T22:24:53Z

I think this is ready to land.

mattcaswell · 2017-05-30T08:03:05Z

Reconfirmed. Still looking for a second team approval.

richsalz

minor change, then approved.

richsalz · 2017-05-30T13:56:37Z

crypto/lhash/lhash.c

free of a NULL pointer is fine. As long as ret is init'd (via zalloc or explicitly), then you can collapse to one goto label.

Some stats are modified from OPENSSL_LH_retrieve, where callers aren't expecting to have to take out an exclusive lock. Switch to using atomic operations for those stats.

mattcaswell · 2017-05-31T08:18:16Z

@richsalz - can you confirm your approval?

richsalz · 2017-05-31T12:18:27Z

+1 reconfirm

sfackler · 2017-06-07T14:17:10Z

Is there anything left to do here on my end?

richsalz · 2017-06-07T14:28:22Z

no, just the occasional post as things fall through the cracks :)
i'll merge in a bit. thanks.

Various initialization functions modify this table, which can cause heap corruption in the absence of external synchronization. Some stats are modified from OPENSSL_LH_retrieve, where callers aren't expecting to have to take out an exclusive lock. Switch to using atomic operations for those stats. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #3525)

Various initialization functions modify this table, which can cause heap corruption in the absence of external synchronization. Some stats are modified from OPENSSL_LH_retrieve, where callers aren't expecting to have to take out an exclusive lock. Switch to using atomic operations for those stats. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #3525) (cherry picked from commit be606c0)

richsalz · 2017-06-07T17:07:29Z

merged at be606c0 in master and 1ec7eff in 1.1.0

Thank you for helping improve openssl!

sfackler · 2017-06-08T00:19:35Z

Thanks!

Various initialization functions modify this table, which can cause heap corruption in the absence of external synchronization. Some stats are modified from OPENSSL_LH_retrieve, where callers aren't expecting to have to take out an exclusive lock. Switch to using atomic operations for those stats. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from openssl#3525) (cherry picked from commit be606c0)

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label May 23, 2017

Add a lock around the OBJ_NAME table

aca3c7e

Various initialization functions modify this table, which can cause heap corruption in the absence of external synchronization. Fixes openssl#3505

mattcaswell removed the hold: cla required The contributor needs to submit a license agreement label May 24, 2017

mattcaswell requested changes May 24, 2017

View reviewed changes

mattcaswell added 1.1.0 branch: master Applies to master branch labels May 24, 2017

sfackler force-pushed the obj-name-sync-fix branch from efd1f7b to aca3c7e Compare May 25, 2017 02:37

mattcaswell requested changes May 26, 2017

View reviewed changes

mattcaswell approved these changes May 26, 2017

View reviewed changes

sfackler force-pushed the obj-name-sync-fix branch from 0a02153 to 1b636a4 Compare May 27, 2017 01:37

richsalz suggested changes May 30, 2017

View reviewed changes

Use atomic operations to adjust some LHASH stats

3b118a4

Some stats are modified from OPENSSL_LH_retrieve, where callers aren't expecting to have to take out an exclusive lock. Switch to using atomic operations for those stats.

sfackler force-pushed the obj-name-sync-fix branch from 1b636a4 to 3b118a4 Compare May 31, 2017 03:23

mattcaswell approved these changes May 31, 2017

View reviewed changes

little-dude mentioned this pull request May 31, 2017

add support for self-signed certificates seanmonstar/reqwest#97

Closed

richsalz self-assigned this Jun 7, 2017

richsalz closed this Jun 7, 2017

sfackler deleted the obj-name-sync-fix branch June 8, 2017 00:19

dot-asm mentioned this pull request Sep 26, 2017

Lock lhash stats. #4418

Closed

pslacerda mentioned this pull request Apr 17, 2018

Segmentation fault when connecting using SSL psycopg/psycopg2#543

Closed

marienz mentioned this pull request Feb 12, 2020

Segmentation fault after small patch drager/wasm-pack#650

Closed

Uh oh!

Conversation

sfackler commented May 23, 2017

Uh oh!

mattcaswell commented May 24, 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!

sfackler commented May 25, 2017

Uh oh!

mattcaswell 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!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

kroeckx commented May 26, 2017 via email

Uh oh!

sfackler commented May 28, 2017

Uh oh!

mattcaswell commented May 30, 2017

Uh oh!

richsalz 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!

mattcaswell commented May 31, 2017

Uh oh!

richsalz commented May 31, 2017

Uh oh!

sfackler commented Jun 7, 2017

Uh oh!

richsalz commented Jun 7, 2017

Uh oh!

richsalz commented Jun 7, 2017

Uh oh!

sfackler commented Jun 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants