UDP+binary+passive and prim_inet:recvfrom/3 bug?

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

UDP+binary+passive and prim_inet:recvfrom/3 bug?

Samuel Elliott
I wonder if this code in prim_inet is correct when using an UDP socket in
passive and binary modes:

recvfrom0(S, Length, Time) when port(S), integer(Length), Length >= 0 ->
    case ctl_cmd(S, ?UDP_REQ_RECV,[enc_time(Time),?int32(Length)]) of
        {ok,[R1,R0]} ->
            Ref = ?u16(R1,R0),
            receive
                {inet_async, S, Ref, {ok, [F,P1,P0 | AddrData]}} ->
                    {IP,Data} = get_ip(F, AddrData),
                    {ok, {IP, ?u16(P1,P0), Data}};
                {inet_async, S, Ref, Status} ->
                    Status
            end;
        Error ->
            Error
    end.

If the UDP socket has the "binary" option set, then I think that the
message that is received by the receive statement looks like

   {inet_async, S, Ref, {ok, <<F,P1,P0, AddrData/binary>>}}

which is covered by the default case instead of being covered explicitely,
as the result of recvfrom0/3 does not hold the expected {ok, {IP, Port, Data}}.
Of course, get_ip4/1 and get_ip6/1 (both called by get_ip/2) need a "binary"
alternative too.

  Sam




Reply | Threaded
Open this post in threaded view
|

UDP+binary+passive and prim_inet:recvfrom/3 bug?

Tony Rogvall-3
Samuel Tardieu wrote:

> I wonder if this code in prim_inet is correct when using an UDP socket in
> passive and binary modes:
>
> recvfrom0(S, Length, Time) when port(S), integer(Length), Length >= 0 ->
>     case ctl_cmd(S, ?UDP_REQ_RECV,[enc_time(Time),?int32(Length)]) of
>         {ok,[R1,R0]} ->
>             Ref = ?u16(R1,R0),
>             receive
>                 {inet_async, S, Ref, {ok, [F,P1,P0 | AddrData]}} ->
>                     {IP,Data} = get_ip(F, AddrData),
>                     {ok, {IP, ?u16(P1,P0), Data}};
>                 {inet_async, S, Ref, Status} ->
>                     Status
>             end;
>         Error ->
>             Error
>     end.
>
> If the UDP socket has the "binary" option set, then I think that the
> message that is received by the receive statement looks like
>
>    {inet_async, S, Ref, {ok, <<F,P1,P0, AddrData/binary>>}}
>
> which is covered by the default case instead of being covered explicitely,
> as the result of recvfrom0/3 does not hold the expected {ok, {IP, Port, Data}}.
> Of course, get_ip4/1 and get_ip6/1 (both called by get_ip/2) need a "binary"
> alternative too.
>
>   Sam

>From inet_drv.c

/*
** passive mode reply:
**        {inet_async, S, Ref, {ok,[H1,...Hsz | Data]}}
*/

This means that the inet_drv ADDs a header list of sz bytes. In this case
the header data is the Familiy, Port and Address, the rest my or may  not be a
binary.

/Tony

-------------- next part --------------
A non-text attachment was scrubbed...
Name: tony.vcf
Type: text/x-vcard
Size: 319 bytes
Desc: Card for Tony Rogvall
URL: <http://erlang.org/pipermail/erlang-questions/attachments/20010105/05651c2b/attachment.vcf>

Reply | Threaded
Open this post in threaded view
|

UDP+binary+passive and prim_inet:recvfrom/3 bug?

Samuel Elliott
On  5/01, Tony Rogvall wrote:

| From inet_drv.c
|
| /*
| ** passive mode reply:
| **        {inet_async, S, Ref, {ok,[H1,...Hsz | Data]}}
| */
|
| This means that the inet_drv ADDs a header list of sz bytes. In this case
| the header data is the Familiy, Port and Address, the rest my or may  not be a
| binary.

However, the following code:

-module (bug).
-export ([start_udp/0]).

