From 911a9eab1f3a0cb41a4a80090f32305c8f526d08 Mon Sep 17 00:00:00 2001 From: Jordan Bracco Date: Thu, 14 May 2020 18:42:33 +0200 Subject: [PATCH] Improve behaviour with too long commands/filenames. --- lib/gen_magic/server.ex | 3 +- src/apprentice.c | 65 ++++++++++++++++++++++-------- test/gen_magic/apprentice_test.exs | 51 +++++++++++++++++++---- 3 files changed, 93 insertions(+), 26 deletions(-) diff --git a/lib/gen_magic/server.ex b/lib/gen_magic/server.ex index 09cdd41..c0f07d7 100644 --- a/lib/gen_magic/server.ex +++ b/lib/gen_magic/server.ex @@ -283,7 +283,7 @@ defmodule GenMagic.Server do 20 => :enotdir, 12 => :enomem, 24 => :emfile, - 36 => :enametoolong, + 36 => :enametoolong } @errno Map.keys(@errnos) @@ -292,6 +292,7 @@ defmodule GenMagic.Server do {:ok, {mime_type, encoding, content}} -> {:ok, Result.build(mime_type, encoding, content)} {:error, {errno, _}} when errno in @errno -> {:error, @errnos[errno]} {:error, {errno, string}} -> {:error, "#{errno}: #{string}"} + {:error, _} = error -> error end end diff --git a/src/apprentice.c b/src/apprentice.c index c558001..1afd53c 100644 --- a/src/apprentice.c +++ b/src/apprentice.c @@ -60,6 +60,11 @@ #define ERROR_BAD_TERM 4 #define ERROR_EI 5 +// We use a bigger than possible valid command length (around 4111 bytes) to +// allow more precise errors when using too long paths. +#define COMMAND_LEN 8000 +#define COMMAND_BUFFER_SIZE COMMAND_LEN + 1 + #define MAGIC_FLAGS_COMMON (MAGIC_CHECK | MAGIC_ERROR) magic_t magic_setup(int flags); @@ -70,14 +75,14 @@ void setup_options(int argc, char **argv); void setup_options_file(char *optarg); void setup_options_default(); void setup_system(); -int process_command(byte *buf); -void process_line(char *line); +int process_command(uint16_t len, byte *buf); void process_file(char *path, ei_x_buff *result); void process_bytes(char *bytes, int size, ei_x_buff *result); size_t read_cmd(byte *buf); size_t write_cmd(byte *buf, size_t len); void error(ei_x_buff *result, const char *error); void handle_magic_error(magic_t handle, int errn, ei_x_buff *result); +void fdseek(uint16_t count); struct magic_file { struct magic_file *prev; @@ -103,29 +108,35 @@ int main(int argc, char **argv) { if (ei_x_free(&ok_buf) != 0) exit(ERROR_EI); - byte buf[4112]; - while (read_cmd(buf) > 0) { - process_command(buf); + byte buf[COMMAND_BUFFER_SIZE]; + uint16_t len; + while ((len = read_cmd(buf)) > 0) { + process_command(len, buf); } return 255; } -int process_command(byte *buf) { +int process_command(uint16_t len, byte *buf) { ei_x_buff result; char atom[128]; int index, version, arity, termtype, termsize; index = 0; - if (ei_decode_version(buf, &index, &version) != 0) { - exit(ERROR_BAD_TERM); - } - // Initialize result if (ei_x_new_with_version(&result) || ei_x_encode_tuple_header(&result, 2)) { exit(ERROR_EI); } + if (len >= COMMAND_LEN) { + error(&result, "badarg"); + return 1; + } + + if (ei_decode_version(buf, &index, &version) != 0) { + exit(ERROR_BAD_TERM); + } + if (ei_decode_tuple_header(buf, &index, &arity) != 0) { error(&result, "badarg"); return 1; @@ -145,11 +156,16 @@ int process_command(byte *buf) { char path[4097]; ei_get_type(buf, &index, &termtype, &termsize); - if (termtype == ERL_BINARY_EXT && termsize < 4096) { - long bin_length; - ei_decode_binary(buf, &index, path, &bin_length); - path[termsize] = '\0'; - process_file(path, &result); + if (termtype == ERL_BINARY_EXT) { + if (termsize < 4096) { + long bin_length; + ei_decode_binary(buf, &index, path, &bin_length); + path[termsize] = '\0'; + process_file(path, &result); + } else { + error(&result, "enametoolong"); + return 1; + } } else { error(&result, "badarg"); return 1; @@ -176,6 +192,7 @@ int process_command(byte *buf) { return 1; } + // Empty the buffer. write_cmd(result.buff, result.index); if (ei_x_free(&result) != 0) { @@ -386,9 +403,15 @@ size_t read_cmd(byte *buf) { } uint16_t len16 = *(uint16_t *)buf; len16 = ntohs(len16); - if (len16 > 4111) { - exit(ERROR_BAD_TERM); + + // Buffer isn't large enough: just return possible len, without reading. + // Up to the caller of verifying the size again and return an error. + // buf left unchanged. + if (len16 > COMMAND_LEN) { + fdseek(len16); + return len16; } + return read_exact(buf, len16); } @@ -412,3 +435,11 @@ void error(ei_x_buff *result, const char *error) { if (ei_x_free(result) != 0) exit(ERROR_EI); } + +void fdseek(uint16_t count) { + int i = 0; + while (i < count) { + getchar(); + i += 1; + } +} diff --git a/test/gen_magic/apprentice_test.exs b/test/gen_magic/apprentice_test.exs index 06c9983..7c6b4b5 100644 --- a/test/gen_magic/apprentice_test.exs +++ b/test/gen_magic/apprentice_test.exs @@ -1,6 +1,8 @@ defmodule GenMagic.ApprenticeTest do use GenMagic.MagicCase + @tmp_path "/tmp/testgenmagicx" + test "sends ready" do port = Port.open(GenMagic.Config.get_port_name(), GenMagic.Config.get_port_options([])) assert_ready(port) @@ -85,19 +87,39 @@ defmodule GenMagic.ApprenticeTest do end test "works with big file path", %{port: port} do - file = too_big() <> "/a" - File.mkdir_p!(too_big()) - File.touch!(file) - on_exit(fn -> File.rm_rf!("/tmp/testmagicex/") end) - send(port, {self(), {:command, :erlang.term_to_binary({:file, file})}}) + # Test with longest valid path. + {dir, bigfile} = too_big(@tmp_path, "/a") + File.mkdir_p!(dir) + File.touch!(bigfile) + on_exit(fn -> File.rm_rf!(@tmp_path) end) + send(port, {self(), {:command, :erlang.term_to_binary({:file, bigfile})}}) assert_receive {^port, {:data, data}} assert {:ok, _} = :erlang.binary_to_term(data) refute_receive _ - file = too_big() <> "/aaaaaaaaaa" + + # This path should be long enough for buffers, but larger than a valid path name. Magic will return an errno 36. + file = @tmp_path <> String.duplicate("a", 256) + send(port, {self(), {:command, :erlang.term_to_binary({:file, file})}}) + assert_receive {^port, {:data, data}} + assert {:error, {36, _}} = :erlang.binary_to_term(data) + refute_receive _ + # Theses filename should be too big for the path buffer. + file = bigfile <> "aaaaaaaaaa" + send(port, {self(), {:command, :erlang.term_to_binary({:file, file})}}) + assert_receive {^port, {:data, data}} + assert {:error, :enametoolong} = :erlang.binary_to_term(data) + refute_receive _ + # This call should be larger than the COMMAND_BUFFER_SIZE. Ensure nothing bad happens! + file = String.duplicate(bigfile, 4) send(port, {self(), {:command, :erlang.term_to_binary({:file, file})}}) assert_receive {^port, {:data, data}} assert {:error, :badarg} = :erlang.binary_to_term(data) refute_receive _ + # We re-run a valid call to ensure the buffer/... haven't been corrupted in port land. + send(port, {self(), {:command, :erlang.term_to_binary({:file, bigfile})}}) + assert_receive {^port, {:data, data}} + assert {:ok, _} = :erlang.binary_to_term(data) + refute_receive _ end end @@ -106,7 +128,20 @@ defmodule GenMagic.ApprenticeTest do assert :ready == :erlang.binary_to_term(data) end - def too_big do - "/tmp/testmagicex/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + def too_big(path, filename, limit \\ 4095) do + last_len = byte_size(filename) + path_len = byte_size(path) + needed = limit - (last_len + path_len) + extra = make_too_big(needed, "") + {path <> extra, path <> extra <> filename} + end + + def make_too_big(needed, acc) when needed <= 255 do + acc <> "/" <> String.duplicate("a", needed - 1) + end + + def make_too_big(needed, acc) do + acc = acc <> "/" <> String.duplicate("a", 254) + make_too_big(needed - 255, acc) end end