Unifying chunk buffer size logic

[4oomin]

Hi, I'm an engineer who use your lighttpd1.4
It would not be a bug on your side. But based on my experience, I think it can cause critical issue.

[1] Issue in the original code

1
Oversized buffer size logic is different between chunk_buffer_acquire_sz() and chunk_acquire().

2
According to chunk_acquire()
Every number of sz become size up to the nearest multiple number of chunk_buf_sz.
But only when sz is 8193, it size down to the nearest multiple number of chunk_buf_sz 8192.

[2] What I fix

I unify all the rule of sizing chunk buffer as sizing up to the nearest multiple number of chunk_buf_sz.
I know you try to avoid excess allocation by sizing down sz when sz is chunk_buf_sz*N+1
I explain below why I fixed it this way.

[3] Why I do this

1
Until recently, I use lighttpd 1.4.52 which is 7 years ago version.
With this version when ethernet speed is over 1Gbps, lighttpd fail to fork because of lack of memory.
I found out chunkqueue_append_buffer_open_sz() in chunk.c was related with it
It allocated new memory whenever sz is 4KB and sz is always 4KB when using 1Gbps ethernet.
So it caused memory leak.
I know it's not directly related with this issue, but these two issue are related with allocating additional space for '\0'
So it would be better to give additional space for '\0' regardless of the size of sz.

2
As I understand, it doesn't give extra space for '\0' when sz is chunk_buf_sz*N+1.
But it allocate extra space if sz is not chunk_buf_sz*N+1. So I think it lose some consistency.

Thank you

[gstrauss]

Please provide some more details about "what" is 4k and "when" this scenario occurs so that I can try to reproduce it. Also, please share your config settings, most importantly whether you have set server.chunkqueue-chunk-sz or not.

A suboptimal allocation strategy in some cases is one thing to look at.
Your claim of a memory leak is separate and more important, if true. Please provide more evidence.

I have a hunch that neither is true in the way you have presented and that your system is memory constrained. Depending on the configuration settings of server.chunkqueue-chunk-sz, lighttpd might save some allocated chunk blocks for reuse (and thereby memory use might increase temporarily), and lighttpd releases those unused chunk blocks every 64 seconds.

[4oomin]

[1] To make sure things before start

Before answering what you ask, I confirm what we are confused

First, the issue of failing fork happened on 1.4.52 version which is very old version.
After updating to 1.4.55, it didn't happened again.
I checked the code difference between that two version and found out that you fix the logic which I think causing the bug.

Second, there wasn't server.chunkqueue-chunk-sz field in lighttpd.conf.
So I used 4096 for chunk_buf_sz which you set in chunk.c as default.

[2] Code review

Back to what you ask, I will show you the logic which cuase failing fork on 1.4.52
when lighttpd read socket, the sequence would be as below

connection_read_cq(..) -> chunk_use_memory(...) -> chunkqueue_append_buffer_open_sz(...)

Focusing on chunkqueue_append_buffer_open_sz,

buffer * chunkqueue_append_buffer_open_sz(chunkqueue *cq, size_t sz) {
	chunk * const c = chunkqueue_append_mem_chunk(cq);
	if (buffer_string_space(c->mem) < sz) {
		chunkqueue_buffer_open_resize(c, sz);
	}
	return c->mem;
}

1. chunkqueue_append_mem_chunk allocate chunk having 4096 size buffer to chunk c at last
   I recommend you to track the code to buffer realloc(...) to understand how it allocates 4096 size buffer.

2. then it compare chunk c's buffer size and required size (size_t sz)
   buffer_string_space(c->mem) will return 4096-1 as following.
   (you would already know value of b->used if you do what I recommend step 1)

  static inline size_t buffer_string_space(const buffer *b) {
	return NULL != b && b->size ? b->size - (b->used | (0 == b->used)) : 0;
}

And Here's the problem, if required size (size_sz) was 4096
then it call chunkqueue_buffer_open_resize(c, sz) even though c->mem was allocated 4096 size buffer already

Jumping into chunkqueue_buffer_open_resize,

static void chunkqueue_buffer_open_resize(chunk *c, size_t sz) {
	chunk * const n = chunk_init((sz + 4095) & ~4095uL);
	buffer * const b = c->mem;
	c->mem = n->mem;
	n->mem = b;
	chunk_release(n);
}

3. it create new chunk n with 4096 size buffer by chunk_init
4. And it call chunk_release, it seems it will release previous chunk but it doesn't do that actually

static void chunk_release(chunk *c) {
    if (c->mem->size >= chunk_buf_sz) {
        chunk_reset(c);
        c->next = chunks;
        chunks = c;
    }
    else {
        chunk_free(c);
    }
}

