From f9850aa5d2fbed3e039ac1a1015ff73065664f17 Mon Sep 17 00:00:00 2001
From: Monty Brandenberg <monty@lindenlab.com>
Date: Mon, 29 Apr 2013 17:09:13 -0400
Subject: [PATCH] BUG-2295/MAINT-2624 [FIXED] Crash in
HttpOpRequest::stageFromActive w/ Content-Range Don't rely on a response body
being present should a Content-Range header be parsed. Unit tests captured
the original crash and confirm the fix.
---
indra/llcorehttp/_httpoprequest.cpp | 10 +-
indra/llcorehttp/httpresponse.h | 9 +-
indra/llcorehttp/tests/test_httprequest.hpp | 152 +++++++++++++++++-
.../llcorehttp/tests/test_llcorehttp_peer.py | 84 ++++++++--
4 files changed, 231 insertions(+), 24 deletions(-)
diff --git a/indra/llcorehttp/_httpoprequest.cpp b/indra/llcorehttp/_httpoprequest.cpp
index 89dcd334f44..12eed06b108 100644
--- a/indra/llcorehttp/_httpoprequest.cpp
+++ b/indra/llcorehttp/_httpoprequest.cpp
@@ -4,7 +4,7 @@
*
* $LicenseInfo:firstyear=2012&license=viewerlgpl$
* Second Life Viewer Source Code
- * Copyright (C) 2012, Linden Research, Inc.
+ * Copyright (C) 2012-2013, Linden Research, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -186,9 +186,11 @@ void HttpOpRequest::stageFromActive(HttpService * service)
if (mReplyLength)
{
// If non-zero, we received and processed a Content-Range
- // header with the response. Verify that what it says
- // is consistent with the received data.
- if (mReplyLength != mReplyBody->size())
+ // header with the response. If there is received data
+ // (and there may not be due to protocol violations,
+ // HEAD requests, etc., see BUG-2295) Verify that what it
+ // says is consistent with the received data.
+ if (mReplyBody && mReplyBody->size() && mReplyLength != mReplyBody->size())
{
// Not as expected, fail the request
mStatus = HttpStatus(HttpStatus::LLCORE, HE_INV_CONTENT_RANGE_HDR);
diff --git a/indra/llcorehttp/httpresponse.h b/indra/llcorehttp/httpresponse.h
index 4a481db6ac5..f19b521fbfc 100644
--- a/indra/llcorehttp/httpresponse.h
+++ b/indra/llcorehttp/httpresponse.h
@@ -48,8 +48,9 @@ class HttpHeaders;
/// individual pieces of the response.
///
/// Typical usage will have the caller interrogate the object
-/// and return from the handler callback. Instances are refcounted
-/// and callers can bump the count and retain the object as needed.
+/// during the handler callback and then simply returning.
+/// But instances are refcounted and and callers can add a
+/// reference and hold onto the object after the callback.
///
/// Threading: Not intrinsically thread-safe.
///
@@ -119,6 +120,10 @@ class HttpResponse : public LLCoreInt::RefCounted
/// caller is going to have to make assumptions on receipt of
/// a 206 status. The @full value may also be zero in cases of
/// parsing problems or a wild-carded length response.
+ ///
+ /// These values will not necessarily agree with the data in
+ /// the body itself (if present). The BufferArray object
+ /// is authoritative for actual data length.
void getRange(unsigned int * offset, unsigned int * length, unsigned int * full) const
{
*offset = mReplyOffset;
diff --git a/indra/llcorehttp/tests/test_httprequest.hpp b/indra/llcorehttp/tests/test_httprequest.hpp
index e5488cf941e..16f39845bb0 100644
--- a/indra/llcorehttp/tests/test_httprequest.hpp
+++ b/indra/llcorehttp/tests/test_httprequest.hpp
@@ -4,7 +4,7 @@
*
* $LicenseInfo:firstyear=2012&license=viewerlgpl$
* Second Life Viewer Source Code
- * Copyright (C) 2012, Linden Research, Inc.
+ * Copyright (C) 2012-2013, Linden Research, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -2650,6 +2650,156 @@ void HttpRequestTestObjectType::test<21>()
}
}
+// BUG-2295 Tests - Content-Range header received but no body
+template <> template <>
+void HttpRequestTestObjectType::test<22>()
+{
+ ScopedCurlInit ready;
+
+ std::string url_base(get_base_url());
+ // std::cerr << "Base: " << url_base << std::endl;
+
+ set_test_name("BUG-2295");
+
+ // Handler can be stack-allocated *if* there are no dangling
+ // references to it after completion of this method.
+ // Create before memory record as the string copy will bump numbers.
+ TestHandler2 handler(this, "handler");
+
+ // record the total amount of dynamically allocated memory
+ mMemTotal = GetMemTotal();
+ mHandlerCalls = 0;
+
+ HttpRequest * req = NULL;
+
+ try
+ {
+ // Get singletons created
+ HttpRequest::createService();
+
+ // Start threading early so that thread memory is invariant
+ // over the test.
+ HttpRequest::startThread();
+
+ // create a new ref counted object with an implicit reference
+ req = new HttpRequest();
+ ensure("Memory allocated on construction", mMemTotal < GetMemTotal());
+
+ // ======================================
+ // Issue bug2295 GETs that will get a 206
+ // ======================================
+ mStatus = HttpStatus(206);
+ static const int test_count(3);
+ for (int i(0); i < test_count; ++i)
+ {
+ char buffer[128];
+ sprintf(buffer, "/bug2295/%d/", i);
+ HttpHandle handle = req->requestGetByteRange(HttpRequest::DEFAULT_POLICY_ID,
+ 0U,
+ url_base + buffer,
+ 0,
+ 25,
+ NULL,
+ NULL,
+ &handler);
+ ensure("Valid handle returned for ranged request", handle != LLCORE_HTTP_HANDLE_INVALID);
+ }
+
+ // Run the notification pump.
+ int count(0);
+ int limit(10);
+ while (count++ < limit && mHandlerCalls < test_count)
+ {
+ req->update(1000000);
+ usleep(100000);
+ }
+ ensure("Request executed in reasonable time", count < limit);
+ ensure("One handler invocation for each request", mHandlerCalls == test_count);
+
+ // ======================================
+ // Issue bug2295 GETs that will get a libcurl 18 (PARTIAL_FILE)
+ // ======================================
+ mStatus = HttpStatus(HttpStatus::EXT_CURL_EASY, CURLE_PARTIAL_FILE);
+ static const int test2_count(1);
+ for (int i(0); i < test2_count; ++i)
+ {
+ char buffer[128];
+ sprintf(buffer, "/bug2295/00000012/%d/", i);
+ HttpHandle handle = req->requestGetByteRange(HttpRequest::DEFAULT_POLICY_ID,
+ 0U,
+ url_base + buffer,
+ 0,
+ 25,
+ NULL,
+ NULL,
+ &handler);
+ ensure("Valid handle returned for ranged request", handle != LLCORE_HTTP_HANDLE_INVALID);
+ }
+
+ // Run the notification pump.
+ count = 0;
+ limit = 10;
+ while (count++ < limit && mHandlerCalls < (test_count + test2_count))
+ {
+ req->update(1000000);
+ usleep(100000);
+ }
+ ensure("Request executed in reasonable time", count < limit);
+ ensure("One handler invocation for each request", mHandlerCalls == (test_count + test2_count));
+
+ // ======================================
+ // Okay, request a shutdown of the servicing thread
+ // ======================================
+ mStatus = HttpStatus();
+ HttpHandle handle = req->requestStopThread(&handler);
+ ensure("Valid handle returned for second request", handle != LLCORE_HTTP_HANDLE_INVALID);
+
+ // Run the notification pump again
+ count = 0;
+ limit = 10;
+ while (count++ < limit && mHandlerCalls < (test_count + test2_count + 1))
+ {
+ req->update(1000000);
+ usleep(100000);
+ }
+ ensure("Second request executed in reasonable time", count < limit);
+ ensure("Second handler invocation", mHandlerCalls == (test_count + test2_count + 1));
+
+ // See that we actually shutdown the thread
+ count = 0;
+ limit = 10;
+ while (count++ < limit && ! HttpService::isStopped())
+ {
+ usleep(100000);
+ }
+ ensure("Thread actually stopped running", HttpService::isStopped());
+
+ // release the request object
+ delete req;
+ req = NULL;
+
+ // Shut down service
+ HttpRequest::destroyService();
+
+ ensure("4 + 1 handler calls on the way out", (test_count + test2_count + 1) == mHandlerCalls);
+
+#if defined(WIN32)
+ // Can only do this memory test on Windows. On other platforms,
+ // the LL logging system holds on to memory and produces what looks
+ // like memory leaks...
+
+ // printf("Old mem: %d, New mem: %d\n", mMemTotal, GetMemTotal());
+ ensure("Memory usage back to that at entry", mMemTotal == GetMemTotal());
+#endif
+ }
+ catch (...)
+ {
+ stop_thread(req);
+ delete req;
+ HttpRequest::destroyService();
+ throw;
+ }
+}
} // end namespace tut
diff --git a/indra/llcorehttp/tests/test_llcorehttp_peer.py b/indra/llcorehttp/tests/test_llcorehttp_peer.py
index 75a3c39ef2f..7f8f765366c 100644
--- a/indra/llcorehttp/tests/test_llcorehttp_peer.py
+++ b/indra/llcorehttp/tests/test_llcorehttp_peer.py
@@ -9,7 +9,7 @@
$LicenseInfo:firstyear=2008&license=viewerlgpl$
Second Life Viewer Source Code
-Copyright (C) 2012, Linden Research, Inc.
+Copyright (C) 2012-2013, Linden Research, Inc.
This library is free software; you can redistribute it and/or
modify it under the terms of the GNU Lesser General Public
@@ -47,6 +47,26 @@
class TestHTTPRequestHandler(BaseHTTPRequestHandler):
"""This subclass of BaseHTTPRequestHandler is to receive and echo
LLSD-flavored messages sent by the C++ LLHTTPClient.
+
+ Target URLs are fairly free-form and are assembled by
+ concatinating fragments. Currently defined fragments
+ are:
+ - '/reflect/' Request headers are bounced back to caller
+ after prefixing with 'X-Reflect-'
+ - '/fail/' Body of request can contain LLSD with
+ 'reason' string and 'status' integer
+ which will become response header.
+ - '/bug2295/' 206 response, no data in body:
+ -- '/bug2295/0/' "Content-Range: bytes 0-75/2983"
+ -- '/bug2295/1/' "Content-Range: bytes 0-75/*"
+ -- '/bug2295/2/' "Content-Range: bytes 0-75/2983",
+ "Content-Length: 0"
+ -- '/bug2295/00000018/0/' Generates PARTIAL_FILE (18) error in libcurl.
+ "Content-Range: bytes 0-75/2983",
+ "Content-Length: 76"
+
+ Some combinations make no sense, there's no effort to protect
+ you from that.
"""
def read(self):
# The following logic is adapted from the library module
@@ -107,22 +127,7 @@ def answer(self, data, withdata=True):
if "/sleep/" in self.path:
time.sleep(30)
- if "fail" not in self.path:
- data = data.copy() # we're going to modify
- # Ensure there's a "reply" key in data, even if there wasn't before
- data["reply"] = data.get("reply", llsd.LLSD("success"))
- response = llsd.format_xml(data)
- debug("success: %s", response)
- self.send_response(200)
- if "/reflect/" in self.path:
- self.reflect_headers()
- self.send_header("Content-type", "application/llsd+xml")
- self.send_header("Content-Length", str(len(response)))
- self.send_header("X-LL-Special", "Mememememe");
- self.end_headers()
- if withdata:
- self.wfile.write(response)
- else: # fail requested
+ if "fail" in self.path:
status = data.get("status", 500)
# self.responses maps an int status to a (short, long) pair of
# strings. We want the longer string. That's why we pass a string
@@ -138,6 +143,51 @@ def answer(self, data, withdata=True):
if "/reflect/" in self.path:
self.reflect_headers()
self.end_headers()
+ elif "/bug2295/" in self.path:
+ # Test for https://jira.secondlife.com/browse/BUG-2295
+ #
+ # Client can receive a header indicating data should
+ # appear in the body without actually getting the body.
+ # Library needs to defend against this case.
+ #
+ if "/bug2295/0/" in self.path:
+ self.send_response(206)
+ self.send_header("Content-Range", "bytes 0-75/2983")
+ elif "/bug2295/1/" in self.path:
+ self.send_response(206)
+ self.send_header("Content-Range", "bytes 0-75/*")
+ elif "/bug2295/2/" in self.path:
+ self.send_response(206)
+ self.send_header("Content-Range", "bytes 0-75/2983")
+ self.send_header("Content-Length", "0")
+ elif "/bug2295/00000012/0/" in self.path:
+ self.send_response(206)
+ self.send_header("Content-Range", "bytes 0-75/2983")
+ self.send_header("Content-Length", "76")
+ else:
+ # Unknown request
+ self.send_response(400)
+ if "/reflect/" in self.path:
+ self.reflect_headers()
+ self.send_header("Content-type", "text/plain")
+ self.end_headers()
+ # No data
+ else:
+ # Normal response path
+ data = data.copy() # we're going to modify
+ # Ensure there's a "reply" key in data, even if there wasn't before
+ data["reply"] = data.get("reply", llsd.LLSD("success"))
+ response = llsd.format_xml(data)
+ debug("success: %s", response)
+ self.send_response(200)
+ if "/reflect/" in self.path:
+ self.reflect_headers()
+ self.send_header("Content-type", "application/llsd+xml")
+ self.send_header("Content-Length", str(len(response)))
+ self.send_header("X-LL-Special", "Mememememe");
+ self.end_headers()
+ if withdata:
+ self.wfile.write(response)
def reflect_headers(self):
for name in self.headers.keys():
--
GitLab