check null terminated messages #75
Conversation
| @@ -1,7 +1,5 @@ | |||
| // https://github.com/chuckremes/nn-core/blob/master/spec/nn_send_spec.rb | |||
|
|
|||
| var assert = require('assert'); | |||
| var should = require('should'); | |||
| var nano = require('../'); | |||
| var nn = nano._bindings; | |||
|
|
|||
There was a problem hiding this comment.
we can remove var nn = nano._bindings; here
There was a problem hiding this comment.
let's leave it for this PR and I'll target unnecessary requires in #74
|
huh, bunch of failed tests, I wonder if the utf8 is getting GC'd |
|
osx is passing. fails on my ubuntu server same as the tests.. hmm |
| size_t length = 0; | ||
|
|
||
| // check if Local<v8::Value> is a v8::String | ||
| if (args[1]->IsString()) { |
|
I'm quite certain that because I'm using args[1] directly, it's getting GC'd before libuv sends the message, so whatever is sent is completely random memory! Will need to copy the string. :( |
|
Also, this only seems to failing for strings, not buffers. |
|
just check for buffer instance otherwise assume string and don't return undefined. that might fix |
nickdesaulniers
force-pushed
the
null-term
branch
2 times, most recently
from
a67941a to
06054e2
Compare
|
pushing another test, not holding my breath though. I know we have no issues for buffers, but we definitely do for strings, since I believe we're adding the null byte (so this patch still doesn't address the whole point of the PR) |
|
ah! NO_NULL_TERMINATION |
|
yep, don't merge yet, I've got a bunch of ideas to clean this up |
|
so, this whole thing might have been a waste of time. I just added a test that sends a string, and sure enough it looks like we have nothing to worry about |
nickdesaulniers
force-pushed
the
null-term
branch
from
06054e2 to
9834e9f
Compare
|
that took some serious patch surgery to save just the tests. These tests help show we don't send null terminated msgs from the application layer. patch ready for review |
|
LGTM. btw, I think we may end up encountering some of this issue again in the near future when we introduce zero-copy send operations using the nn_allocmsg stuff |
reqshark
changed the title
|
sizzurp 727f552 |
nanomsg/nanomsg#363 (comment)
review? @reqshark