Hi again Nenad,
On Tue, Jul 17, 2018 at 05:18:45AM +0200, Willy Tarreau wrote:
> Hi Nenad,
>
> On Tue, Jul 17, 2018 at 03:37:37AM +0200, Nenad Merdanovic wrote:
> > Ugh, this was a long time ago. [FROM MEMORY] The element should not be
> > duplicated as far as I can remember. The references are stored in an ebtree
> > in order to prevent duplication and to provide consistent view when updated
> > dynamically.
>
> OK. Then maybe the elements are freed after scanning the ebtree as well,
> and we're meeting the node again after it was freed. I'll run a test
> with the memory debugging options to see if it crashes on first dereference.
OK I found it, the same keys_ref is indeed assigned to multiple bind_confs :
static int bind_parse_tls_ticket_keys(char **args, int cur_arg, struct proxy
*px, struct bind_conf *conf, char **err)
{
...
keys_ref = tlskeys_ref_lookup(args[cur_arg + 1]);
if(keys_ref) {
conf->keys_ref = keys_ref;
return 0;
}
So we clearly end up with a normal double-free. I'm checking what's the best
solution to fix this now, as we don't have flags nor any such thing in the
bind_conf to indicate that it must or must not be freed. We can't duplicate
the allocation either because it's used for the updates. I think the cleanest
solution will be to add a refcount to the tls_keys_ref entry.
Then I'm proposing the attached patch which works for me and is obvious
enough to make valgrind happy as well.
Could you guys please give it a try to confirm ? I'll then merge it.
Thanks,
Willy
>From 50588a4e3ccc92cdb0c8a347193192030f0ea9f0 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w...@1wt.eu>
Date: Tue, 17 Jul 2018 10:05:32 +0200
Subject: BUG/MINOR: ssl: properly ref-count the tls_keys entries
Commit 200b0fa ("MEDIUM: Add support for updating TLS ticket keys via
socket") introduced support for updating TLS ticket keys from the CLI,
but missed a small corner case : if multiple bind lines reference the
same tls_keys file, the same reference is used (as expected), but during
the clean shutdown, it will lead to a double free when destroying the
bind_conf contexts since none of the lines knows if others still use
it. The impact is very low however, mostly a core and/or a message in
the system's log upon old process termination.
Let's introduce some basic refcounting to prevent this from happening,
so that only the last bind_conf frees it.
Thanks to Janusz Dziemidowicz and Thierry Fournier for both reporting
the same issue with an easy reproducer.
This fix needs to be backported from 1.6 to 1.8.
---
include/types/ssl_sock.h | 1 +
src/ssl_sock.c | 6 ++++--
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/include/types/ssl_sock.h b/include/types/ssl_sock.h
index 8a5b3ee..2e02631 100644
--- a/include/types/ssl_sock.h
+++ b/include/types/ssl_sock.h
@@ -59,6 +59,7 @@ struct tls_keys_ref {
struct list list; /* Used to chain refs. */
char *filename;
int unique_id; /* Each pattern reference have unique id. */
+ int refcount; /* number of users of this tls_keys_ref. */
struct tls_sess_key *tlskeys;
int tls_ticket_enc_index;
__decl_hathreads(HA_RWLOCK_T lock); /* lock used to protect the ref */
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index b5547cc..3f70b12 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -4823,7 +4823,7 @@ void ssl_sock_destroy_bind_conf(struct bind_conf
*bind_conf)
ssl_sock_free_ssl_conf(&bind_conf->ssl_conf);
free(bind_conf->ca_sign_file);
free(bind_conf->ca_sign_pass);
- if (bind_conf->keys_ref) {
+ if (bind_conf->keys_ref && !--bind_conf->keys_ref->refcount) {
free(bind_conf->keys_ref->filename);
free(bind_conf->keys_ref->tlskeys);
LIST_DEL(&bind_conf->keys_ref->list);
@@ -7672,7 +7672,8 @@ static int bind_parse_tls_ticket_keys(char **args, int
cur_arg, struct proxy *px
}
keys_ref = tlskeys_ref_lookup(args[cur_arg + 1]);
- if(keys_ref) {
+ if (keys_ref) {
+ keys_ref->refcount++;
conf->keys_ref = keys_ref;
return 0;
}
@@ -7719,6 +7720,7 @@ static int bind_parse_tls_ticket_keys(char **args, int
cur_arg, struct proxy *px
i -= 2;
keys_ref->tls_ticket_enc_index = i < 0 ? 0 : i % TLS_TICKETS_NO;
keys_ref->unique_id = -1;
+ keys_ref->refcount = 1;
HA_RWLOCK_INIT(&keys_ref->lock);
conf->keys_ref = keys_ref;
--
1.7.12.1
