| From: Andrew Cagney <andrew.cag...@gmail.com> | On Mon, 1 Mar 2021 at 10:17, D. Hugh Redelmeier <h...@mimosa.com> wrote: | > | > In shunk_t | > const void *ptr; | | BTW, it started out as const char * because it was being used on | strings. It changed to const void * as part of being made more | generic.
const char * is not great. const unsigned char * is better. C considers unsigned char to be the correct type for raw bytes. char can be signed and on some (rare!) machines 0xFF and 0x00 are equal chars. | The nice thing about 'const void *' is that C compilers are ok with it | being assigned to const pointers without a cast vis: | const uint8_t *p = shunk.ptr; | vs the bug: | uint8_t *p = (uint8_t*)shunk.ptr; It's true that const void * can be assigned to any (non-function) pointer-to-const type. But it cannot be indexed as if it were raw bytes. Do shunks or chunks ever hold anything other than raw bytes? I count ordinary C strings as raw bytes. | > I understand why the former is const and the latter isn't. | > But why do the base types differ? | | History. chunk_t being (unsigned) bytes is pretty entrenched. So I would have expected shunks to follow that. | > I stubbed my toe on this yesterday. in se_label_match: passert(a->ptr == NULL || (a->len > 0 && a->ptr[a->len - 1] == '\0')); When "a" was a shunk_t *, it required an ugly cast: passert(a->ptr == NULL || (a->len > 0 && ((const uint8_t *)a->ptr)[a->len - 1] == '\0')); | > ================ | > | > Sometimes security labels are passed around in shunk_t and sometimes | > chunk_t. This is awkward and widespread. I'm not sure if there is a good | > cure because I haven't looked carefully. | | > I chose to make the a and b parameters to se_label_match to have type | > chunk_t * because that required the fewest casts. | | Where? Here are the four calls to se_label match. Together there are eight chunk_t * parameters, each for a security label. In only one case is the parameter cast to chunk_t *. programs/pluto/connections.c:2480: && se_label_match(&sec_label, &sr->this.sec_label, logger) programs/pluto/ikev1_spdb_struct.c:114: if (!se_label_match((chunk_t *)&sec_label, &c->spd.this.sec_label, st->st_logger)) { programs/pluto/ikev2_ts.c:887: if (!se_label_match(&cur_i->sec_label, &d->spd.this.sec_label, logger)) { programs/pluto/ikev2_ts.c:903: } else if (se_label_match(&ends->r->sec_label, &d->spd.this.sec_label, logger)) { | - the sec label provided by the acquire is NUL terminated byte stream | (note, not string) (I hacked IKEv1 to enforce this, somewhere I've a | patch for IKEv2) Please do fix the IKEv2 case. Right now there is code that carelessly enforces the rule partially. For example, in two places in score_ends_seclabel(). | - since the sec_label bubbling up from the acquire points into a | readonly-ish stack structure it's arguable that it should be a shunk_t I would think so. | where things break down is with IKEv2's struct traffic_selector (IKEv1 | very clearly clones the sec_label into struct state). | | My hunch is that IKEv2's struct traffic_selector should be a shunk_t. | It starts out pointing into the pbs_in structure but then, when | selected and saved, is cloned and stored in the same fields as for | IKEv1 with lots of pointers updated. Is the only magic in freeing the shunk? Maybe we need a free_shunk_content() where this magic is contained. _______________________________________________ Swan-dev mailing list Swan-dev@lists.libreswan.org https://lists.libreswan.org/mailman/listinfo/swan-dev