v8 usage error in socket.cc::_getoption()

[GoogleCodeExporter]

Source:

http://code.google.com/p/v8cgi/source/browse/trunk/src/lib/socket/socket.cc

Function: _getoption()

char * buf = new char[length];
int result = getsockopt(sock, SOL_SOCKET, name, buf, (socklen_t *) 
&length);
if (result == 0) {
    v8::Handle<v8::Value> response = JS_STR(buf, length);
    delete[] buf;
}

Two comments:

a) You can get rid of the new/delete buf if you use std::vector<char>. You 
can use (&vec[0]) to pass such arrays to C APIs.

b) The conversion of buf to a string here has undefined results. If v8 sees 
any bytes with values >=128 then it assumes the string is UC16 and will try 
to decode it as such. Since the above is taking a pointer to an integer 
value as input, the result not be properly converted to a string most of 
the time (only when all 4 bytes at <=127).

Happy hacking!

Original issue reported on code.google.com by sgbeal@googlemail.com on 31 Mar 2010 at 9:24

[GoogleCodeExporter]

Hi Stephan,

thanks for review.

a) can you please show me an example? I am not sure if this is possible; calling
getsockopt can (imho) reallocate the buffer to increase its size...

b) the JS_STR macro is a shortcut to v8::String::New, in this case called as
v8::String::New(char *, int). According to docs, this will expect utf-8 encoded
string
(http://bespin.cz/~ondras/html/classv8_1_1String.html#66188e5be9378cad8cbf953053
de86bc),
so I do not see the point with UC16. On the other hand, my char-based approach
currently seems to make sense only with string-based socket options, correct?

Original comment by ondrej.zara on 31 Mar 2010 at 12:52

[GoogleCodeExporter]

Hi, Ondrej!

a) i've started work on a similar socket class, based on your code. See:

http://code.google.com/p/v8-juice/source/browse/extra-plugins/src/socket/socket.
cpp

and search for "std::vector", and you'll see how i've used that to remove the 
explicit 
allocs/deallocs.

b) i wasn't aware about the UTF8 requirement for String::New. In any case, the 
conversion of 4 
raw integer bytes to UTF-anything isn't valid here. Reminder to self: there's 
more info about 
the utf8/utf16/etc conversions here:

http://groups.google.com/group/v8-users/browse_thread/thread/1621beb71d65e6fa/28
83be6ddd10cddd

:)

Original comment by sgbeal@googlemail.com on 31 Mar 2010 at 3:26

[GoogleCodeExporter]

Hi, Stephen! Thanks for a prompt response...

1) binary in JS - this is indeed complicated. You might be interested in 
CommonJS
binary standardizations efforts so far (http://wiki.commonjs.org/wiki/Binary), 
but
note that none of those was ratified yet.

2) getopt/setopt - I conclude that my code is only valid for those socket 
options
which expect/return string values. It looks like a switch statement is 
necessary to
distinguish between character and numeric options.

3) std::vector - I am still not convinced that your example is correct. After 
looking
at your code, this is what I understand:

  a) a new std::vector<char> is created and pre-allocated to "len" length, all
positions filled with '\0'
  b) reference to first element of this vector is passed to {get,set}sockopt. This
works just fine for setsockopt, as this operation does not modify the buffer
  c) however, getsockopt is (imho?) free to reallocate the buffer you pass to it,
which might have destructive results: you are manipulating with your vector's 
memory
without letting the vector know...

Original comment by ondrej.zara on 31 Mar 2010 at 6:59

[GoogleCodeExporter]

Update: after checking manpage for a while, I see that my previous argumentation
regarding std::vector reallocation was wrong. getsockopt() cannot modify the 
buffer's
size, so your approach is perfectly safe.

In fact, it seems identical to "char buf[len]", but this syntax is 
unfortunately not
accepted by all compilers. From this point of view, the std:::vector approach 
truly
sounds like the best idea. Thanks! :)

Original comment by ondrej.zara on 31 Mar 2010 at 7:10

• Changed state: Accepted

[GoogleCodeExporter]

Commited in r945.

Original comment by ondrej.zara on 29 Nov 2011 at 9:03

• Changed state: Fixed