Re: [PATCH] MINOR: ssl: check transaction path before assigning it
On Sun, Nov 24, 2019 at 12:15 AM William Lallemand wrote: > That's a remain of the previous way of doing this, which was done with an > array of 2 old_ckch, so the previous check was something like: > > > if (!old_ckchs[0] && !old_ckchs[1]) > > When a transaction is created the old_ckchs and the path are set. > So we can check the path OR the old_ckchs, but it's not needed to do both, > since there are both set and free during a transaction at the same time. > > The goal there was to check if a transaction exists, not to check if we free'd > the path before setting it. ok got it, thanks for the explanation. In that case I will send a cleanup patch shortly, but I let you decide whether it is worth merging it or wait for another rework to integrate it. -- William
Re: [PATCH] MINOR: ssl: check transaction path before assigning it
On Sat, Nov 23, 2019 at 10:44:31PM +0100, William Dauchy wrote: > we were checking two times old_ckchs where we wanted to check whether > the transaction path was freed (and so null) before > > this should fix issue #351 > > Reported-by: Илья Шипицин > Signed-off-by: William Dauchy > --- > src/ssl_sock.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ssl_sock.c b/src/ssl_sock.c > index bcfa3e71..4cdb795f 100644 > --- a/src/ssl_sock.c > +++ b/src/ssl_sock.c > @@ -10468,7 +10468,7 @@ static int cli_parse_set_cert(char **args, char > *payload, struct appctx *appctx, > /* we succeed, we can save the ckchs in the transaction */ > > /* if there wasn't a transaction, update the old ckchs */ > - if (!ckchs_transaction.old_ckchs && !ckchs_transaction.old_ckchs) { > + if (!ckchs_transaction.old_ckchs && !ckchs_transaction.path) { > ckchs_transaction.old_ckchs = appctx->ctx.ssl.old_ckchs; > ckchs_transaction.path = appctx->ctx.ssl.path; > err = memprintf(&err, "Transaction created for certificate > %s!\n", ckchs_transaction.path); That's a remain of the previous way of doing this, which was done with an array of 2 old_ckch, so the previous check was something like: > if (!old_ckchs[0] && !old_ckchs[1]) When a transaction is created the old_ckchs and the path are set. So we can check the path OR the old_ckchs, but it's not needed to do both, since there are both set and free during a transaction at the same time. The goal there was to check if a transaction exists, not to check if we free'd the path before setting it. -- William Lallemand
[PATCH] MINOR: ssl: check transaction path before assigning it
we were checking two times old_ckchs where we wanted to check whether the transaction path was freed (and so null) before this should fix issue #351 Reported-by: Илья Шипицин Signed-off-by: William Dauchy --- src/ssl_sock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index bcfa3e71..4cdb795f 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -10468,7 +10468,7 @@ static int cli_parse_set_cert(char **args, char *payload, struct appctx *appctx, /* we succeed, we can save the ckchs in the transaction */ /* if there wasn't a transaction, update the old ckchs */ - if (!ckchs_transaction.old_ckchs && !ckchs_transaction.old_ckchs) { + if (!ckchs_transaction.old_ckchs && !ckchs_transaction.path) { ckchs_transaction.old_ckchs = appctx->ctx.ssl.old_ckchs; ckchs_transaction.path = appctx->ctx.ssl.path; err = memprintf(&err, "Transaction created for certificate %s!\n", ckchs_transaction.path); -- 2.24.0