On Thu, Oct 14, 2021 at 07:48:08PM +0200, Tim Duesterhus wrote: > Remi, > > please find a suggested cleanup for your JWT patch series. I think that > using the ist functions results in easier to understand code, because you > don't need to manually calculate lengths and offsets. > > Apply with `git am --scissors` to automatically cut the commit message. > > -- >8 -- > Using the ist helper functions is arguably cleaner than using C's regular > string functions on an ist.
It's indeed more readable but I must confess I'm now confused by both versions regarding what they're trying to do: > ctx.blk = NULL; > if (http_find_header(htx, hdr_name, &ctx, 0)) { > - char *space = NULL; > - space = memchr(ctx.value.ptr, ' ', ctx.value.len); > - if (space && strncasecmp("Bearer", ctx.value.ptr, > ctx.value.len) == 0) { > - chunk_initlen(&bearer_val, space+1, 0, ctx.value.len - > (space - ctx.value.ptr) - 1); I don't see how this can ever match: - we search for a space in the <len> first characters starting at <ptr> - if we find one such space, we check if these characters are exactly equal to the string "Bearer" (modulo the case), and if so we take the value. => by definition this cannot match since there is no space in "Bearer" It made me doubt about strncasecmp() to the point that I had to write some code to verify I wasn't mistaken about how it worked. Rémi, am I missing something or is it just that this code snippet indeed has a bug that was not spotted by the regtests (which I'm fine with, they're regression tests, not unit tests seeking 100% coverage) ? > + struct ist type = istsplit(&ctx.value, ' '); > + > + if (isteqi(type, ist("Bearer")) && istend(type) != > istend(ctx.value)) { > + chunk_initlen(&bearer_val, istptr(ctx.value), 0, > istlen(ctx.value)); Here I would find it more understandable to test like this: if (isteqi(type, ist("Bearer")) && istlen(ctx.value)) { Also, I had a check to RFC2617 and there's *at least* one space after the auth scheme, so we need to skip all of them. For me that would give this: struct ist type = istsplit(&ctx.value, ' '); ctx = istskip(ctx, ' '); if (isteqi(type, ist("Bearer")) && istlen(ctx.value)) chunk_initlen(&bearer_val, istptr(ctx.value), 0, istlen(ctx.value)); But I'd like someone to double-check all this, and possibly submit a fix if anything needs to be fixed (at least the multiple spaces case seems to require one). Thanks, Willy