diff --git a/src/client.c b/src/client.c index a35d162..84b174e 100644 --- a/src/client.c +++ b/src/client.c @@ -1,7 +1,8 @@ #include "client.h" #include "serve.h" -#include "util.h" #include "ioutil.h" +#include "sockutil.h" +#include "util.h" #include "bitset.h" #include "nbdtypes.h" #include "self_pipe.h" @@ -141,7 +142,7 @@ void write_not_zeroes(struct client* client, uint64_t from, int len) * hand-optimized something specific. */ - int all_zeros = (zerobuffer[0] == 0) && + int all_zeros = (zerobuffer[0] == 0) && (0 == memcmp( zerobuffer, zerobuffer+1, blockrun-1 )); if ( !all_zeros ) { @@ -192,7 +193,7 @@ int client_read_request( struct client * client , struct nbd_request *out_reques FD_ZERO(&fds); FD_SET(client->socket, &fds); self_pipe_fd_set( client->stop_signal, &fds ); - fd_count = select(FD_SETSIZE, &fds, NULL, NULL, ptv); + fd_count = sock_try_select(FD_SETSIZE, &fds, NULL, NULL, ptv); if ( fd_count == 0 ) { /* This "can't ever happen" */ if ( NULL == ptv ) { fatal( "No FDs selected, and no timeout!" ); } @@ -347,30 +348,43 @@ void client_flush( struct client * client, size_t len ) int client_request_needs_reply( struct client * client, struct nbd_request request ) { - debug("request type %d", request.type); - + /* The client is stupid, but don't take down the whole server as a result. + * We send a reply before disconnecting so that at least some indication of + * the problem is visible, and so proxies don't retry the same (bad) request + * forever. + */ if (request.magic != REQUEST_MAGIC) { - fatal("Bad magic %08x", request.magic); + warn("Bad magic %08x from client", request.magic); + client_write_reply( client, &request, EBADMSG ); + client->disconnect = 1; // no need to flush + return 0; } + debug( + "request type=%"PRIu32", from=%"PRIu64", len=%"PRIu32, + request.type, request.from, request.len + ); + + /* check it's not out of range */ + if ( request.from+request.len > client->serve->size) { + warn("write request %"PRIu64"+%"PRIu32" out of range", + request.from, request.len + ); + if ( request.type == REQUEST_WRITE ) { + client_flush( client, request.len ); + } + client_write_reply( client, &request, EPERM ); /* TODO: Change to ERANGE ? */ + client->disconnect = 0; + return 0; + } + + switch (request.type) { case REQUEST_READ: break; case REQUEST_WRITE: - /* check it's not out of range */ - if ( request.from+request.len > client->serve->size) { - warn("write request %d+%d out of range", - request.from, - request.len - ); - client_write_reply( client, &request, 1 ); - client_flush( client, request.len ); - client->disconnect = 0; - return 0; - } break; - case REQUEST_DISCONNECT: debug("request disconnect"); client->disconnect = 1; @@ -432,7 +446,7 @@ void client_reply_to_write( struct client* client, struct nbd_request request ) * never consult the allocation_map until the builder thread has * finished. */ - bitset_set_range(client->serve->allocation_map, + bitset_set_range(client->serve->allocation_map, request.from, request.len); } diff --git a/tests/acceptance/fakes/source/connect_from_banned_ip.rb b/tests/acceptance/fakes/source/connect_from_banned_ip.rb index f87b099..aa16fa0 100755 --- a/tests/acceptance/fakes/source/connect_from_banned_ip.rb +++ b/tests/acceptance/fakes/source/connect_from_banned_ip.rb @@ -13,10 +13,8 @@ addr, port = *ARGV client = FakeSource.new( addr, port, "Timed out connecting", "127.0.0.6" ) sleep( 0.25 ) -client.ensure_disconnected +rsp = client.disconnected? ? 0 : 1 client.close -exit(0) - - +exit(rsp) diff --git a/tests/acceptance/file_writer.rb b/tests/acceptance/file_writer.rb index 8c20f53..92ad220 100644 --- a/tests/acceptance/file_writer.rb +++ b/tests/acceptance/file_writer.rb @@ -8,6 +8,10 @@ class FileWriter @pattern = "" end + def size + @blocksize * @pattern.split("").size + end + # We write in fixed block sizes, given by "blocksize" # _ means skip a block # 0 means write a block full of zeroes diff --git a/tests/acceptance/flexnbd/constants.rb b/tests/acceptance/flexnbd/constants.rb index 128a8bf..992031a 100644 --- a/tests/acceptance/flexnbd/constants.rb +++ b/tests/acceptance/flexnbd/constants.rb @@ -32,6 +32,9 @@ module FlexNBD end read_constants() + + REQUEST_MAGIC = "\x25\x60\x95\x13" unless defined?(REQUEST_MAGIC) + REPLY_MAGIC = "\x67\x44\x66\x98" unless defined?(REPLY_MAGIC) + end # module FlexNBD - diff --git a/tests/acceptance/flexnbd/fake_dest.rb b/tests/acceptance/flexnbd/fake_dest.rb index bae0531..25df522 100644 --- a/tests/acceptance/flexnbd/fake_dest.rb +++ b/tests/acceptance/flexnbd/fake_dest.rb @@ -56,8 +56,6 @@ module FlexNBD } end - REPLY_MAGIC="\x67\x44\x66\x98" - def write_error( handle ) write_reply( handle, 1 ) end @@ -76,7 +74,7 @@ module FlexNBD if opts[:magic] == :wrong write_rand( @sock, 4 ) else - @sock.write( REPLY_MAGIC ) + @sock.write( ::FlexNBD::REPLY_MAGIC ) end @sock.write( [err].pack("N") ) @@ -93,6 +91,10 @@ module FlexNBD @sock.read( len ) end + def write_data( len ) + @sock.write( len ) + end + def self.parse_be64(str) raise "String is the wrong length: 8 bytes expected (#{str.length} received)" unless @@ -161,3 +163,4 @@ module FlexNBD end # module FakeDest end # module FlexNBD + diff --git a/tests/acceptance/flexnbd/fake_source.rb b/tests/acceptance/flexnbd/fake_source.rb index 930c56a..dfbce69 100644 --- a/tests/acceptance/flexnbd/fake_source.rb +++ b/tests/acceptance/flexnbd/fake_source.rb @@ -30,7 +30,7 @@ module FlexNBD def read_hello() - timing_out( FlexNBD::MS_HELLO_TIME_SECS, + timing_out( ::FlexNBD::MS_HELLO_TIME_SECS, "Timed out waiting for hello." ) do fail "No hello." unless (hello = @sock.read( 152 )) && hello.length==152 @@ -47,15 +47,14 @@ module FlexNBD end - def send_request( type, handle="myhandle", from=0, len=0 ) + def send_request( type, handle="myhandle", from=0, len=0, magic=REQUEST_MAGIC ) fail "Bad handle" unless handle.length == 8 - @sock.write( "\x25\x60\x95\x13" ) + @sock.write( magic ) @sock.write( [type].pack( 'N' ) ) @sock.write( handle ) - @sock.write( "\x0"*4 ) - @sock.write( [from].pack( 'N' ) ) - @sock.write( [len ].pack( 'N' ) ) + @sock.write( [n64( from )].pack( 'q' ) ) + @sock.write( [len].pack( 'N' ) ) end @@ -122,10 +121,10 @@ module FlexNBD end - def ensure_disconnected - Timeout.timeout( 2 ) do - @sock.read(1) - end + def disconnected? + result = nil + Timeout.timeout( 2 ) { result = ( @sock.read(1) == nil ) } + result end @@ -140,6 +139,22 @@ module FlexNBD end end + private + + # take a 64-bit number, turn it upside down (due to : + # http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/11920 + # ) + def n64(b) + ((b & 0xff00000000000000) >> 56) | + ((b & 0x00ff000000000000) >> 40) | + ((b & 0x0000ff0000000000) >> 24) | + ((b & 0x000000ff00000000) >> 8) | + ((b & 0x00000000ff000000) << 8) | + ((b & 0x0000000000ff0000) << 24) | + ((b & 0x000000000000ff00) << 40) | + ((b & 0x00000000000000ff) << 56) + end end # class FakeSource end # module FlexNBD + diff --git a/tests/acceptance/test_serve_mode.rb b/tests/acceptance/test_serve_mode.rb new file mode 100644 index 0000000..1278e68 --- /dev/null +++ b/tests/acceptance/test_serve_mode.rb @@ -0,0 +1,86 @@ +require 'test/unit' +require 'environment' +require 'flexnbd/fake_source' + +class TestServeMode < Test::Unit::TestCase + + def setup + super + @env = Environment.new + @env.writefile1( "0" ) + @env.serve1 + end + + def teardown + @env.cleanup + super + end + + def connect_to_server + client = FlexNBD::FakeSource.new(@env.ip, @env.port1, "Connecting to server failed") + begin + result = client.read_hello + assert_equal "NBDMAGIC", result[:magic] + assert_equal @env.file1.size, result[:size] + yield client + ensure + client.close rescue nil + end + end + + def test_bad_request_magic_receives_error_response + connect_to_server do |client| + + # replace REQUEST_MAGIC with all 0s to make it look bad + client.send_request( 0, "myhandle", 0, 0, "\x00\x00\x00\x00" ) + rsp = client.read_response + + assert_equal FlexNBD::REPLY_MAGIC, rsp[:magic] + assert_equal "myhandle", rsp[:handle] + assert rsp[:error] != 0, "Server sent success reply back: #{rsp[:error]}" + + # The client should be disconnected now + assert client.disconnected?, "Server not disconnected" + end + end + + + def test_read_request_out_of_bounds_receives_error_response + connect_to_server do |client| + client.write_read_request( @env.file1.size, 4096 ) + rsp = client.read_response + + assert_equal FlexNBD::REPLY_MAGIC, rsp[:magic] + assert_equal "myhandle", rsp[:handle] + assert rsp[:error] != 0, "Server sent success reply back: #{rsp[:error]}" + + # Ensure we're not disconnected by sending a request. We don't care about + # whether the reply is good or not, here. + client.write_read_request( 0, 4096 ) + rsp = client.read_response + assert_equal FlexNBD::REPLY_MAGIC, rsp[:magic] + end + end + + def test_write_request_out_of_bounds_receives_error_response + connect_to_server do |client| + client.write( @env.file1.size, "\x00" * 4096 ) + rsp = client.read_response + + assert_equal FlexNBD::REPLY_MAGIC, rsp[:magic] + assert_equal "myhandle", rsp[:handle] + assert rsp[:error] != 0, "Server sent success reply back: #{rsp[:error]}" + + # Ensure we're not disconnected by sending a request. We don't care about + # whether the reply is good or not, here. + client.write( 0, "\x00" * @env.file1.size ) + rsp = client.read_response + assert_equal FlexNBD::REPLY_MAGIC, rsp[:magic] + end + + end + + + +end +