[These comments would have been better made during code review. I'm
sorry that I didn't have time to do this earlier. Idiosyncratic
excuse: I gain understanding while nit-picking (see
a5c5d5ca7828f3b3aa0384b22cc9154acd23f985)]
The loop structure of score_ends_seclabel doesn't look right to me.
But I don't actually know the logic that it is supposed to implement.
I may also be misunderstanding the code.
for each tsi
if there's a match with the spd "this"
remember the match_i index
for each tsr
if there is a match with the spd "this"
remember the match-r index
What's suspicious:
- There is no early-out for either loop for a match, so the last match
will be remembered. Not the best match. If any match will do, surely
we should stop the loop at the first.
- Both loops are matching against spd "this". Surely "that" should be
involved in one or the other
- the variables changed by the outer loop do not affect the inner
loop. It makes no sense to nest them. My guess: there is a reason
in the specs but it isn't in the code.
- match_r is initialized to false before the outer loop. Perhaps it
should also be initialized before the inner loop. This depends on
there being a reason to nest the loops.
- consider the code:
if (cur_i->sec_label.len == 0) {
/* ??? complain loudly */
continue;
}
(There is a related test in the inner loop too.)
I seem to remember being told that the labels, even though they are
a sequence of bytes with a length, are also NUL-terminated strings.
If so
1. this is dumb design: it is a redundant representation and
pointlessly introduces the ability to have invalid representations
(the NUL might be missing.)
2. the test, if it is meant to check for this problem, is only
partial. Something like this should be used.
if (cur_i->sec_label.len == 0 ||
cur_i->sec_label.ptr[cur_i->sec_label.len-1] !=
'\0')
{
/* ??? complain loudly */
continue;
}
3. but better: the test should be done when the data-structure is
ingested,
as part of input validation. Then the rest of Pluto need not deal
with
this.
4. this requirement should be documented in the RFC and in the code.
(Perhaps it is.)
_______________________________________________
Swan-dev mailing list
Swan-dev@lists.libreswan.org
https://lists.libreswan.org/mailman/listinfo/swan-dev