start_udp () ->
  {ok, U} = gen_udp:open (4161, [binary, {active, false}]),
  io:format ("Received: ~p~n", [prim_inet:recvfrom (U, 1500)]),
  erlang:halt ().

gives, when a UDP packet is received on port 4161:

{ok,<<1,12,73,127,0,0,1,0,6,1,0,0,1,0,0,0,0,0,0,6,102,111,111,98,97,114,0,0,1,0,1>>}

If the binary option is removed, it gives:

{ok,{{127,0,0,1},
     3154,
     [0,6,1,0,0,1,0,0,0,0,0,0,6,102,111,111,98,97,114,0,0,1,0,1]}}




Reply | Threaded
Open this post in threaded view
|

UDP+binary+passive and prim_inet:recvfrom/3 bug?

Tony Rogvall-3
Samuel Tardieu wrote:

> On  5/01, Tony Rogvall wrote:
>
> | From inet_drv.c
> |
> | /*
> | ** passive mode reply:
> | **        {inet_async, S, Ref, {ok,[H1,...Hsz | Data]}}
> | */
> |
> | This means that the inet_drv ADDs a header list of sz bytes. In this case
> | the header data is the Familiy, Port and Address, the rest my or may  not be a
> | binary.
>
> However, the following code:
>
> -module (bug).
> -export ([start_udp/0]).
>
> start_udp () ->
>   {ok, U} = gen_udp:open (4161, [binary, {active, false}]),
>   io:format ("Received: ~p~n", [prim_inet:recvfrom (U, 1500)]),
>   erlang:halt ().
>
> gives, when a UDP packet is received on port 4161:
>
> {ok,<<1,12,73,127,0,0,1,0,6,1,0,0,1,0,0,0,0,0,0,6,102,111,111,98,97,114,0,0,1,0,1>>}
>
> If the binary option is removed, it gives:
>
> {ok,{{127,0,0,1},
>      3154,
>      [0,6,1,0,0,1,0,0,0,0,0,0,6,102,111,111,98,97,114,0,0,1,0,1]}}

Yes you are absolutely right, by demonstration :-)

If you need a quick fix you can update the inet_drv as follows:
(I can not produce a clean patch right now, I hope some one at OTP team can do it for
us)

for simplicity replace the following functions (a proper emacs mode will fix the looks
of this)

/Tony

------------------START CUTTING AND PASTING ----------------------------

static int inet_async_binary_data(desc, phsz, bin, offs, len)
inet_descriptor* desc; unsigned int phsz;
DriverBinary* bin; int offs; int len;
{
    unsigned int hsz = desc->hsz + phsz;
    DriverTermData spec[20];
    DriverTermData caller = desc->caller;
    int aid;
    int req;
    int i = 0;

    DEBUGF(("inet_async_binary_data(%d): offs=%d, len = %d\r\n",
     desc->port, offs, len));

    if (deq_async(desc, &aid, &caller, &req) < 0)
 return -1;

    i = LOAD_ATOM(spec, i, am_inet_async);
    i = LOAD_PORT(spec, i, desc->dport);
    i = LOAD_INT(spec, i,  aid);

    i = LOAD_ATOM(spec, i, am_ok);

    if ((desc->mode == INET_MODE_LIST) || (hsz > len)) {
 /* INET_MODE_LIST => [H1,H2,...Hn] */
 i = LOAD_STRING(spec, i, bin->orig_bytes+offs, len);
    }
    else {
 /* INET_MODE_BINARY => [H1,H2,...HSz | Binary] */
 int sz = len - hsz;
 i = LOAD_BINARY(spec, i, bin, offs+hsz, sz);
 if (hsz > 0)
     i = LOAD_STRING_CONS(spec, i, bin->orig_bytes+offs, hsz);
    }
    i = LOAD_TUPLE(spec, i, 2);
    i = LOAD_TUPLE(spec, i, 4);
    ASSERT(i <= 20);
    desc->caller = 0;
    return driver_send_term(desc->port, caller, spec, i);
}


