вт, 16 мая 2023 г. в 21:18, Theo Buehler <t...@theobuehler.org>:
> > 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.
>
"rdata" is passed to pitem_new, it is hard to check if it is dereferenced
there or not.
>
> 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;
>
it would be nice
>
> > @@ -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.
>
