[Swan-dev] shunk_t vs chunk_t

[D. Hugh Redelmeier]

| From: Andrew Cagney <andrew.cagney at gmail.com>

| On Mon, 1 Mar 2021 at 10:17, D. Hugh Redelmeier <hugh at 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.