static int tcp_reply_binary_data(desc, bin, offs, len)
tcp_descriptor* desc;  DriverBinary* bin; int offs; int len;
{
    int code;

    /* adjust according to packet type */
    switch(desc->inet.htype) {
    case TCP_PB_1:  offs += 1; len -= 1; break;
    case TCP_PB_2:  offs += 2; len -= 2; break;
    case TCP_PB_4:  offs += 4; len -= 4; break;
    case TCP_PB_FCGI:
 len -= ((struct fcgi_head*)(bin->orig_bytes+offs))->paddingLength;
 break;
    }

    SCANBIT8(INETP(desc), bin->orig_bytes+offs, len);

    if (desc->inet.deliver == INET_DELIVER_PORT)
        code = inet_port_binary_data(INETP(desc), bin, offs, len);
#ifdef USE_HTTP
    else if (desc->inet.htype == TCP_PB_HTTP) {
        if ((code = http_message(desc, bin->orig_bytes+offs, len)) < 0)
     http_error_message(desc, bin->orig_bytes+offs, len);
 code = 0;
    }
#endif
    else if (desc->inet.active == INET_PASSIVE)
      return inet_async_binary_data(INETP(desc), 0, bin, offs, len);
    else
 code = tcp_binary_message(desc, bin, offs, len);
    if (code < 0)
 return code;
    if (desc->inet.active == INET_ONCE)
 desc->inet.active = INET_PASSIVE;
    return code;
}


static int udp_reply_binary_data(desc, hsz, bin, offs, len)
inet_descriptor* desc; unsigned int hsz; DriverBinary* bin; int offs; int len;
{
    int code;

    SCANBIT8(desc, bin->orig_bytes+offs, len);

    if (desc->active == INET_PASSIVE)
 return inet_async_binary_data(desc, hsz, bin, offs, len);
    else if (desc->deliver == INET_DELIVER_PORT)
 code = inet_port_binary_data(desc, bin, offs, len);
    else
 code = udp_binary_message(desc, bin, offs, len);
    if (code < 0)
 return code;
    if (desc->active == INET_ONCE)
 desc->active = INET_PASSIVE;
    return code;
}

static int udp_inet_input(desc, event)
udp_descriptor* desc; HANDLE event;
{
    int n;
    int len;
    inet_address other;
    char abuf[sizeof(inet_address)];  /* buffer address */
    int sz;
    char* ptr;
    DriverBinary* buf; /* binary */
    int packet_count = INET_UDP_POLL;
    int count = 0;   /* number of packets delivered to owner */

    while(packet_count--) {
 len = sizeof(other);
 sz = desc->inet.bufsz;
 /* Allocate space for message and address */
 if ((buf = alloc_buffer(sz+len)) == NULL)
     return udp_error(desc, ENOMEM);
 ptr = buf->orig_bytes + len;  /* point to message part */

 /* Note: On Windows NT, recvfrom() fails if the socket is connected. */
 if (desc->inet.state & INET_F_ACTIVE) {
     n = sock_recv(desc->inet.s, ptr, sz, 0);
     other = desc->inet.remote;
 }
 else
     n = sock_recvfrom(desc->inet.s, ptr, sz, 0,
         (struct sockaddr*)&other, &len);
 if (n == SOCKET_ERROR) {
     int err = sock_errno();
     release_buffer(buf);
     if (err != ERRNO_BLOCK) {
  if (!desc->inet.active) {
      async_error(desc, err);
      driver_cancel_timer(desc->inet.port);
      sock_select(INETP(desc),FD_READ,0);
  }
  else {
      udp_error_message(desc, err);
  }
     }
     else if (!desc->inet.active)
  sock_select(INETP(desc),FD_READ,1);
     return count;  /* strange, not ready */
 }
 else {
     int offs;
     int nsz;
     int code;

     inet_input_count(INETP(desc), n);

     inet_get_address(desc->inet.sfamily, abuf, &other, &len);

     /* copy formatted address to ptr len is actual length */
     sys_memcpy(ptr - len, abuf, len);
     ptr -= len;
     nsz = n + len;                /* nsz = data + address */
     offs = ptr - buf->orig_bytes; /* initial pointer offset */

     /* check if we need to reallocate binary */
     if ((desc->inet.mode == INET_MODE_BINARY) &&
  (desc->inet.hsz < n) && (nsz < BIN_REALLOC_LIMIT(sz))) {
  DriverBinary* tmp;
  if ((tmp = realloc_buffer(buf,nsz+offs)) != NULL)
      buf = tmp;
     }
     code = udp_reply_binary_data(desc,(unsigned int)len,buf,offs,nsz);
     free_buffer(buf);
     if (code < 0)
  return count;
     count++;
     if (!desc->inet.active) {
  driver_cancel_timer(desc->inet.port); /* possibly cancel */
  sock_select(INETP(desc),FD_READ,0);
  return count;  /* passive mode (read one packet only) */
     }
 }
    }
    return count;
}

