Project

General

Profile

Actions

Bug #1154

closed

Segmentation fault in mod_proxy_backend_http.c

Added by snailfly about 17 years ago. Updated about 15 years ago.

Status:
Fixed
Priority:
Normal
Category:
mod_proxy_backend_http
Target version:
ASK QUESTIONS IN Forums:

Description

When testing 15.0-svn r1820,we got Segmentation fault,after gdb digging,we found that in mod_proxy_backend_http.c line 196:
http://trac.lighttpd.net/trac/browser/trunk/src/mod_proxy_backend_http.c?rev=1820


        case HTTP_CHUNK_END:
            /* discard CRLF.*/
            for(ch = 0; (size_t)(c->offset) < (c->mem->used - 1) && ch != '\n' ;) {
                ch = c->mem->ptr[c->offset];
                c->offset++;
                in->bytes_out++;
            }

c->mem->used should never be less then one,but it's so Segmentation fault happend.change the code to follow can fix it:


        case HTTP_CHUNK_END:
            /* discard CRLF.*/
            for(ch = 0; c->mem->used >0 && (size_t)(c->offset) < (c->mem->used - 1) && ch != '\n' ;) {
                ch = c->mem->ptr[c->offset];
                c->offset++;
                in->bytes_out++;
            }

and more,base on http://trac.lighttpd.net/trac/changeset/1622 , maybe the other c->mem->used should be fix too.


Files

lighttpd_bug1154.patch (1.14 KB) lighttpd_bug1154.patch davidb54, 2009-01-07 09:21

Related issues 1 (0 open1 closed)

Has duplicate Bug #1882: SEGV in mod_proxy_backend_http.c: wrong int comparisonFixed2009-01-30Actions
Actions #1

Updated by snailfly about 17 years ago

Replying to snailfly:

c->mem->used should never be less then one,but it's so Segmentation fault happend.change the code to follow can fix it:

more in line 177:


            if (c->offset == 0 && we_want == we_have) {
                /* we are copying the whole buffer, just steal it */
                chunkqueue_steal_chunk(out, c);
            } else {
                b = chunkqueue_get_append_buffer(out);
                buffer_copy_string_len(b, c->mem->ptr + c->offset, we_want);
                c->offset += we_want;
            }

chunkqueue_steal_chunk change the value of c->mem-used to zero in some case.


mod_proxy_backend_http.c.185: (trace) befor chunkqueue_steal_chunk c->mem->used=5793
mod_proxy_backend_http.c.187: (trace) after chunkqueue_steal_chunk c->mem->used=0

chunkqueue_steal_chunk is found in chunk.c.

Actions #2

Updated by davidb54 over 15 years ago

I believe I have also experienced this bug. I was able to reproduce it while running lighttpd under valgrind. The valgrind output that indicates the source of the crash is included below.

The problem seems to arise in some cases where an entire lighttpd memory chunk is moved from the input queue to the output queue. It is possible for this lighttpd chunk transfer to bring the HTTP chunk parser to the HTTP_CHUNK_END state with an empty chunk (because of the chunkqueue_steal_chunk call) via switch/case fall-through. The HTTP_CHUNK_END case does not check for an empty chunk, and can thus cause a crash by dereferencing a NULL pointer (as indicated by the valgrind output).

It looks to me like the solution is to use the approach suggested by snailfly in the HTTP_CHUNK_END case only (check that c->mem->used > 0), and avoid reaching for another chunk if we are in the HTTP_CHUNK_END state with an empty chunk (which is only possible if we fell through from HTTP_CHUNK_DATA after reading all of the bytes that we were looking for). This allows the HTTP chunk parser to finish the request instead of marking it as "ran out of data".

I have attached a patch.

-dave

Valgrind output ============================================================================

32015 Invalid read of size 1
32015 at 0x64B36C3: proxy_http_stream_decoder (mod_proxy_backend_http.c:203)
32015 by 0x62A8E4F: proxy_stream_encode_decode (mod_proxy_core.c:847)
32015 by 0x62AABD6: proxy_state_engine (mod_proxy_core.c:1708)
32015 by 0x62AB3B7: mod_proxy_core_start_backend (mod_proxy_core.c:2395)
32015 by 0x41C0DE: plugins_call_handle_read_response_content (plugin.c:387)
32015 by 0x40DEDF: connection_state_machine (connections.c:1313)
32015 by 0x4091EB: lighty_mainloop (server.c:1005)
32015 by 0x40AC46: main (server.c:1773)
32015 Address 0x0 is not stack'd, malloc'd or (recently) free'd
Actions #3

Updated by icy over 15 years ago

  • Assignee changed from jan to stbuehler
  • Pending set to Yes
  • Patch available set to Yes
Actions #4

Updated by stbuehler about 15 years ago

  • Pending changed from Yes to No
  • Patch available changed from Yes to No

I don't see why don't want to get the next chunk if you stole the previous; that way you just behave like you already found CRLF, and i don't think that is a good idea.

Actions #5

Updated by stbuehler about 15 years ago

  • Status changed from Patch Pending to Fixed
  • % Done changed from 0 to 100

Applied in changeset r2443.

Actions

Also available in: Atom