> I tried to find "missing malloc null check" using the following coccinelle
> script (easy to run from within CI)
Cool, that's nice. We tend to be strict with error checking in new code,
but having such a sanity check certainly won't hurt. If we only need to
fix half a dozen functions, it might actually be worth it. Thanks
> diff -u -p a/apps/openssl/apps.c b/apps/openssl/apps.c
> --- a/apps/openssl/apps.c
> +++ b/apps/openssl/apps.c
> @@ -379,6 +379,8 @@ password_callback(char *buf, int bufsiz,
> PW_MIN_LENGTH, bufsiz - 1);
> if (ok >= 0 && verify) {
> buff = malloc(bufsiz);
> +if (buff == NULL)
> +return;
> ok = UI_add_verify_string(ui, prompt, ui_flags, buff,
> PW_MIN_LENGTH, bufsiz - 1, buf);
UI_add_verify_string() handles a NULL buff correctly, although it is
far from obvious. We will end up in general_allocate_prompt() with
type == UIT_VERIFY and result_buf == NULL and will return -1 eventually.
If we wanted to NULL check the allocation here, we'd need to rework this
entire function (which we should probably do anyway).
> diff -u -p a/crypto/asn1/bio_ndef.c b/crypto/asn1/bio_ndef.c
> --- a/crypto/asn1/bio_ndef.c
> +++ b/crypto/asn1/bio_ndef.c
Already discussed.
> diff -u -p a/tests/ecdhtest.c b/tests/ecdhtest.c
> --- a/tests/ecdhtest.c
> +++ b/tests/ecdhtest.c
Thanks, I committed an appropriate version of this.
> diff -u -p a/ssl/d1_pkt.c b/ssl/d1_pkt.c
> --- a/ssl/d1_pkt.c
> +++ b/ssl/d1_pkt.c
> @@ -214,6 +214,8 @@ dtls1_buffer_record(SSL *s, record_pqueu
> return 0;
>
> rdata = malloc(sizeof(DTLS1_RECORD_DATA_INTERNAL));
> +if (rdata == NULL)
> +return;
> item = pitem_new(priority, rdata);
> if (rdata == NULL || item == NULL)
> goto init_err;
The checks actually happen in 'if (rdata == NULL || item == NULL)' above.
pitem_new() does not dereference rdata, so the error checks are fine as
they are.
We could rewrite this to do the following, with a corresponding
NULL-initialization of item at the top of the function. I would actually
prefer this.
if ((rdata = malloc(sizeof(*rdata))) == NULL)
goto init_err;
if ((item = pitem_new(priority, rdata)) == NULL)
goto init_err;
> @@ -260,6 +262,8 @@ dtls1_buffer_rcontent(SSL *s, rcontent_p
> return 0;
>
> rdata = malloc(sizeof(DTLS1_RCONTENT_DATA_INTERNAL));
> +if (rdata == NULL)
> +return;
> item = pitem_new(priority, rdata);
> if (rdata == NULL || item == NULL)
> goto init_err;
>
Exactly the same here.
