Remi, On 11/3/21 9:47 AM, Remi Tricot-Le Breton wrote:
As for the second one, it would have been easier to simply add a string length comparison before the strcmp in my opinion. We would have had a one line fix instead of a full conversion of strXXX calls into ist equivalents (most of which worked fine thanks to the constant algorithm string length).
Your patch is already merged and the bug is fixed. However I'd like to comment on the reasons behind why I refactored the whole function to use the ist API:
I *strongly* dislike code that just works because of some implicit assumptions that might or might not hold after future changes. This is C, every messed up boundary check can easily result in some major issues.
In this specific case the implicit assumption is that all supported algorithms are exactly 5 characters long. This is true with the current implementation in HAProxy which just supports the HS, RS, ES, and PS families. However the JOSE specification [1] also specifies other values for the 'alg' field (though most of them for encrypted instead of just signed tokens) and they have differing lengths.
If someone in the future wants to add support for a 6 character algorithm, then they need to remember to add the length check to not run into the same strncmp() issue like you did. My experience is that if a mistake is made once, it is going to be made again.
The ist API already contains a large number of functions for *safe* handling of strings that are not NUL terminated. It is very hard to misuse them, because they all include the appropriate checks. If you see isteq(dynamic, ist("static")) then it is very clear that this will match the string "statis" exactly. For strncmp you will need to perform the length check yourself. You also need to remember to manually subtract 1 for the trailing NUL when using sizeof, whereas this is done automatically by the ist() function.
While I am not an expert in optimization and getting the last percent of performance out of a function, I don't think the ist API is going to be measurably slower than strncmp. It is used everywhere within HTX after all! For the JWT the OpenSSL crypto is going to take up the majority of the time anyway.
Hope this help to clarify why I'm strongly pushing the use of the ist API and why I've contributed a fair number of additional functions in the past when I encountered situations where the current functions were not ergonomic to use. One example is the istsplit() function which I added for uri_normalizer.c. It makes tokenization of strings very easy and safe.
Best regards Tim Düsterhus [1] https://www.iana.org/assignments/jose/jose.xhtml
