Handling CoAP.SocketServer errors

When the SocketServer attempts to decode messages there is the
possibility of error caused by bad data.

This commit adapts the code so that any exceptions caused by decoding
are captured, and a warning is logged, and the process is stopped
without causing an error.

The types of errors seen were:

- bad return value: {:error, "" is not repeatable""}
- (CondClauseError) no cond clause evaluated to a truthy value (coap_ex
  0.1.0) lib/coap/message_options.ex
- (CaseClauseError) no case clause matching <<...>> (coap_ex 0.1.0)
  lib/coap/message_options.ex:54
- (FunctionClauseError) no function clause matching in
  CoAP.Block.decode/1
- (MatchError) no match of right hand side value: """"

One could argue that these "errors" should be fixed by updating the
MessageOptions module to gracefully handle such conditions, and that is
definitely worth considering. By doing that, we would then be able to
complete the decoding, which would allow extracting the message token
which is required to identify the associated connection process.

config/config.exs

@@ -1,3 +1,3 @@
-use Mix.Config
+import Mix.Config
 
 import_config "#{Mix.env()}.exs"

config/test.exs

@@ -1,3 +1,3 @@
-use Mix.Config
+import Mix.Config
 
 config :logger, level: :info

lib/coap/block.ex

@@ -58,6 +58,10 @@ defmodule CoAP.Block do
   def decode(<<number::size(28), more::size(1), size_exponent::size(3)>>),
     do: decode(number, more, size_exponent)
 
+  def decode(number) do
+    decode(number, 0, 0)
+  end
+
   @spec decode(integer, 0 | 1, integer) :: t()
   def decode(number, more, size_exponent) do
     %__MODULE__{

lib/coap/socket_server.ex

@@ -91,23 +91,38 @@ defmodule CoAP.SocketServer do
   def handle_info({:udp, _socket, peer_ip, peer_port, data}, state) do
     debug("CoAP socket received raw data #{to_hex(data)} from #{inspect({peer_ip, peer_port})}")
 
-    message = Message.decode(data)
+    case decode_message(data) do
+      {:ok, message} ->
+        connection_id = {peer_ip, peer_port, message.token}
 
-    {connection, new_state} =
-      connection_for(message.request, {peer_ip, peer_port, message.token}, state)
+        {connection, new_state} = connection_for(message.request, connection_id, state)
+
+        if connection do
+          send(connection, {:receive, message})
+
+          {:noreply, new_state}
+        else
+          # If we can't find a connection, we can't deliver the message to the client
+
+          warn(
+            "CoAP socket received message for lost connection from " <>
+              "ip: #{inspect(peer_ip)}, port: #{inspect(peer_port)}.  Message: #{inspect(message)}"
+          )
+
+          {:stop, :normal, state}
+        end
+
+      {:error, reason} ->
+        # If we can't decode the message, we can't construct the connection_id which is necessary to lookup
+        # the connection process, which is required to return an error message to the client.
 
-    case connection do
-      nil ->
         warn(
-          "CoAP socket received message for lost connection from " <>
-            "ip: #{inspect(peer_ip)}, port: #{inspect(peer_port)}.  Message: #{inspect(message)}"
+          "CoAP socket failed to decode udp packets because #{inspect(reason)} from " <>
+            "ip: #{inspect(peer_ip)}, port: #{inspect(peer_port)}.  data: #{inspect(data, limit: :infinity, print_limit: :infinity)}"
         )
 
-      c ->
-        send(c, {:receive, message})
+        {:stop, :normal, state}
     end
-
-    {:noreply, new_state}
   end
 
   # Deliver messages to be sent to a peer
@@ -148,9 +163,7 @@ defmodule CoAP.SocketServer do
     connection = Map.get(state[:connections], connection_id)
 
     debug(
-      "CoAP socket SERVER received DOWN:#{reason} in CoAP.SocketServer from:#{
-        inspect(connection_id)
-      }:#{inspect(connection)}:#{inspect(ref)}"
+      "CoAP socket SERVER received DOWN:#{reason} in CoAP.SocketServer from:#{inspect(connection_id)}:#{inspect(connection)}:#{inspect(ref)}"
     )
 
     {:noreply,
@@ -166,9 +179,7 @@ defmodule CoAP.SocketServer do
     connection = Map.get(state[:connections], connection_id)
 
     debug(
-      "CoAP socket CLIENT received DOWN:#{reason} in CoAP.SocketServer from: #{
-        inspect(connection_id)
-      }:#{inspect(connection)}:#{inspect(ref)}"
+      "CoAP socket CLIENT received DOWN:#{reason} in CoAP.SocketServer from: #{inspect(connection_id)}:#{inspect(connection)}:#{inspect(ref)}"
     )
 
     {:stop, :normal, state}
@@ -239,4 +250,14 @@ defmodule CoAP.SocketServer do
 
   defp type(%{port: 0}), do: :client
   defp type(_), do: :server
+
+  defp decode_message(data) do
+    {:ok, Message.decode(data)}
+  rescue
+    e ->
+      {:error, e}
+  catch
+    e ->
+      {:error, e}
+  end
 end

test/coap/socket_server_test.exs

@@ -0,0 +1,79 @@
+defmodule CoAP.SocketServerTest do
+  use ExUnit.Case
+
+  import ExUnit.CaptureLog
+
+  alias CoAP.SocketServer
+  alias CoAP.Adapters.GenericServer
+
+  defmodule FakeEndpoint do
+  end
+
+  describe "CoAP.SocketServer decoding errors" do
+    test "non repeatable options" do
+      peer_ip = "127.0.0.1"
+      peer_port = 5830
+
+      {:ok, server} = SocketServer.start_link([{GenericServer, FakeEndpoint}, peer_port])
+
+      data =
+        <<100, 69, 0, 1, 252, 1, 46, 56, 68, 80, 231, 168, 249, 100, 69, 0, 1, 252, 1, 46, 56, 68,
+          80, 231, 168, 249>>
+
+      log =
+        capture_log(fn ->
+          ref = Process.monitor(server)
+          send(server, {:udp, :socket, peer_ip, peer_port, data})
+
+          response =
+            receive do
+              {:DOWN, ^ref, :process, ^server, _reason} ->
+                :ok
+            after
+              100 -> {:error, {:timeout, :await_response}}
+            end
+
+          assert response == :ok
+        end)
+
+      assert log =~ "CoAP socket failed to decode udp packets"
+      assert log =~ "is not repeatable"
+      assert log =~ "from ip: \"#{peer_ip}\""
+      assert log =~ "port: #{peer_port}"
+      assert log =~ "data: #{inspect(data, binaries: :as_binary)}"
+    end
+
+    test "CaseClauseError" do
+      peer_ip = "127.0.0.1"
+      peer_port = 5830
+
+      {:ok, server} = SocketServer.start_link([{GenericServer, FakeEndpoint}, peer_port])
+
+      data =
+        <<100, 69, 0, 1, 83, 61, 239, 152, 68, 244, 81, 253, 46, 100, 69, 0, 1, 83, 61, 239, 152,
+          68, 244, 81, 253, 46>>
+
+      log =
+        capture_log(fn ->
+          ref = Process.monitor(server)
+          send(server, {:udp, :socket, peer_ip, peer_port, data})
+
+          response =
+            receive do
+              {:DOWN, ^ref, :process, ^server, _reason} ->
+                :ok
+            after
+              100 -> {:error, {:timeout, :await_response}}
+            end
+
+          assert response == :ok
+        end)
+
+      assert log =~ "CoAP socket failed to decode udp packets"
+      assert log =~ "CaseClauseError"
+      assert log =~ "from ip: \"#{peer_ip}\""
+      assert log =~ "port: #{peer_port}"
+      assert log =~ "data: #{inspect(data, binaries: :as_binary)}"
+    end
+  end
+end