Author: rhuijben Date: Tue Nov 3 23:09:09 2015 New Revision: 1712435 URL: http://svn.apache.org/viewvc?rev=1712435&view=rev Log: Resolve two memory leaks found by ivan.
While looking at this code also shrink the amount of memory needed at runtime while decoding http2 hpack data. * buckets/hpack_buckets.c (serf_hpack_context_t): Remove unnecessary reference. This value is stored in the bucket. (serf__bucket_hpack_create, serf__bucket_hpack_setx, serialize, serf_hpack_destroy_and_data): Update usage. (serf_hpack_decode_ctx_t): Remove variable. (serf__bucket_hpack_decode_create): Start by using a cheap small buffer. (hpack_decode_buffer_ensure): New helper function. (handle_read_entry_and_clear): Add argument. Update allocator usages. (hpack_process): Update caller. Ensure buffer when we know its size. (serf_hpack_decode_destroy): Relase the buffer when done. * buckets/prefix_buckets.c (serf_prefix_destroy): Release stream when done. * test/test_buckets.c (test_prefix_buckets): Recreate aggregate to handle changed behavior. Modified: serf/trunk/buckets/hpack_buckets.c serf/trunk/buckets/prefix_buckets.c serf/trunk/test/test_buckets.c Modified: serf/trunk/buckets/hpack_buckets.c URL: http://svn.apache.org/viewvc/serf/trunk/buckets/hpack_buckets.c?rev=1712435&r1=1712434&r2=1712435&view=diff ============================================================================== --- serf/trunk/buckets/hpack_buckets.c (original) +++ serf/trunk/buckets/hpack_buckets.c Tue Nov 3 23:09:09 2015 @@ -523,7 +523,6 @@ hpack_table_get(apr_uint64_t v, typedef struct serf_hpack_context_t { - serf_bucket_alloc_t *alloc; serf_hpack_table_t *tbl; serf_hpack_entry_t *first; @@ -592,7 +591,6 @@ serf__bucket_hpack_create(serf_hpack_tab { serf_hpack_context_t *ctx = serf_bucket_mem_alloc(allocator, sizeof(*ctx)); - ctx->alloc = allocator; ctx->tbl = hpack_table; ctx->first = ctx->last = NULL; @@ -611,7 +609,7 @@ serf__bucket_hpack_setc(serf_bucket_t *h } void -serf__bucket_hpack_setx(serf_bucket_t *hpack_bucket, +serf__bucket_hpack_setx(serf_bucket_t *bucket, const char *key, apr_size_t key_size, int key_copy, @@ -621,7 +619,7 @@ serf__bucket_hpack_setx(serf_bucket_t *h int dont_index, int never_index) { - serf_hpack_context_t *ctx = hpack_bucket->data; + serf_hpack_context_t *ctx = bucket->data; serf_hpack_entry_t *entry; apr_size_t i; @@ -639,9 +637,9 @@ serf__bucket_hpack_setx(serf_bucket_t *h if (entry && value[0] == ':') { if (entry->free_val) - serf_bucket_mem_free(ctx->alloc, (void*)entry->value); + serf_bucket_mem_free(bucket->allocator, (void*)entry->value); - entry->value = serf_bstrmemdup(ctx->alloc, value, value_size); + entry->value = serf_bstrmemdup(bucket->allocator, value, value_size); entry->value_len = value_size; entry->free_val = TRUE; entry->dont_index = never_index ? 2 : (dont_index ? 1 : 0); @@ -653,7 +651,7 @@ serf__bucket_hpack_setx(serf_bucket_t *h /* We probably want to allow duplicate *and* join behavior? */ } - entry = serf_bucket_mem_calloc(ctx->alloc, sizeof(*entry)); + entry = serf_bucket_mem_calloc(bucket->allocator, sizeof(*entry)); if (ctx->tbl && ctx->tbl->lowercase_keys) { @@ -664,7 +662,7 @@ serf__bucket_hpack_setx(serf_bucket_t *h encoding in HTTP/2. A request or response containing uppercase header field names MUST be treated as malformed (Section 8.1.2.6). */ - char *ckey = serf_bstrmemdup(ctx->alloc, key, key_size); + char *ckey = serf_bstrmemdup(bucket->allocator, key, key_size); for (i = 0; i < key_size; i++) { if (ckey[i] >= 'A' && key[i] <= 'Z') @@ -680,14 +678,14 @@ serf__bucket_hpack_setx(serf_bucket_t *h } else { - entry->key = serf_bstrmemdup(ctx->alloc, key, key_size); + entry->key = serf_bstrmemdup(bucket->allocator, key, key_size); entry->free_key = TRUE; } entry->key_len = key_size; if (value_copy) { - entry->value = serf_bstrmemdup(ctx->alloc, value, value_size); + entry->value = serf_bstrmemdup(bucket->allocator, value, value_size); entry->free_val = TRUE; } else @@ -783,7 +781,7 @@ serialize(serf_bucket_t *bucket) efficient HTTP2 / HPACK header format */ serf_hpack_context_t *ctx = bucket->data; - serf_bucket_alloc_t *alloc = ctx->alloc; + serf_bucket_alloc_t *alloc = bucket->allocator; serf_hpack_entry_t *entry; serf_hpack_entry_t *next; @@ -897,7 +895,7 @@ serf_hpack_destroy_and_data(serf_bucket_ { next = hi->next; - hpack_free_entry(hi, ctx->alloc); + hpack_free_entry(hi, bucket->allocator); } serf_default_destroy_and_data(bucket); @@ -922,7 +920,6 @@ const serf_bucket_type_t serf_bucket_typ typedef struct serf_hpack_decode_ctx_t { - serf_bucket_alloc_t *alloc; serf_hpack_table_t *tbl; serf_bucket_t *stream; @@ -991,7 +988,6 @@ serf__bucket_hpack_decode_create(serf_bu { serf_hpack_decode_ctx_t *ctx = serf_bucket_mem_calloc(alloc, sizeof(*ctx)); - ctx->alloc = alloc; ctx->tbl = hpack_table; ctx->stream = stream; ctx->max_entry_size = max_entry_size; @@ -999,7 +995,15 @@ serf__bucket_hpack_decode_create(serf_bu if (max_entry_size > 0x0100000) max_entry_size = 0x0100000; /* 1 MB */ - ctx->buffer_size = max_entry_size + 16; + /* The buffer should be large enough to keep a *compressed* key + or value and will be resized if necessary. + + Longer keys are more likely to use compression, so the default + should be enough for simple requests. + + (It is also used for compressed integer values, but there 10 bytes + should be enough to store a uint64 with 7 bits/byte) */ + ctx->buffer_size = 128; ctx->buffer_used = 0; ctx->buffer = serf_bucket_mem_alloc(alloc, ctx->buffer_size); @@ -1027,6 +1031,30 @@ serf__bucket_hpack_decode_create(serf_bu return serf_bucket_create(&serf_bucket_type__hpack_decode, alloc, ctx); } +static void +hpack_decode_buffer_ensure(serf_bucket_t *bucket, + apr_size_t minsize) +{ + serf_hpack_decode_ctx_t *ctx = bucket->data; + char *new_buffer; + + if (minsize < ctx->buffer_size) + return; + + while (minsize < ctx->buffer_size) + { + ctx->buffer_size *= 2; + } + + new_buffer = serf_bucket_mem_alloc(bucket->allocator, + ctx->buffer_size); + + /* In general only a small part of the old buffer is used at this point */ + memcpy(new_buffer, ctx->buffer, ctx->buffer_used); + serf_bucket_mem_free(bucket->allocator, ctx->buffer); + ctx->buffer = new_buffer; +} + static apr_status_t read_hpack_int(apr_uint64_t *v, unsigned char *flags, @@ -1104,7 +1132,8 @@ read_hpack_int(apr_uint64_t *v, } static apr_status_t -handle_read_entry_and_clear(serf_hpack_decode_ctx_t *ctx) +handle_read_entry_and_clear(serf_hpack_decode_ctx_t *ctx, + serf_bucket_alloc_t *alloc) { serf_hpack_table_t *tbl = ctx->tbl; const char *keep_key = NULL; @@ -1134,13 +1163,13 @@ handle_read_entry_and_clear(serf_hpack_d { ctx->wrote_header = TRUE; - b = SERF_BUCKET_SIMPLE_STRING("HTTP/2.0 ", ctx->alloc); + b = SERF_BUCKET_SIMPLE_STRING("HTTP/2.0 ", alloc); serf_bucket_aggregate_append(ctx->agg, b); - b = serf_bucket_simple_copy_create(ctx->val, ctx->val_size, ctx->alloc); + b = serf_bucket_simple_copy_create(ctx->val, ctx->val_size, alloc); serf_bucket_aggregate_append(ctx->agg, b); - b = SERF_BUCKET_SIMPLE_STRING(" <http2>\r\n", ctx->alloc); + b = SERF_BUCKET_SIMPLE_STRING(" <http2>\r\n", alloc); serf_bucket_aggregate_append(ctx->agg, b); } else if (ctx->key_size && ctx->key[0] == ':') @@ -1152,21 +1181,22 @@ handle_read_entry_and_clear(serf_hpack_d /* Write some header with some status code first */ ctx->wrote_header = TRUE; - b = SERF_BUCKET_SIMPLE_STRING("HTTP/2.0 505 Missing ':status' header\r\n", - ctx->alloc); + b = SERF_BUCKET_SIMPLE_STRING( + "HTTP/2.0 505 Missing ':status' header\r\n", + alloc); serf_bucket_aggregate_append(ctx->agg, b); /* And now the actual header */ - b = serf_bucket_simple_copy_create(ctx->key, ctx->key_size, ctx->alloc); + b = serf_bucket_simple_copy_create(ctx->key, ctx->key_size, alloc); serf_bucket_aggregate_append(ctx->agg, b); - b = SERF_BUCKET_SIMPLE_STRING(": ", ctx->alloc); + b = SERF_BUCKET_SIMPLE_STRING(": ", alloc); serf_bucket_aggregate_append(ctx->agg, b); - b = serf_bucket_simple_copy_create(ctx->val, ctx->val_size, ctx->alloc); + b = serf_bucket_simple_copy_create(ctx->val, ctx->val_size, alloc); serf_bucket_aggregate_append(ctx->agg, b); - b = SERF_BUCKET_SIMPLE_STRING("\r\n", ctx->alloc); + b = SERF_BUCKET_SIMPLE_STRING("\r\n", alloc); serf_bucket_aggregate_append(ctx->agg, b); } } @@ -1175,16 +1205,16 @@ handle_read_entry_and_clear(serf_hpack_d serf_bucket_t *b; /* Write header */ - b = serf_bucket_simple_copy_create(ctx->key, ctx->key_size, ctx->alloc); + b = serf_bucket_simple_copy_create(ctx->key, ctx->key_size, alloc); serf_bucket_aggregate_append(ctx->agg, b); - b = SERF_BUCKET_SIMPLE_STRING(": ", ctx->alloc); + b = SERF_BUCKET_SIMPLE_STRING(": ", alloc); serf_bucket_aggregate_append(ctx->agg, b); - b = serf_bucket_simple_copy_create(ctx->val, ctx->val_size, ctx->alloc); + b = serf_bucket_simple_copy_create(ctx->val, ctx->val_size, alloc); serf_bucket_aggregate_append(ctx->agg, b); - b = SERF_BUCKET_SIMPLE_STRING("\r\n", ctx->alloc); + b = SERF_BUCKET_SIMPLE_STRING("\r\n", alloc); serf_bucket_aggregate_append(ctx->agg, b); } @@ -1328,7 +1358,7 @@ hpack_process(serf_bucket_t *bucket) if (status) continue; - status = handle_read_entry_and_clear(ctx); + status = handle_read_entry_and_clear(ctx, bucket->allocator); if (status) return status; @@ -1368,6 +1398,8 @@ hpack_process(serf_bucket_t *bucket) if (v > ctx->max_entry_size) return SERF_ERROR_HTTP2_COMPRESSION_ERROR; + hpack_decode_buffer_ensure(bucket, (apr_size_t)v); + ctx->key_size = (apr_size_t)v; ctx->state = HPACK_DECODE_STATE_KEY; /* Fall through */ @@ -1448,6 +1480,7 @@ hpack_process(serf_bucket_t *bucket) || (v + ctx->key_size) > ctx->max_entry_size) return SERF_ERROR_HTTP2_COMPRESSION_ERROR; + hpack_decode_buffer_ensure(bucket, (apr_size_t)v); ctx->val_size = (apr_size_t)v; ctx->state = HPACK_DECODE_STATE_VALUE; /* Fall through */ @@ -1512,7 +1545,7 @@ hpack_process(serf_bucket_t *bucket) ctx->val_size); - status = handle_read_entry_and_clear(ctx); + status = handle_read_entry_and_clear(ctx, bucket->allocator); if (status) continue; @@ -1559,7 +1592,7 @@ hpack_process(serf_bucket_t *bucket) if (!ctx->item_callback) { /* Write the final "\r\n" for http/1.1 compatibility */ - b = SERF_BUCKET_SIMPLE_STRING("\r\n", ctx->alloc); + b = SERF_BUCKET_SIMPLE_STRING("\r\n", bucket->allocator); serf_bucket_aggregate_append(ctx->agg, b); } } @@ -1695,6 +1728,8 @@ serf_hpack_decode_destroy(serf_bucket_t if (ctx->agg) serf_bucket_destroy(ctx->agg); + serf_bucket_mem_free(bucket->allocator, ctx->buffer); + /* Key and value are handled by table. If we fail reading table can't be used anyway, so the allocator cleanup will handle the leak */ Modified: serf/trunk/buckets/prefix_buckets.c URL: http://svn.apache.org/viewvc/serf/trunk/buckets/prefix_buckets.c?rev=1712435&r1=1712434&r2=1712435&view=diff ============================================================================== --- serf/trunk/buckets/prefix_buckets.c (original) +++ serf/trunk/buckets/prefix_buckets.c Tue Nov 3 23:09:09 2015 @@ -221,6 +221,8 @@ static void serf_prefix_destroy(serf_buc if (ctx->buffer) serf_bucket_mem_free(bucket->allocator, ctx->buffer); + serf_bucket_destroy(ctx->stream); + serf_default_destroy_and_data(bucket); } Modified: serf/trunk/test/test_buckets.c URL: http://svn.apache.org/viewvc/serf/trunk/test/test_buckets.c?rev=1712435&r1=1712434&r2=1712435&view=diff ============================================================================== --- serf/trunk/test/test_buckets.c (original) +++ serf/trunk/test/test_buckets.c Tue Nov 3 23:09:09 2015 @@ -1970,6 +1970,7 @@ static void test_prefix_buckets(CuTest * serf_bucket_destroy(prefix); /* Then more than first chunk*/ + agg = serf_bucket_aggregate_create(alloc); bkt = SERF_BUCKET_SIMPLE_STRING(BODY, alloc); serf_bucket_aggregate_append(agg, bkt); bkt = SERF_BUCKET_SIMPLE_STRING(BODY, alloc); @@ -1987,6 +1988,7 @@ static void test_prefix_buckets(CuTest * serf_bucket_destroy(prefix); /* And an early EOF */ + agg = serf_bucket_aggregate_create(alloc); bkt = SERF_BUCKET_SIMPLE_STRING(BODY, alloc); serf_bucket_aggregate_append(agg, bkt);