On 18/09/2021 17:53, Taylor Blau wrote:
parse_feature_value() does not update offset if the feature being
searched for does not specify a value. A loop that uses
parse_feature_value() to find a feature which was specified without a
value therefore might never exit (such loops will typically use
next_server_feature_value() as opposed to parse_feature_value() itself).
This usually isn't an issue: there's no point in using
next_server_feature_value() to search for repeated instances of the same
capability unless that capability typically specifies a value - but a
broken server could send a response that omits the value for a feature
even when we are expecting a value.
It may be worth adding a little detail here. parse_feature_value takes
an offset, and uses it to seek past the point in features_list that
we've already seen. But if we get a value-less feature, then offset is
never updated, and we'll keep parsing the same thing over and over in a
loop.
(I know that you know all of that, but I think it is worth spelling out
a little more clearly in the patch message).
Good point - I've tried to improve this for V2 (I've mostly just copied
your description verbatim).
Therefore we add an offset update calculation for the no-value case,
which helps ensure that loops using next_server_feature_value() will
always terminate.
next_server_feature_value(), and the offset calculation, were first
added in 2.28 in:
2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25)
This line wrapping is a little odd, but not a big deal.
I'll fix this for V2 - I think I tried too hard to make this look nice,
but putting the reference inline does look better (and I've now realised
this seems to be the usual way of doing things here).
ATB,
Andrzej
