<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, 8 May 2021 at 12:27, D. Hugh Redelmeier <<a href="mailto:hugh@mimosa.com">hugh@mimosa.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">| commit 9945236619b17fa13dfd1cfbe60359dcbf3fcd21<br>
| Author: D. Hugh Redelmeier <<a href="mailto:hugh@mimosa.com" target="_blank">hugh@mimosa.com</a>><br>
| Date:   Fri May 7 23:58:52 2021 -0400<br>
| <br>
|     pluto: packet.c: add consistency check to pbs_in_struct<br>
|     <br>
|     pbs_in_struct now requires that if and only if the structure has a<br>
|     length field then the obj_pbs isn't NULL.<br>
|     <br>
|     There are a few places where this part was intentionally ignored.<br>
|     This required adding a dummy pbs variable ("ignored").<br>
<br>
Structs, as understood by packet.h, sometimes have variable parts<br>
after the fixed fields.<br>
<br>
Here's an extract from the ancient comments at the head of<br>
pbs_in_struct.  The last parameter of pbs_in_struct is<br>
        struct pbs_in *obj_pbs"<br>
The phrase "is supplied" means "is not NULL".<br>
<br></blockquote><div><br></div><div>The old behavior was that a NULL implied that the nested struct, if any, could be ignored.  I'd call that obvious behaviour.</div><div>(passing a pbs when not needed was benign).</div><div><br></div><div>Now, if this is done wrong, and on a rarely executed code path, we've got a ticking time bomb.</div><div>It's no longer obvious from a simple visual check of the call that passing in NULL or not isn't going to explode.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
 * If obj_pbs is supplied, a new pb_stream is created for the<br>
 * variable part of the structure (this depends on their<br>
 * being one length field in the structure).  The cursor of this<br>
 * new PBS is set to after the parsed part of the struct.<br>
<br>
I was prompted to add this because it would have caught a mistake we<br>
made but never committed.  It has not caught any actual bugs in our<br>
code.  The more checking, the more confidence we can have in our code.<br></blockquote><div><br></div><div> Would it have crashed pluto?</div></div></div>