---------------------- YOU CAN STOP NOW -------------------------------------


-------------- next part --------------
A non-text attachment was scrubbed...
Name: tony.vcf
Type: text/x-vcard
Size: 319 bytes
Desc: Card for Tony Rogvall
URL: <http://erlang.org/pipermail/erlang-questions/attachments/20010108/9aa7d6e8/attachment.vcf>

Reply | Threaded
Open this post in threaded view
|

UDP+binary+passive and prim_inet:recvfrom/3 bug?

Raimo Niskanen-3
In reply to this post by Samuel Elliott
Samuel Tardieu wrote:

>
> On  5/01, Tony Rogvall wrote:
>
> | From inet_drv.c
> |
> | /*
> | ** passive mode reply:
> | **        {inet_async, S, Ref, {ok,[H1,...Hsz | Data]}}
> | */
> |
> | This means that the inet_drv ADDs a header list of sz bytes. In this case
> | the header data is the Familiy, Port and Address, the rest my or may  not be a
> | binary.
>
> However, the following code:
>
> -module (bug).
> -export ([start_udp/0]).
>
> start_udp () ->
>   {ok, U} = gen_udp:open (4161, [binary, {active, false}]),
>   io:format ("Received: ~p~n", [prim_inet:recvfrom (U, 1500)]),
>   erlang:halt ().
>
> gives, when a UDP packet is received on port 4161:
>
> {ok,<<1,12,73,127,0,0,1,0,6,1,0,0,1,0,0,0,0,0,0,6,102,111,111,98,97,114,0,0,1,0,1>>}
>
> If the binary option is removed, it gives:
>
> {ok,{{127,0,0,1},
>      3154,
>      [0,6,1,0,0,1,0,0,0,0,0,0,6,102,111,111,98,97,114,0,0,1,0,1]}}

The code in prim_inet is correct, but inet_drv.c needs some improvement.
We will look into this. It seems like the binary UDP case is not handled
correctly.

/ Raimo Niskanen, Ericsson Utvecklings AB - Erlang/OTP


Reply | Threaded
Open this post in threaded view
|

UDP+binary+passive and prim_inet:recvfrom/3 bug?

Raimo Niskanen-3
In reply to this post by Tony Rogvall-3
I will affect these changes to OTP R7B, include them in a patch, and get
back with more info about which patch.

/ Raimo Niskanen, Ericsson UAB, Erlang/OTP




Tony Rogvall wrote:

