Re: [PATCH] CLEANUP: h2: Help static analyzers understand the list's end marker

[Tim Düsterhus]

Willy,

Am 19.03.20 um 15:55 schrieb Willy Tarreau:
> Actually I'm pretty sure that I did it this way precisely for performance
> reasons: avoid repeatedly checking a pointer for half of the headers which
> are pseudo headers (method, scheme, authority, path just for the request).
> 
> It's perfectly possible that the difference is negligible though, but if
> it's not, I'm sorry but I'll favor performance over static analysers'
> own pleasure. So this one will definitely deserve a test.

Yes, the performance <-> static analyzer trade-off not preferring the
static analyzers is acceptable to me.

Best regards
Tim Düsterhus

Re: [PATCH] CLEANUP: h2: Help static analyzers understand the list's end marker

[Willy Tarreau]

Hi Tim,

On Thu, Mar 19, 2020 at 03:15:24PM +0100, Tim Duesterhus wrote:
> Willy,
> 
> I know you dislike adjusting code to please static analyzers, but I'd argue
> that using the new IST_NULL + isttest() combination is easier to understand 
> for humans as well. A simple .ptr == NULL check might also be slightly faster
> compared to isteq() with an empty string?
> 
> I have verified that reg-tests pass, but as this is deep within the internals
> please check this carefully.
> 
> Best regards
> Tim Düsterhus
> 
> Apply with `git am --scissors` to automatically cut the commit message.
> 
> -- >8 --
> Clang Static Analyzer (scan-build) was having a hard time to understand that
> `hpack_encode_header` would never be called with a garbage value for `v`.
> 
> It failed to detect that `if (isteq(n, ist(""))) break;` would exist the
> loop in all cases. By setting `n` to `IST_NULL` and checking with
> `isttest()` it no longer complains.

Actually I'm pretty sure that I did it this way precisely for performance
reasons: avoid repeatedly checking a pointer for half of the headers which
are pseudo headers (method, scheme, authority, path just for the request).

It's perfectly possible that the difference is negligible though, but if
it's not, I'm sorry but I'll favor performance over static analysers'
own pleasure. So this one will definitely deserve a test.

Thanks,
Willy

[PATCH] CLEANUP: h2: Help static analyzers understand the list's end marker

[Tim Duesterhus]

Willy,

I know you dislike adjusting code to please static analyzers, but I'd argue
that using the new IST_NULL + isttest() combination is easier to understand 
for humans as well. A simple .ptr == NULL check might also be slightly faster
compared to isteq() with an empty string?

I have verified that reg-tests pass, but as this is deep within the internals
please check this carefully.

Best regards
Tim Düsterhus

Apply with `git am --scissors` to automatically cut the commit message.

-- >8 --
Clang Static Analyzer (scan-build) was having a hard time to understand that
`hpack_encode_header` would never be called with a garbage value for `v`.

It failed to detect that `if (isteq(n, ist(""))) break;` would exist the
loop in all cases. By setting `n` to `IST_NULL` and checking with
`isttest()` it no longer complains.

The check must be moved to the beginning of the loop to prevent a NULL
pointer derefence for the pseudo header skip.
---
 src/mux_h2.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/mux_h2.c b/src/mux_h2.c
index 013ef86f8..b353c4d7c 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -4617,7 +4617,7 @@ static size_t h2s_frt_make_resp_headers(struct h2s *h2s, 
struct htx *htx)
}
 
/* marker for end of headers */
-   list[hdr].n = ist("");
+   list[hdr].n = IST_NULL;
 
if (h2s->status == 204 || h2s->status == 304) {
/* no contents, claim c-len is present and set to zero */
@@ -4660,6 +4660,9 @@ static size_t h2s_frt_make_resp_headers(struct h2s *h2s, 
struct htx *htx)
 
/* encode all headers, stop at empty name */
for (hdr = 0; hdr < sizeof(list)/sizeof(list[0]); hdr++) {
+   if (!isttest(list[hdr].n))
+   break; // end
+
/* these ones do not exist in H2 and must be dropped. */
if (isteq(list[hdr].n, ist("connection")) ||
isteq(list[hdr].n, ist("proxy-connection")) ||
@@ -4672,9 +4675,6 @@ static size_t h2s_frt_make_resp_headers(struct h2s *h2s, 
struct htx *htx)
if (*(list[hdr].n.ptr) == ':')
continue;
 
-   if (isteq(list[hdr].n, ist("")))
-   break; // end
-
if (!hpack_encode_header(&outbuf, list[hdr].n, list[hdr].v)) {
/* output full */
if (b_space_wraps(mbuf))
@@ -4870,7 +4870,7 @@ static size_t h2s_bck_make_req_headers(struct h2s *h2s, 
struct htx *htx)
}
 
/* marker for end of headers */
-   list[hdr].n = ist("");
+   list[hdr].n = IST_NULL;
 
mbuf = br_tail(h2c->mbuf);
  retry:
@@ -5007,6 +5007,9 @@ static size_t h2s_bck_make_req_headers(struct h2s *h2s, 
struct htx *htx)
struct ist n = list[hdr].n;
struct ist v = list[hdr].v;
 
+   if (!isttest(n))
+   break; // end
+
/* these ones do not exist in H2 and must be dropped. */
if (isteq(n, ist("connection")) ||
(auth.len && isteq(n, ist("host"))) ||
@@ -5030,9 +5033,6 @@ static size_t h2s_bck_make_req_headers(struct h2s *h2s, 
struct htx *htx)
if (*(n.ptr) == ':')
continue;
 
-   if (isteq(n, ist("")))
-   break; // end
-
if (!hpack_encode_header(&outbuf, n, v)) {
/* output full */
if (b_space_wraps(mbuf))
-- 
2.25.2