the previous chunk size is also same as chunk_buf_sz.
because the previous chunk buffer was also created by chunk_init(chunk_buf_sz) at the beginning
or returned from reuse chunk list having only 4KB buffer
(which is same as my case that required sz was only 4KB continuously, so it has only 4KB size buffer in chunks (reuse list))

So it keep holding 4KB memory in chunk list rather than releasing memory actually.
Therefore it cause critical issue if required sz is 4KB continuously, it keep adding all the 4KB memory on chunk list.

In my case, the issue happened when trying to send a file larger than 30 MB through lighttpd.
I checked the lighttpd memory usage by /proc/[lighttpd pid]/status.
The lighttpd memory usage was exactly same as the file size at that time.

How it could be resolved in 1.4.55 version

1. On 1.4.55 version, it doesn't use the logic below anymore at chunkqueue_append_buffer_open_sz(...)

if (buffer_string_space(c->mem) < sz) {
		chunkqueue_buffer_open_resize(c, sz);
	}

Rather than that,
when chunk_init(...) is called, it allocates and resizes memory at once by using realloc().

[3] Why I pull request on 1.4.82 version

Finally, I think you will wonder then why I pull request even the issue doesn't happen again by updating to 1.4.55.
In fact, as I said at the first comment, this request is not directly related with 1.4.52 issue...😅

1.4.55 version (resolved 1.4.52 version issue)

static chunk * chunk_acquire(size_t sz) {
    if (sz <= chunk_buf_sz) {
        if (chunks) {
            chunk *c = chunks;
            chunks = c->next;
            return c;
        }
        sz = chunk_buf_sz;
    }
    else {
        /*(round up to nearest chunk_buf_sz)*/
        sz = (sz + (chunk_buf_sz-1)) & ~(chunk_buf_sz-1);
        chunk *c = chunk_pop_oversized(sz);
        if (c) return c;
    }

    return chunk_init(sz);
}

1.4.82 version (latest version)

static chunk * chunk_acquire(size_t sz) {
    if (sz <= (chunk_buf_sz|1)) {
        if (chunks) {
            chunk *c = chunks;
            chunks = c->next;
            return c;
        }
        sz = chunk_buf_sz;
    }
    else {
        /*(round up to nearest chunk_buf_sz)*/
        sz = (sz + (chunk_buf_sz-1)) & ~(chunk_buf_sz-1);
        chunk *c = chunk_pop_oversized(sz);
        if (c) return c;
    }

    return chunk_init_sz(sz);
}

As you can see, you change the first conditional statements
I know this change is for avoiding excess allocation like avoiding to allocate 24KB when 16K+1B is required.

If so, then I think you should also change sz = (sz + (chunk_buf_sz-1)) & ~(chunk_buf_sz-1) to
chunk_init_sz(((sz&~1uL)+(chunk_buf_sz-1)) & ~(chunk_buf_sz-1)) for consistency

If not changing like what I recommend, you could avoid excess allocation just only if required memory is chunk_buf_sz+ 1.
With sz = (sz + (chunk_buf_sz-1)) & ~(chunk_buf_sz-1), all the other case of (chunk_buf_sz * N) + 1 will allocate
chunk_buf_sz * (N+ 1)+1 sized memory. ( expected size was (chunk_buf_sz * N) + 1 )

So changing to chunk_init_sz(((sz&~1uL)+(chunk_buf_sz-1)) & ~(chunk_buf_sz-1)); will ensure that every (chunk_buf_sz * N) + 1 become (chunk_buf_sz * N)

But instead of that, I suggest revert it to 1.4.55 version.

Based on 1.4.55 version, when (chunk_buf_sz * N) + 1 size is required, then it return (chunk_buf_sz * N) + 1.
So there isn't extra space for '/0' unlike any other required sizes.

On the 1.4.52 version, on the other hand, it always ensure extra space for '/0' regardless of required size.

Thank you

[gstrauss]

lighttpd 1.4.52 was released Nov 2018, approaching 7 years ago.

I hope that you realize that while your ability to troubleshoot this is commendable, it is unfortunately negated by your oversight in not first testing to see if newer versions of lighttpd addressed have better optimizations. Issues such as https://redmine.lighttpd.net/issues/3033 were identified and fixed many years ago, but after lighttpd 1.4.55.

How it could be resolved in 1.4.55 version

Why should I care? lighttpd 1.4.55 is over 5 years ago. You seem to have neglected sharing an explanation of why you are incapable of using modern versions of lighttpd.

[4oomin]

please read part 3

[gstrauss]

Sorry. I was trying to answer quickly. I'll re-read in a few weeks. I am unable to give this further attention at this time.

Aside: it does not look good for you to keep misspelling the name of the variable chunk_buf_sz.

[4oomin]

Okay, thank you for quick replying and feedback.
(I fix the chunk_buff_sz to chunk_buf_sz)