Project

General

Profile

Bug #1154

Segmentation fault in mod_proxy_backend_http.c

Added by snailfly over 12 years ago. Updated over 10 years ago.

Status:
Fixed
Priority:
Normal
Assignee:
Category:
mod_proxy_backend_http
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
Missing in 1.5.x:

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.

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

Related issues

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

Actions

Associated revisions

Revision 2443 (diff)
Added by stbuehler over 10 years ago

Fix segfault in mod_proxy_backend_http (fixes #1154)

History

#1

Updated by snailfly over 12 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.

#2

Updated by davidb54 over 10 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
#3

Updated by icy over 10 years ago

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

Updated by stbuehler over 10 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.

#5

Updated by stbuehler over 10 years ago

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

Applied in changeset r2443.

Also available in: Atom