Skip to content

Commit

Permalink
Potential fix for nodejs#2438
Browse files Browse the repository at this point in the history 
- Save StringPtr if the header hasn't been completely received yet after one
  packet.
- Add one to num_fields and num_values. They were actually one less than the
  number of fields and values.
- Remove always_inline makes debugging difficult, and has negligible
  performance benefits.
  • Loading branch information
@ry
ry committed Jan 4, 2012
1 parent 2cde498 commit f3da6c6
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 33 deletions.
78 changes: 47 additions & 31 deletions src/node_http_parser.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,35 +103,20 @@ static char* current_buffer_data;
static size_t current_buffer_len;


// gcc 3.x knows the always_inline attribute but fails at build time with a
// "sorry, unimplemented: inlining failed" error when compiling at -O0
#if defined(__GNUC__)
# if __GNUC__ >= 4
# define always_inline __attribute__((always_inline))
# else
# define always_inline inline
# endif
#elif defined(_MSC_VER)
# define always_inline __forceinline
#else
# define always_inline
#endif


#define HTTP_CB(name) \
static int name(http_parser* p_) { \
Parser* self = container_of(p_, Parser, parser_); \
return self->name##_(); \
} \
int always_inline name##_()
int name##_()


#define HTTP_DATA_CB(name) \
static int name(http_parser* p_, const char* at, size_t length) { \
Parser* self = container_of(p_, Parser, parser_); \
return self->name##_(at, length); \
} \
int always_inline name##_(const char* at, size_t length)
int name##_(const char* at, size_t length)


static inline Persistent<String>
Expand Down Expand Up @@ -179,6 +164,19 @@ struct StringPtr {
}


// If str_ does not point to a heap string yet, this function makes it do
// so. This is called at the end of each http_parser_execute() so as not
// to leak references. See issue #2438 and test-http-parser-bad-ref.js.
void Save() {
if (!on_heap_ && size_ > 0) {
char* s = new char[size_];
memcpy(s, str_, size_);
str_ = s;
on_heap_ = true;
}
}


void Reset() {
if (on_heap_) {
delete[] str_;
Expand Down Expand Up @@ -237,7 +235,7 @@ class Parser : public ObjectWrap {


HTTP_CB(on_message_begin) {
num_fields_ = num_values_ = -1;
num_fields_ = num_values_ = 0;
url_.Reset();
return 0;
}
Expand All @@ -252,18 +250,20 @@ class Parser : public ObjectWrap {
HTTP_DATA_CB(on_header_field) {
if (num_fields_ == num_values_) {
// start of new field name
if (++num_fields_ == ARRAY_SIZE(fields_)) {
num_fields_++;
if (num_fields_ == ARRAY_SIZE(fields_)) {
// ran out of space - flush to javascript land
Flush();
num_fields_ = 0;
num_values_ = -1;
num_fields_ = 1;
num_values_ = 0;
}
fields_[num_fields_].Reset();
fields_[num_fields_ - 1].Reset();
}

assert(num_fields_ < (int)ARRAY_SIZE(fields_));
assert(num_fields_ == num_values_ + 1);

fields_[num_fields_].Update(at, length);
fields_[num_fields_ - 1].Update(at, length);

return 0;
}
Expand All @@ -272,13 +272,14 @@ class Parser : public ObjectWrap {
HTTP_DATA_CB(on_header_value) {
if (num_values_ != num_fields_) {
// start of new header value
values_[++num_values_].Reset();
num_values_++;
values_[num_values_ - 1].Reset();
}

assert(num_values_ < (int)ARRAY_SIZE(values_));
assert(num_values_ == num_fields_);

values_[num_values_].Update(at, length);
values_[num_values_ - 1].Update(at, length);

return 0;
}
Expand All @@ -302,7 +303,7 @@ class Parser : public ObjectWrap {
if (parser_.type == HTTP_REQUEST)
message_info->Set(url_sym, url_.ToString());
}
num_fields_ = num_values_ = -1;
num_fields_ = num_values_ = 0;

// METHOD
if (parser_.type == HTTP_REQUEST) {
Expand Down Expand Up @@ -364,7 +365,7 @@ class Parser : public ObjectWrap {
HTTP_CB(on_message_complete) {
HandleScope scope;

if (num_fields_ != -1)
if (num_fields_)
Flush(); // Flush trailing HTTP headers.

Local<Value> cb = handle_->Get(on_message_complete_sym);
Expand Down Expand Up @@ -401,6 +402,19 @@ class Parser : public ObjectWrap {
}


void Save() {
url_.Save();

for (int i = 0; i < num_fields_; i++) {
fields_[i].Save();
}

for (int i = 0; i < num_values_; i++) {
values_[i].Save();
}
}


// var bytesParsed = parser->execute(buffer, off, len);
static Handle<Value> Execute(const Arguments& args) {
HandleScope scope;
Expand Down Expand Up @@ -447,6 +461,8 @@ class Parser : public ObjectWrap {
size_t nparsed =
http_parser_execute(&parser->parser_, &settings, buffer_data + off, len);

parser->Save();

// Unassign the 'buffer_' variable
assert(current_buffer);
current_buffer = NULL;
Expand Down Expand Up @@ -515,9 +531,9 @@ class Parser : public ObjectWrap {
Local<Array> CreateHeaders() {
// num_values_ is either -1 or the entry # of the last header
// so num_values_ == 0 means there's a single header
Local<Array> headers = Array::New(2 * (num_values_ + 1));
Local<Array> headers = Array::New(2 * num_values_);

for (int i = 0; i < num_values_ + 1; ++i) {
for (int i = 0; i < num_values_; ++i) {
headers->Set(2 * i, fields_[i].ToString());
headers->Set(2 * i + 1, values_[i].ToString());
}
Expand Down Expand Up @@ -553,8 +569,8 @@ class Parser : public ObjectWrap {
void Init(enum http_parser_type type) {
http_parser_init(&parser_, type);
url_.Reset();
num_fields_ = -1;
num_values_ = -1;
num_fields_ = 0;
num_values_ = 0;
have_flushed_ = false;
got_exception_ = false;
}
Expand Down
6 changes: 4 additions & 2 deletions test/simple/test-http-parser.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ function expectBody(expected) {
//
(function() {
var request = Buffer(
'POST /it HTTP/1.1' + CRLF +
'POST /helpme HTTP/1.1' + CRLF +
'Content-Type: text/plain' + CRLF +
'Transfer-Encoding: chunked' + CRLF +
CRLF +
Expand All @@ -403,7 +403,7 @@ function expectBody(expected) {

parser.onHeadersComplete = mustCall(function(info) {
assert.equal(info.method, 'POST');
assert.equal(info.url || parser.url, '/it');
assert.equal(info.url || parser.url, '/helpme');
assert.equal(info.versionMajor, 1);
assert.equal(info.versionMinor, 1);
});
Expand All @@ -424,7 +424,9 @@ function expectBody(expected) {

for (var i = 1; i < request.length - 1; ++i) {
var a = request.slice(0, i);
console.error("request.slice(0, " + i + ") = ", JSON.stringify(a.toString()));
var b = request.slice(i);
console.error("request.slice(" + i + ") = ", JSON.stringify(b.toString()));
test(a, b);
}
})();
Expand Down

0 comments on commit f3da6c6

Please sign in to comment.