[Swan-dev] why I added a consistency check to pbs_in_struct

[Andrew Cagney]

On Sat, 8 May 2021 at 12:27, D. Hugh Redelmeier <hugh at mimosa.com> wrote:

> | commit 9945236619b17fa13dfd1cfbe60359dcbf3fcd21
> | Author: D. Hugh Redelmeier <hugh at mimosa.com>
> | Date:   Fri May 7 23:58:52 2021 -0400
> |
> |     pluto: packet.c: add consistency check to pbs_in_struct
> |
> |     pbs_in_struct now requires that if and only if the structure has a
> |     length field then the obj_pbs isn't NULL.
> |
> |     There are a few places where this part was intentionally ignored.
> |     This required adding a dummy pbs variable ("ignored").
>
> Structs, as understood by packet.h, sometimes have variable parts
> after the fixed fields.
>
> Here's an extract from the ancient comments at the head of
> pbs_in_struct.  The last parameter of pbs_in_struct is
>         struct pbs_in *obj_pbs"
> The phrase "is supplied" means "is not NULL".
>
>
The old behavior was that a NULL implied that the nested struct, if any,
could be ignored.  I'd call that obvious behaviour.
(passing a pbs when not needed was benign).

Now, if this is done wrong, and on a rarely executed code path, we've got a
ticking time bomb.
It's no longer obvious from a simple visual check of the call that passing
in NULL or not isn't going to explode.


>  * If obj_pbs is supplied, a new pb_stream is created for the
>  * variable part of the structure (this depends on their
>  * being one length field in the structure).  The cursor of this
>  * new PBS is set to after the parsed part of the struct.
>
> I was prompted to add this because it would have caught a mistake we
> made but never committed.  It has not caught any actual bugs in our
> code.  The more checking, the more confidence we can have in our code.
>

 Would it have crashed pluto?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libreswan.org/pipermail/swan-dev/attachments/20210508/de5f01a7/attachment.html>