diff --git a/.credo.exs b/.credo.exs index 7bc729a..e6487f1 100644 --- a/.credo.exs +++ b/.credo.exs @@ -124,7 +124,7 @@ {Credo.Check.Refactor.MatchInCondition, []}, {Credo.Check.Refactor.NegatedConditionsInUnless, []}, {Credo.Check.Refactor.NegatedConditionsWithElse, []}, - {Credo.Check.Refactor.Nesting, []}, + {Credo.Check.Refactor.Nesting, [max_nesting: 3]}, {Credo.Check.Refactor.UnlessWithElse, []}, {Credo.Check.Refactor.WithClauses, []}, diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e14d083..2942724 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,15 +8,15 @@ on: env: MIX_ENV: test - + jobs: build: runs-on: ubuntu-latest name: OTP ${{matrix.otp}} / Elixir ${{matrix.elixir}} strategy: matrix: - otp: ['23.3', '24.0'] - elixir: ['1.12.1', '1.11.4'] + otp: ['28.5', '29.0'] + elixir: ['1.18.4', '1.19.0'] steps: - uses: actions/checkout@v2 - uses: erlef/setup-beam@v1 @@ -30,4 +30,4 @@ jobs: - name: Run tests run: mix test - name: Run dialyzer - run: mix dialyzer \ No newline at end of file + run: mix dialyzer diff --git a/lib/decode_error.ex b/lib/decode_error.ex new file mode 100644 index 0000000..effb3fd --- /dev/null +++ b/lib/decode_error.ex @@ -0,0 +1,4 @@ +defmodule Exgencode.DecodeError do + @moduledoc false + defexception [:message] +end diff --git a/lib/exgencode.ex b/lib/exgencode.ex index 86900dd..898e3d2 100644 --- a/lib/exgencode.ex +++ b/lib/exgencode.ex @@ -346,6 +346,12 @@ defmodule Exgencode do {field_name, props[:decode]} end) + offset_targets = + for {field_name, props} <- field_list, props[:offset_to] != nil, into: %{} do + {props[:offset_to], field_name} + end + |> Macro.escape() + struct_fields = for {field_name, props} <- field_list, props[:type] not in [:constant, :skip] do {field_name, props[:default]} @@ -376,15 +382,24 @@ defmodule Exgencode do end def decode(pdu, binary, version) do - do_decode(pdu, binary, unquote(fields_for_decodes), version) + do_decode(pdu, binary, unquote(fields_for_decodes), version, bit_size(binary)) end - defp do_decode(pdu, binary, [{field, decode_fun} | rest], version) do + defp do_decode(pdu, binary, [{field, decode_fun} | rest], version, pdu_bit_size) do + binary = + Exgencode.EncodeDecode.skip_to_offset( + pdu, + field, + binary, + pdu_bit_size, + unquote(offset_targets) + ) + {new_pdu, rest_binary} = decode_fun.(version).(pdu, binary) - do_decode(new_pdu, rest_binary, rest, version) + do_decode(new_pdu, rest_binary, rest, version, pdu_bit_size) end - defp do_decode(pdu, rest_bin, [], _) do + defp do_decode(pdu, rest_bin, [], _, _) do {pdu, rest_bin} end end diff --git a/lib/exgencode/encode_decode.ex b/lib/exgencode/encode_decode.ex index 6b898d7..ab07d71 100644 --- a/lib/exgencode/encode_decode.ex +++ b/lib/exgencode/encode_decode.ex @@ -303,6 +303,45 @@ defmodule Exgencode.EncodeDecode do wrap_conditional_decode(props, basic_fun) end + def skip_to_offset(pdu, field, rest_binary, pdu_bit_size, offset_targets) do + case offset_targets do + %{^field => offset_field} -> + do_skip_to_offset(pdu, field, offset_field, rest_binary, pdu_bit_size) + + _ -> + rest_binary + end + end + + defp do_skip_to_offset(pdu, field, offset_field, rest_binary, pdu_bit_size) do + case Map.get(pdu, offset_field) do + offset when offset in [0, nil] -> + rest_binary + + offset -> + cursor = pdu_bit_size - bit_size(rest_binary) + target = offset * 8 + + cond do + target == cursor -> + rest_binary + + target < cursor -> + raise Exgencode.DecodeError, + "offset field #{inspect(offset_field)} of #{inspect(field)} cannot point backwards!" + + target > pdu_bit_size -> + raise Exgencode.DecodeError, + "offset_field #{inspect(offset_field)} cannot point outside binary!" + + true -> + gap_bits = target - cursor + <<_padding::size(gap_bits), rest::bitstring>> = rest_binary + rest + end + end + end + def wrap_custom_encode(field_name, encode_fun) do quote do fn pdu -> diff --git a/lib/exgencode/validator.ex b/lib/exgencode/validator.ex index 7220d4c..739dddb 100644 --- a/lib/exgencode/validator.ex +++ b/lib/exgencode/validator.ex @@ -126,6 +126,12 @@ defmodule Exgencode.Validator do end def validate_pdu(pdu_name, fields) do + validate_pdu_size(pdu_name, fields) + validate_offset_ordering(pdu_name, fields) + validate_no_duplicate_offsets(pdu_name, fields) + end + + defp validate_pdu_size(pdu_name, fields) do total_size = fields |> Enum.reject(fn {_field_name, props} -> props[:type] == :variable end) @@ -143,6 +149,40 @@ defmodule Exgencode.Validator do ) end + defp validate_offset_ordering(pdu_name, fields) do + Enum.reduce(fields, [], fn {field_name, props}, seen_names -> + target = props[:offset_to] + + if not is_nil(target) and target in seen_names do + raise ArgumentError, + "#{inspect(Macro.to_string(pdu_name))} #{inspect(target)}: backward offsets are unsupported!" + end + + [field_name | seen_names] + end) + end + + defp validate_no_duplicate_offsets(pdu_name, fields) do + Enum.reduce(fields, [], fn {field_name, props}, seen_targets -> + target = props[:offset_to] + + cond do + is_nil(target) -> + seen_targets + + target in seen_targets -> + raise_argument_error( + pdu_name, + field_name, + "multiple offset fields pointing to target #{inspect(target)} is unsupported!" + ) + + true -> + [target | seen_targets] + end + end) + end + defp raise_argument_error(pdu_name, field_name, msg) do raise ArgumentError, "Badly defined field #{inspect(field_name)} in #{inspect(pdu_name |> Macro.to_string())} - " <> diff --git a/mix.exs b/mix.exs index d6686ab..c321139 100644 --- a/mix.exs +++ b/mix.exs @@ -4,8 +4,8 @@ defmodule Exgencode.Mixfile do def project do [ app: :exgencode, - version: "2.5.2", - elixir: "~> 1.7", + version: "2.5.3", + elixir: "~> 1.18", start_permanent: Mix.env() == :prod, deps: deps(), package: package(), @@ -26,8 +26,8 @@ defmodule Exgencode.Mixfile do defp deps do [ {:ex_doc, "~> 0.20", only: :dev, runtime: false}, - {:credo, "~> 1.0", only: [:dev, :test]}, - {:dialyxir, "~> 1.1.0", only: [:dev, :test], runtime: false} + {:credo, "~> 1.7.13", only: [:dev, :test]}, + {:dialyxir, "~> 1.4.0", only: [:dev, :test], runtime: false} ] end diff --git a/mix.lock b/mix.lock index 3dd1917..946393c 100644 --- a/mix.lock +++ b/mix.lock @@ -1,12 +1,12 @@ %{ - "bunt": {:hex, :bunt, "0.2.0", "951c6e801e8b1d2cbe58ebbd3e616a869061ddadcc4863d0a2182541acae9a38", [:mix], [], "hexpm", "7af5c7e09fe1d40f76c8e4f9dd2be7cebd83909f31fee7cd0e9eadc567da8353"}, - "credo": {:hex, :credo, "1.5.6", "e04cc0fdc236fefbb578e0c04bd01a471081616e741d386909e527ac146016c6", [:mix], [{:bunt, "~> 0.2.0", [hex: :bunt, repo: "hexpm", optional: false]}, {:file_system, "~> 0.2.8", [hex: :file_system, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm", "4b52a3e558bd64e30de62a648518a5ea2b6e3e5d2b164ef5296244753fc7eb17"}, - "dialyxir": {:hex, :dialyxir, "1.1.0", "c5aab0d6e71e5522e77beff7ba9e08f8e02bad90dfbeffae60eaf0cb47e29488", [:mix], [{:erlex, ">= 0.2.6", [hex: :erlex, repo: "hexpm", optional: false]}], "hexpm", "07ea8e49c45f15264ebe6d5b93799d4dd56a44036cf42d0ad9c960bc266c0b9a"}, + "bunt": {:hex, :bunt, "1.0.0", "081c2c665f086849e6d57900292b3a161727ab40431219529f13c4ddcf3e7a44", [:mix], [], "hexpm", "dc5f86aa08a5f6fa6b8096f0735c4e76d54ae5c9fa2c143e5a1fc7c1cd9bb6b5"}, + "credo": {:hex, :credo, "1.7.19", "cc52129665fc7c15143d47838fda0f9cd6dac9ceced7bf4da6f85fcbfe64b12a", [:mix], [{:bunt, "~> 0.2.1 or ~> 1.0", [hex: :bunt, repo: "hexpm", optional: false]}, {:file_system, "~> 0.2 or ~> 1.0", [hex: :file_system, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm", "2d8bc95d5a7bb99dd2613621d4f08c6a3575c3fd4b62e6a2b48a100352a557b8"}, + "dialyxir": {:hex, :dialyxir, "1.4.7", "dda948fcee52962e4b6c5b4b16b2d8fa7d50d8645bbae8b8685c3f9ecb7f5f4d", [:mix], [{:erlex, ">= 0.2.8", [hex: :erlex, repo: "hexpm", optional: false]}], "hexpm", "b34527202e6eb8cee198efec110996c25c5898f43a4094df157f8d28f27d9efe"}, "earmark_parser": {:hex, :earmark_parser, "1.4.13", "0c98163e7d04a15feb62000e1a891489feb29f3d10cb57d4f845c405852bbef8", [:mix], [], "hexpm", "d602c26af3a0af43d2f2645613f65841657ad6efc9f0e361c3b6c06b578214ba"}, - "erlex": {:hex, :erlex, "0.2.6", "c7987d15e899c7a2f34f5420d2a2ea0d659682c06ac607572df55a43753aa12e", [:mix], [], "hexpm", "2ed2e25711feb44d52b17d2780eabf998452f6efda104877a3881c2f8c0c0c75"}, + "erlex": {:hex, :erlex, "0.2.9", "7debbbaa9f4f368b8cd648983e0f1d7963028508e9c59e9d4ed504e94ef52a55", [:mix], [], "hexpm", "8cfffc0ec7159e6d73de2ab28a588064de80f88b2798d5cbe4482cbbc200178b"}, "ex_doc": {:hex, :ex_doc, "0.24.2", "e4c26603830c1a2286dae45f4412a4d1980e1e89dc779fcd0181ed1d5a05c8d9", [:mix], [{:earmark_parser, "~> 1.4.0", [hex: :earmark_parser, repo: "hexpm", optional: false]}, {:makeup_elixir, "~> 0.14", [hex: :makeup_elixir, repo: "hexpm", optional: false]}, {:makeup_erlang, "~> 0.1", [hex: :makeup_erlang, repo: "hexpm", optional: false]}], "hexpm", "e134e1d9e821b8d9e4244687fb2ace58d479b67b282de5158333b0d57c6fb7da"}, - "file_system": {:hex, :file_system, "0.2.10", "fb082005a9cd1711c05b5248710f8826b02d7d1784e7c3451f9c1231d4fc162d", [:mix], [], "hexpm", "41195edbfb562a593726eda3b3e8b103a309b733ad25f3d642ba49696bf715dc"}, - "jason": {:hex, :jason, "1.2.2", "ba43e3f2709fd1aa1dce90aaabfd039d000469c05c56f0b8e31978e03fa39052", [:mix], [{:decimal, "~> 1.0 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: true]}], "hexpm", "18a228f5f0058ee183f29f9eae0805c6e59d61c3b006760668d8d18ff0d12179"}, + "file_system": {:hex, :file_system, "1.1.1", "31864f4685b0148f25bd3fbef2b1228457c0c89024ad67f7a81a3ffbc0bbad3a", [:mix], [], "hexpm", "7a15ff97dfe526aeefb090a7a9d3d03aa907e100e262a0f8f7746b78f8f87a5d"}, + "jason": {:hex, :jason, "1.4.5", "2e3a008590b0b8d7388c20293e9dcc9cf3e5d642fd2a114e4cbbb52e595d940a", [:mix], [{:decimal, "~> 1.0 or ~> 2.0 or ~> 3.0", [hex: :decimal, repo: "hexpm", optional: true]}], "hexpm", "b0c823996102bcd0239b3c2444eb00409b72f6a140c1950bc8b457d836b30684"}, "makeup": {:hex, :makeup, "1.0.5", "d5a830bc42c9800ce07dd97fa94669dfb93d3bf5fcf6ea7a0c67b2e0e4a7f26c", [:mix], [{:nimble_parsec, "~> 0.5 or ~> 1.0", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "cfa158c02d3f5c0c665d0af11512fed3fba0144cf1aadee0f2ce17747fba2ca9"}, "makeup_elixir": {:hex, :makeup_elixir, "0.15.1", "b5888c880d17d1cc3e598f05cdb5b5a91b7b17ac4eaf5f297cb697663a1094dd", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}, {:nimble_parsec, "~> 1.1", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "db68c173234b07ab2a07f645a5acdc117b9f99d69ebf521821d89690ae6c6ec8"}, "makeup_erlang": {:hex, :makeup_erlang, "0.1.1", "3fcb7f09eb9d98dc4d208f49cc955a34218fc41ff6b84df7c75b3e6e533cc65f", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}], "hexpm", "174d0809e98a4ef0b3309256cbf97101c6ec01c4ab0b23e926a9e17df2077cbb"}, diff --git a/test/exgencode_test.exs b/test/exgencode_test.exs index 634764f..1562a6d 100644 --- a/test/exgencode_test.exs +++ b/test/exgencode_test.exs @@ -87,11 +87,15 @@ defmodule ExgencodeTest do nested_pdu = %TestPdu.NestedVersionedMsg{nested: %TestPdu.VersionedMsg{}} binary = <<2::size(16), 10::size(16)>> - assert {^nested_pdu, <<>>} = Exgencode.Pdu.decode(%TestPdu.NestedVersionedMsg{}, binary, "1.0.0") + + assert {^nested_pdu, <<>>} = + Exgencode.Pdu.decode(%TestPdu.NestedVersionedMsg{}, binary, "1.0.0") nested_pdu = %TestPdu.NestedVersionedMsg{nested: %TestPdu.VersionedMsg{newerField: 111}} binary = <<2::size(16), 10::size(16), 111::size(8)>> - assert {^nested_pdu, <<>>} = Exgencode.Pdu.decode(%TestPdu.NestedVersionedMsg{}, binary, "2.0.0") + + assert {^nested_pdu, <<>>} = + Exgencode.Pdu.decode(%TestPdu.NestedVersionedMsg{}, binary, "2.0.0") end test "versioned encode/decode symmetry" do @@ -569,14 +573,14 @@ defmodule ExgencodeTest do test "custom size function" do pdu = %TestPdu.CustomSizeFunPdu{custom: {5, 1024}} - assert <<2, 8, 5, 0, 0, 0, 4, 0, 8, 4>> = Exgencode.Pdu.encode(pdu) + assert <<2, 9, 5, 0, 0, 0, 4, 0, 8, 4>> = Exgencode.Pdu.encode(pdu) offsets = Exgencode.Pdu.set_offsets(pdu) assert {offsets, <<>>} == Exgencode.Pdu.decode( %TestPdu.CustomSizeFunPdu{}, - <<2, 8, 5, 0, 0, 0, 4, 0, 8, 4>>, + <<2, 9, 5, 0, 0, 0, 4, 0, 8, 4>>, nil ) end @@ -585,4 +589,104 @@ defmodule ExgencodeTest do pdu = %TestPdu.OffsetMadnessPdu{} assert <<3, 4, 0, 11, 5, 0>> = Exgencode.Pdu.encode(pdu) end + + test "decode reaches offset_to : skips gap" do + binary = <<7, 0, 3, "abc", 0, 0xDE, 0xAD, 0xBE, 0xEF>> + + assert {%TestPdu.OffsetMsg{ + offset_to_footer: 7, + name_length: 3, + name: "abc", + footer: 0xDEADBEEF + }, <<>>} == Exgencode.Pdu.decode(%TestPdu.OffsetMsg{}, binary) + end + + test "decode with zero-length gap (offset == cursor) is unchanged" do + binary = <<6, 0, 3, "abc", 0xDE, 0xAD, 0xBE, 0xEF>> + + assert {%TestPdu.OffsetMsg{ + offset_to_footer: 6, + name_length: 3, + name: "abc", + footer: 0xDEADBEEF + }, <<>>} == Exgencode.Pdu.decode(%TestPdu.OffsetMsg{}, binary) + end + + test "decode with offset 0 leaves the target field absent" do + binary = <<0, 0, 3, "abc">> + + assert {%TestPdu.OffsetMsg{ + offset_to_footer: 0, + name_length: 3, + name: "abc", + footer: nil + }, <<>>} == Exgencode.Pdu.decode(%TestPdu.OffsetMsg{}, binary) + end + + test "multiple offsets with offset_to gap" do + binary = <<4, 8, 0, 2, "XY", 0, 0, 0xBE, 0xEF>> + + assert {%TestPdu.MultiOffsetMsg{ + offset_to_a: 4, + offset_to_b: 8, + len_a: 2, + field_a: "XY", + field_b: 0xBEEF + }, <<>>} == Exgencode.Pdu.decode(%TestPdu.MultiOffsetMsg{}, binary) + end + + test "decode raises on a backwards offset" do + binary = <<4, 0, 3, "abc", 0xDE, 0xAD, 0xBE, 0xEF>> + + assert_raise Exgencode.DecodeError, ~r/point backwards/, fn -> + Exgencode.Pdu.decode(%TestPdu.OffsetMsg{}, binary) + end + end + + test "decode raises on an offset past the end of the binary" do + binary = <<20, 0, 3, "abc", 0, 0xDE, 0xAD, 0xBE, 0xEF>> + + assert_raise Exgencode.DecodeError, ~r/cannot point outside binary/, fn -> + Exgencode.Pdu.decode(%TestPdu.OffsetMsg{}, binary) + end + end + + test "offsets in a nested subrecord stay relative to that subrecord" do + binary = <<9, 7, 0, 3, "abc", 0, 0xDE, 0xAD, 0xBE, 0xEF>> + + assert {%TestPdu.NestedOffsetMsg{ + header: 9, + sub: %TestPdu.OffsetMsg{ + offset_to_footer: 7, + name_length: 3, + name: "abc", + footer: 0xDEADBEEF + } + }, <<>>} == Exgencode.Pdu.decode(%TestPdu.NestedOffsetMsg{}, binary) + end + + test "defining two offset fields to the same target is rejected at compile time" do + assert_raise ArgumentError, ~r/multiple offset fields pointing/, fn -> + defmodule BadDup do + import Exgencode + + defpdu DuplicateOffsetPdu, + off_a: [size: 8, offset_to: :target], + off_b: [size: 8, offset_to: :target], + target: [size: 8, conditional: :off_a] + end + end + end + + test "defining an offset field after its target is rejected at compile time" do + assert_raise ArgumentError, ~r/backward offsets are unsupported/, fn -> + defmodule BadOrder do + import Exgencode + + defpdu BackwardsOffsetPdu, + target: [size: 8], + off: [size: 8, offset_to: :target] + end + end + end end diff --git a/test/helpers/test_pdu.ex b/test/helpers/test_pdu.ex index 6463827..c3f91fc 100644 --- a/test/helpers/test_pdu.ex +++ b/test/helpers/test_pdu.ex @@ -157,9 +157,9 @@ defmodule Exgencode.TestPdu do <> = val {struct(pdu, %{custom: {size, vals}}), rest} end, - size: fn %CustomSizeFunPdu{custom: {size, _vals}} -> size * 8 end + size: fn %CustomSizeFunPdu{custom: {size, _vals}} -> (size + 1) * 8 end ], - anotherWhyNot: [offset_to: :oneMore, size: 8], + anotherWhyNot: [size: 8, default: 8], oneMore: [default: 4, size: 8] defpdu OffsetMadnessPdu, @@ -168,4 +168,21 @@ defmodule Exgencode.TestPdu do somethingIrrelevant: [default: 11, size: 16], anotherOffset: [offset_to: :somethingElse, size: 8], somethingElse: [default: 0, size: 8] + + defpdu OffsetMsg, + offset_to_footer: [size: 8, offset_to: :footer, default: 0x00], + name_length: [size: 16, default: 0], + name: [type: :variable, size: :name_length, conditional: :name_length], + footer: [size: 32, conditional: :offset_to_footer] + + defpdu MultiOffsetMsg, + offset_to_a: [size: 8, offset_to: :field_a, default: 0x00], + offset_to_b: [size: 8, offset_to: :field_b, default: 0x00], + len_a: [size: 16, default: 0], + field_a: [type: :variable, size: :len_a, conditional: :len_a], + field_b: [size: 16, conditional: :offset_to_b] + + defpdu NestedOffsetMsg, + header: [size: 8, default: 0], + sub: [type: :subrecord, default: %OffsetMsg{}] end