[Nice] patch for libnice-0.1.1/stun/tools/stund.c

[John Selbie]

During my interop testing with stuntman (stun server) and its client
library code, I found a couple of bugs in the "stund" tool that builds with
libnice. I have provided a patch to fix the issue.

Issue: If stund tool receives a STUN binding request with an unknown
attribute, the code will get into an infinite spinning loop and not process
any more incoming packets.

Here is what is happening.

The dgram_process() function receives a datagram with a STUN binding
request.

If stun_agent_validate() returns UNKNOWN_REQUEST_ATTRIBUTE (which it will
do for almost any message with an attribute.)

Then stun_age_build_unknown_attributes_error() is called to build the
message.  Then a "goto send_buf".

Then send_safe is called. But the iov.iov_len hasn't been properly set.
It's initial value is 65552, which is longer than the length of a valid UDP
datagram.  So now sendmsg() is called inside safe_send() and returns -1
(errno==EMSGSIZE).  And then safe_send gets into an infinite loop and never
returns.

The simple fix is to move the stun_agent_finish_message() call to be below
the send_buf label instead of directly above it.  Diff below.

Two other issues.

1) stun/stunmessage.h defines STUN_MAX_MESSAGE_SIZE to be 65552.  For UDP,
this is impossible, because the maximum value for a 16-bit length field in
the UDP header is 65535. Subtract off the 8 byte UDP header, and that
leaves a theoretical maximum for a datagram payload buffer to be 65527 (or
65507 if the IP header is factored in).  And realistically, STUN messages
don't get anywhere near that size.

2) Also, safe_send does not look safe to me. Since the socket isn't
non-blocking or have any other special attributes, it doesn't make sense to
keep repeatedly trying to call sendmsg() until it succeeds. (At least not
without evaluating the errno). So in the diff below, I just removed it in
favor of a direct call to sendmsg.

I found this issue with the the stuntman client library code, when it sends
a binding request, it always sends a CHANGE-REQUEST attribute even if both
the ip and port flags are both false. The reason I have to do this is
because of a bug in JSTUN. The JSTUN server code has a bug that will reject
any message that doesn't have a CHANGE-REQUEST attribute.  But the stund
tool with libnice will reject any binding request with any attribute in it
(because the library call to tell the agent the acceptable attributes
aren't there.)

Here's the diff for stund.c

201,213d200
< /** Sends a message through a socket */
< ssize_t send_safe (int fd, const struct msghdr *msg)
< {
<   ssize_t len;
<
<   do
<     len = sendmsg (fd, msg, 0);
<   while ((len == -1) && (recv_err (fd) == 0));
<
<   return len;
< }
<
<
282d268
<   iov.iov_len = stun_agent_finish_message (agent, &response, NULL, 0);
283a270
>   iov.iov_len = stun_agent_finish_message (agent, &response, NULL, 0);
285,286c272,273
<   len = send_safe (sock, &mh);
<   return (len < iov.iov_len) ? -1 : 0;
---
>   len = sendmsg (sock, &mh, 0);
>   return len;


John Selbie
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/nice/attachments/20120208/36dabf59/attachment-0001.htm>