Julien-cpsn / ATAC

A simple API client (postman like) in your terminal
https://atac.julien-cpsn.com/
MIT License
1.86k stars 81 forks source link

[bug] unexpected panic when specifying `--directory` of a tmpfs #11

Closed brandon1024 closed 5 months ago

brandon1024 commented 5 months ago

Issue Description

When specifying a tmpfs location as an argument to --directory, the application panics unexpectedly.

$ atac -d /tmp
parsing: /tmp/mat-debug-91998.log
thread 'main' panicked at src/app/startup/startup.rs:55:17:
unhandled file type
stack backtrace:
   0:     0x55b7dbc36f86 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h410d4c66be4e37f9
   1:     0x55b7dbc64e40 - core::fmt::write::he40921d4802ce2ac
   2:     0x55b7dbc3355f - std::io::Write::write_fmt::h5de5a4e7037c9b20
   3:     0x55b7dbc36d64 - std::sys_common::backtrace::print::h11c067a88e3bdb22
   4:     0x55b7dbc38797 - std::panicking::default_hook::{{closure}}::h8c832ecb03fde8ea
   5:     0x55b7dbc384f9 - std::panicking::default_hook::h1633e272b4150cf3
   6:     0x55b7dbc38c28 - std::panicking::rust_panic_with_hook::hb164d19c0c1e71d4
   7:     0x55b7dbc38ac9 - std::panicking::begin_panic_handler::{{closure}}::h0369088c533c20e9
   8:     0x55b7dbc37486 - std::sys_common::backtrace::__rust_end_short_backtrace::hc11d910daf35ac2e
   9:     0x55b7dbc38854 - rust_begin_unwind
  10:     0x55b7db6a8d85 - core::panicking::panic_fmt::ha6effc2775a0749c
  11:     0x55b7db6d177c - atac::app::startup::startup::<impl atac::app::app::App>::startup::hb0dbb4ca5eb46b2f
  12:     0x55b7db7742b1 - tokio::runtime::park::CachedParkThread::block_on::he0f8c1c457bbf1b1
  13:     0x55b7db75039a - tokio::runtime::context::runtime::enter_runtime::hf17d553f5e0f270a
  14:     0x55b7db78297e - tokio::runtime::runtime::Runtime::block_on::h54ffd2db751dfad6
  15:     0x55b7db7570d9 - atac::main::h26d3a8c116b6fb46
  16:     0x55b7db73e903 - std::sys_common::backtrace::__rust_begin_short_backtrace::hea90534575b9fc13
  17:     0x55b7db79200d - std::rt::lang_start::{{closure}}::h5c94db54965fe6bb
  18:     0x55b7dbc2d491 - std::rt::lang_start_internal::h4d236095b69a230b
  19:     0x55b7db7571b5 - main
  20:     0x7f2819dce24a - __libc_start_call_main
                               at ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
  21:     0x7f2819dce305 - __libc_start_main_impl
                               at ./csu/../csu/libc-start.c:360:3
  22:     0x55b7db6a95d5 - _start
  23:                0x0 - <unknown>

I haven't dug any deeper. It should be easily reproducible.

Looks like it should be a quick fix. Cool project! :-)

Merci!

Julien-cpsn commented 5 months ago

Hi!

The error says unhandled file type which is done "on purpose" I would say. Actually, you cannot use the application on directory that have other files than the ATAC ones (config atac.toml, log atac.log and collections my_collection.json).

TL;DR: Start with an empty directory and then it will be "atac-only".

If you think this was a bad idea, it can change without problem :)

Merci également

brandon1024 commented 5 months ago

In that case, perhaps a better error message would help. Additionally it's not stated anywhere in the documentation the requirements for this directory.

I'll be honest, I do find this behaviour a bit strange (and concerning). Correct me if I'm wrong, but I get the impression that atac reads everything in the config directory at startup to configure itself. I worry a bit about this; reading and parsing arbitrary files can be potentially unsafe.

I won't push the issue further though :-) It's your call if you'd like to keep existing behaviour or make changes, I'm only trying to help!

Additional Thoughts

If it were me, I would organize atac.toml such that paths to atac.log and collections are defined there, and only those files listed there are read at startup, instead of the current behaviour of reading everything in that directory.

[core]
    log-path = "atac.log"

[collections]
    my_collection = "my_collection.json"

This has the added benefit that you can separate configuration from runtime files: it's pretty common to see applications use configuration files in the user's $HOME directory or $XDG_CONFIG_HOME, with runtime files and other configuration files located in $XDG_DATA_HOME/$XDG_CONFIG_HOME.

If users could put atac.toml in their home directory, the command line option --directory <dir> would be no longer strictly necessary.

I digress :sweat_smile:

Julien-cpsn commented 5 months ago

In that case, perhaps a better error message would help. Additionally it's not stated anywhere in the documentation the requirements for this directory.

I'll be honest, I do find this behaviour a bit strange (and concerning). Correct me if I'm wrong, but I get the impression that atac reads everything in the config directory at startup to configure itself. I worry a bit about this; reading and parsing arbitrary files can be potentially unsafe.

I won't push the issue further though :-) It's your call if you'd like to keep existing behaviour or make changes, I'm only trying to help!

It may seem unclear until you read the code, the application is not parsing anything that is not handleable. So no worries about that. I removed the panic when comming to a non-handleable file, it will just ignore it.

Additional Thoughts

If it were me, I would organize atac.toml such that paths to atac.log and collections are defined there, and only those files listed there are read at startup, instead of the current behaviour of reading everything in that directory.

[core]
    log-path = "atac.log"

[collections]
    my_collection = "my_collection.json"

This has the added benefit that you can separate configuration from runtime files: it's pretty common to see applications use configuration files in the user's $HOME directory or $XDG_CONFIG_HOME, with runtime files and other configuration files located in $XDG_DATA_HOME/$XDG_CONFIG_HOME.

If users could put atac.toml in their home directory, the command line option --directory <dir> would be no longer strictly necessary.

Yes it is planned that the config file (atac.toml) will host more of those things (e.g. log file path). However, listing the collections in the config file is not part of the main philosophy. But I'll keep that in mind, it's an interesting point of view.