On Thu, Aug 03, 2023 at 03:42:46PM +0200, Claudio Jeker wrote:
> iked has a special version of ibuf_size() called ibuf_length(). In the
> long run I want to remove this special case. The problem is that
> ibuf_length(NULL) returns 0 while ibuf_size() fails.
> Allowing the NULL pointer here results in bad code since it is no longer
> obvious if a buffer is initalised or not.
>
> So here is a first step on cleaning up this mess. It switches all
> ibuf_length() calls to ibuf_size() where it is obvious that the argument
> is not NULL (e.g. when ibuf_data(buf) is just at the same time).
> Also in some cases the check should actually be if buf == NULL since
> in those cases the buf is later allocated. (ikev2_pld.c and
> ikev2.c::ikev2_sa_responder()).
>
> Please double check if I did not introduce some error.
I agree that all the buffers you pass to ibuf_size() are non-NULL
either because they were previously checked, freshly allocated or
would lead to a code path where they would already be dereferenced
without your diff.
The conversions to NULL checks seem correct, so
ok tb.
One comment for tobhe and markus below
> --
> :wq Claudio
>
>
> Index: ca.c
> ===================================================================
> RCS file: /cvs/src/sbin/iked/ca.c,v
> retrieving revision 1.95
> diff -u -p -r1.95 ca.c
> --- ca.c 28 Jun 2023 14:10:24 -0000 1.95
> +++ ca.c 28 Jul 2023 11:29:25 -0000
> @@ -207,7 +207,7 @@ int
> ca_certbundle_add(struct ibuf *buf, struct iked_id *id)
> {
> uint8_t type = id->id_type;
> - size_t len = ibuf_length(id->id_buf);
> + size_t len = ibuf_size(id->id_buf);
> void *val = ibuf_data(id->id_buf);
>
> if (id == NULL ||
That id == NULL check doesn't make much sense given the above dereferences.