>
> Samuel Tardieu wrote:
>
> > On  5/01, Tony Rogvall wrote:
> >
> > | From inet_drv.c
> > |
> > | /*
> > | ** passive mode reply:
> > | **        {inet_async, S, Ref, {ok,[H1,...Hsz | Data]}}
> > | */
> > |
> > | This means that the inet_drv ADDs a header list of sz bytes. In this case
> > | the header data is the Familiy, Port and Address, the rest my or may  not be a
> > | binary.
> >
> > However, the following code:
> >
> > -module (bug).
> > -export ([start_udp/0]).
> >
> > start_udp () ->
> >   {ok, U} = gen_udp:open (4161, [binary, {active, false}]),
> >   io:format ("Received: ~p~n", [prim_inet:recvfrom (U, 1500)]),
> >   erlang:halt ().
> >
> > gives, when a UDP packet is received on port 4161:
> >
> > {ok,<<1,12,73,127,0,0,1,0,6,1,0,0,1,0,0,0,0,0,0,6,102,111,111,98,97,114,0,0,1,0,1>>}
> >
> > If the binary option is removed, it gives:
> >
> > {ok,{{127,0,0,1},
> >      3154,
> >      [0,6,1,0,0,1,0,0,0,0,0,0,6,102,111,111,98,97,114,0,0,1,0,1]}}
>
> Yes you are absolutely right, by demonstration :-)
>
> If you need a quick fix you can update the inet_drv as follows:
> (I can not produce a clean patch right now, I hope some one at OTP team can do it for
> us)
>
> for simplicity replace the following functions (a proper emacs mode will fix the looks
> of this)
>
> /Tony
>
> ------------------START CUTTING AND PASTING ----------------------------
>
> static int inet_async_binary_data(desc, phsz, bin, offs, len)
> inet_descriptor* desc; unsigned int phsz;
> DriverBinary* bin; int offs; int len;
> {
>     unsigned int hsz = desc->hsz + phsz;
>     DriverTermData spec[20];
>     DriverTermData caller = desc->caller;
>     int aid;
>     int req;
>     int i = 0;
>
>     DEBUGF(("inet_async_binary_data(%d): offs=%d, len = %d\r\n",
>      desc->port, offs, len));
>
>     if (deq_async(desc, &aid, &caller, &req) < 0)
>  return -1;
>
>     i = LOAD_ATOM(spec, i, am_inet_async);
>     i = LOAD_PORT(spec, i, desc->dport);
>     i = LOAD_INT(spec, i,  aid);
>
>     i = LOAD_ATOM(spec, i, am_ok);
>
>     if ((desc->mode == INET_MODE_LIST) || (hsz > len)) {
>  /* INET_MODE_LIST => [H1,H2,...Hn] */
>  i = LOAD_STRING(spec, i, bin->orig_bytes+offs, len);
>     }
>     else {
>  /* INET_MODE_BINARY => [H1,H2,...HSz | Binary] */
>  int sz = len - hsz;
>  i = LOAD_BINARY(spec, i, bin, offs+hsz, sz);
>  if (hsz > 0)
>      i = LOAD_STRING_CONS(spec, i, bin->orig_bytes+offs, hsz);
>     }
>     i = LOAD_TUPLE(spec, i, 2);
>     i = LOAD_TUPLE(spec, i, 4);
>     ASSERT(i <= 20);
>     desc->caller = 0;
>     return driver_send_term(desc->port, caller, spec, i);
> }
>
> static int tcp_reply_binary_data(desc, bin, offs, len)
> tcp_descriptor* desc;  DriverBinary* bin; int offs; int len;
> {
>     int code;
>
>     /* adjust according to packet type */
>     switch(desc->inet.htype) {
>     case TCP_PB_1:  offs += 1; len -= 1; break;
>     case TCP_PB_2:  offs += 2; len -= 2; break;
>     case TCP_PB_4:  offs += 4; len -= 4; break;
>     case TCP_PB_FCGI:
>  len -= ((struct fcgi_head*)(bin->orig_bytes+offs))->paddingLength;
>  break;
>     }
>
>     SCANBIT8(INETP(desc), bin->orig_bytes+offs, len);
>
>     if (desc->inet.deliver == INET_DELIVER_PORT)
>         code = inet_port_binary_data(INETP(desc), bin, offs, len);
> #ifdef USE_HTTP
>     else if (desc->inet.htype == TCP_PB_HTTP) {
>         if ((code = http_message(desc, bin->orig_bytes+offs, len)) < 0)
>      http_error_message(desc, bin->orig_bytes+offs, len);
>  code = 0;
>     }
> #endif
>     else if (desc->inet.active == INET_PASSIVE)
>       return inet_async_binary_data(INETP(desc), 0, bin, offs, len);
>     else
>  code = tcp_binary_message(desc, bin, offs, len);
>     if (code < 0)
>  return code;
>     if (desc->inet.active == INET_ONCE)
>  desc->inet.active = INET_PASSIVE;
>     return code;
> }
>
> static int udp_reply_binary_data(desc, hsz, bin, offs, len)
> inet_descriptor* desc; unsigned int hsz; DriverBinary* bin; int offs; int len;
> {
>     int code;
>
>     SCANBIT8(desc, bin->orig_bytes+offs, len);
>
>     if (desc->active == INET_PASSIVE)
>  return inet_async_binary_data(desc, hsz, bin, offs, len);
>     else if (desc->deliver == INET_DELIVER_PORT)
>  code = inet_port_binary_data(desc, bin, offs, len);
>     else
>  code = udp_binary_message(desc, bin, offs, len);
>     if (code < 0)
>  return code;
>     if (desc->active == INET_ONCE)
>  desc->active = INET_PASSIVE;
>     return code;
> }
>
> static int udp_inet_input(desc, event)
> udp_descriptor* desc; HANDLE event;
> {
>     int n;
>     int len;
>     inet_address other;
>     char abuf[sizeof(inet_address)];  /* buffer address */
>     int sz;
>     char* ptr;
>     DriverBinary* buf; /* binary */
>     int packet_count = INET_UDP_POLL;
>     int count = 0;   /* number of packets delivered to owner */
>
>     while(packet_count--) {
>  len = sizeof(other);
>  sz = desc->inet.bufsz;
>  /* Allocate space for message and address */
>  if ((buf = alloc_buffer(sz+len)) == NULL)
>      return udp_error(desc, ENOMEM);
>  ptr = buf->orig_bytes + len;  /* point to message part */
>
>  /* Note: On Windows NT, recvfrom() fails if the socket is connected. */
>  if (desc->inet.state & INET_F_ACTIVE) {
>      n = sock_recv(desc->inet.s, ptr, sz, 0);
>      other = desc->inet.remote;
>  }
>  else
>      n = sock_recvfrom(desc->inet.s, ptr, sz, 0,
>          (struct sockaddr*)&other, &len);
>  if (n == SOCKET_ERROR) {
>      int err = sock_errno();
>      release_buffer(buf);
>      if (err != ERRNO_BLOCK) {
>   if (!desc->inet.active) {
>       async_error(desc, err);
>       driver_cancel_timer(desc->inet.port);
>       sock_select(INETP(desc),FD_READ,0);
>   }
>   else {
>       udp_error_message(desc, err);
>   }
>      }
>      else if (!desc->inet.active)
>   sock_select(INETP(desc),FD_READ,1);
>      return count;  /* strange, not ready */
>  }
>  else {
>      int offs;
>      int nsz;
>      int code;
>
>      inet_input_count(INETP(desc), n);
>
>      inet_get_address(desc->inet.sfamily, abuf, &other, &len);
>
>      /* copy formatted address to ptr len is actual length */
>      sys_memcpy(ptr - len, abuf, len);
>      ptr -= len;
>      nsz = n + len;                /* nsz = data + address */
>      offs = ptr - buf->orig_bytes; /* initial pointer offset */
>
>      /* check if we need to reallocate binary */
>      if ((desc->inet.mode == INET_MODE_BINARY) &&
>   (desc->inet.hsz < n) && (nsz < BIN_REALLOC_LIMIT(sz))) {
>   DriverBinary* tmp;
>   if ((tmp = realloc_buffer(buf,nsz+offs)) != NULL)
>       buf = tmp;
>      }
>      code = udp_reply_binary_data(desc,(unsigned int)len,buf,offs,nsz);
>      free_buffer(buf);
>      if (code < 0)
>   return count;
>      count++;
>      if (!desc->inet.active) {
>   driver_cancel_timer(desc->inet.port); /* possibly cancel */
>   sock_select(INETP(desc),FD_READ,0);
>   return count;  /* passive mode (read one packet only) */
>      }
>  }
>     }
>     return count;
> }
>
> ---------------------- YOU CAN STOP NOW -------------------------------------