Mem value sanitizing - take 3

include/multipass/constants.h

@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) 2019 Canonical, Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef MULTIPASS_CONSTANTS_H
+#define MULTIPASS_CONSTANTS_H
+
+namespace multipass
+{
+constexpr auto min_memory_size = "128M";
+constexpr auto min_disk_size = "512M";
+} // namespace multipass
+
+#endif // MULTIPASS_CONSTANTS_H

include/multipass/utils.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017-2018 Canonical, Ltd.
+ * Copyright (C) 2017-2019 Canonical, Ltd.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -18,6 +18,7 @@
 #ifndef MULTIPASS_UTILS_H
 #define MULTIPASS_UTILS_H
 
+#include <multipass/optional.h>
 #include <multipass/path.h>
 #include <multipass/virtual_machine.h>
 
@@ -51,8 +52,9 @@ QString make_uuid();
 std::string contents_of(const multipass::Path& file_path);
 bool has_only_digits(const std::string& value);
 void validate_server_address(const std::string& value);
-bool valid_memory_value(const QString& mem_string);
-bool valid_hostname(const QString& name_string);
+optional<long long> in_bytes(const std::string& mem_value);
+std::string in_bytes_string(long long bytes);
+bool valid_hostname(const std::string& name_string);
 bool invalid_target_path(const QString& target_path);
 std::string to_cmd(const std::vector<std::string>& args, QuoteType type);
 bool run_cmd_for_status(const QString& cmd, const QStringList& args, const int timeout=30000);

src/client/cmd/launch.cpp

@@ -21,6 +21,7 @@
 #include "animated_spinner.h"
 #include <multipass/cli/argparser.h>
 #include <multipass/cli/client_platform.h>
+#include <multipass/constants.h>
 
 #include <fmt/format.h>
 
@@ -256,11 +257,15 @@ mp::ReturnCode cmd::Launch::request_launch()
         {
             if (error == LaunchError::INVALID_DISK_SIZE)
             {
-                error_details = fmt::format("Invalid disk size value supplied: {}", request.disk_space());
+                error_details =
+                    fmt::format("Invalid disk size value supplied: {}. Note that a minimum of {} is required.",
+                                request.disk_space(), min_disk_size);
             }
             else if (error == LaunchError::INVALID_MEM_SIZE)
             {
-                error_details = fmt::format("Invalid memory size value supplied: {}", request.mem_size());
+                error_details =
+                    fmt::format("Invalid memory size value supplied: {}. Note that a minimum of {} is required.",
+                                request.mem_size(), min_memory_size);
             }
             else if (error == LaunchError::INVALID_HOSTNAME)
             {

src/daemon/daemon.cpp

@@ -20,6 +20,7 @@
 #include "json_writer.h"
 
 #include <multipass/cloud_init_iso.h>
+#include <multipass/constants.h>
 #include <multipass/exceptions/exitless_sshprocess_exception.h>
 #include <multipass/exceptions/sshfs_missing_error.h>
 #include <multipass/exceptions/start_exception.h>
@@ -52,6 +53,7 @@
 #include <functional>
 #include <stdexcept>
 #include <unordered_set>
+#include <utility>
 
 namespace mp = multipass;
 namespace mpl = multipass::logging;
@@ -69,6 +71,8 @@ constexpr auto reboot_cmd = "sudo reboot";
 constexpr auto up_timeout = 2min; // This may be tweaked as appropriate and used in places that wait for ssh to be up
 constexpr auto stop_ssh_cmd = "sudo systemctl stop ssh";
 constexpr auto max_install_sshfs_retries = 3;
+const auto normalized_min_mem = mp::utils::in_bytes(mp::min_memory_size);
+const auto normalized_min_disk = mp::utils::in_bytes(mp::min_disk_size);
 
 mp::Query query_from(const mp::LaunchRequest* request, const std::string& name)
 {
@@ -151,24 +155,22 @@ void prepare_user_data(YAML::Node& user_data_config, YAML::Node& vendor_config)
 }
 
 mp::VirtualMachineDescription to_machine_desc(const mp::LaunchRequest* request, const std::string& name,
+                                              const std::string& mem_size, const std::string& disk_space,
                                               const std::string& mac_addr, const std::string& ssh_username,
                                               const mp::VMImage& image, YAML::Node& meta_data_config,
                                               YAML::Node& user_data_config, YAML::Node& vendor_data_config,
                                               const mp::SSHKeyProvider& key_provider)
 {
     const auto num_cores = request->num_cores() < 1 ? 1 : request->num_cores();
-    const auto mem_size = request->mem_size().empty() ? "1G" : request->mem_size();
-    const auto disk_size = request->disk_space().empty() ? "5G" : request->disk_space();
     const auto instance_dir = mp::utils::base_dir(image.image_path);
     const auto cloud_init_iso =
         make_cloud_init_image(name, instance_dir, meta_data_config, user_data_config, vendor_data_config);
-    return {num_cores, mem_size, disk_size, name, mac_addr, ssh_username, image, cloud_init_iso, key_provider};
+    return {num_cores, mem_size, disk_space, name, mac_addr, ssh_username, image, cloud_init_iso, key_provider};
 }
 
 template <typename T>
-auto name_from(const mp::LaunchRequest* request, mp::NameGenerator& name_gen, const T& currently_used_names)
+auto name_from(const std::string& requested_name, mp::NameGenerator& name_gen, const T& currently_used_names)
 {
-    auto requested_name = request->instance_name();
     if (requested_name.empty())
     {
         auto name = name_gen.make_name();
@@ -278,25 +280,35 @@ auto fetch_image_for(const std::string& name, const mp::FetchType& fetch_type, m
 
 auto validate_create_arguments(const mp::LaunchRequest* request)
 {
-    mp::LaunchError launch_error;
+    auto mem_size = request->mem_size();
+    auto disk_space = request->disk_space();
+    auto instance_name = request->instance_name();
+    auto option_errors = mp::LaunchError{};
 
-    if (!request->disk_space().empty() && !mp::utils::valid_memory_value(QString::fromStdString(request->disk_space())))
-    {
-        launch_error.add_error_codes(mp::LaunchError::INVALID_DISK_SIZE);
-    }
+    const auto opt_mem_size = mp::utils::in_bytes(mem_size.empty() ? "1G" : mem_size);
+    const auto opt_disk_space = mp::utils::in_bytes(disk_space.empty() ? "5G" : disk_space);
 
-    if (!request->mem_size().empty() && !mp::utils::valid_memory_value(QString::fromStdString(request->mem_size())))
-    {
-        launch_error.add_error_codes(mp::LaunchError::INVALID_MEM_SIZE);
-    }
+    if (opt_mem_size && *opt_mem_size >= normalized_min_mem)
+        mem_size = mp::utils::in_bytes_string(*opt_mem_size);
+    else
+        option_errors.add_error_codes(mp::LaunchError::INVALID_MEM_SIZE);
 
-    if (!request->instance_name().empty() &&
-        !mp::utils::valid_hostname(QString::fromStdString(request->instance_name())))
-    {
-        launch_error.add_error_codes(mp::LaunchError::INVALID_HOSTNAME);
-    }
+    if (opt_disk_space && *opt_disk_space >= normalized_min_disk)
+        disk_space = mp::utils::in_bytes_string(*opt_disk_space);
+    else
+        option_errors.add_error_codes(mp::LaunchError::INVALID_DISK_SIZE);
 
-    return launch_error;
+    if (!request->instance_name().empty() && !mp::utils::valid_hostname(request->instance_name()))
+        option_errors.add_error_codes(mp::LaunchError::INVALID_HOSTNAME);
+
+    struct CheckedArguments
+    {
+        std::string mem_size;
+        std::string disk_space;
+        std::string instance_name;
+        mp::LaunchError option_errors;
+    } ret{mem_size, disk_space, instance_name, option_errors};
+    return ret;
 }
 
 auto grpc_status_for_mount_error(const std::string& instance_name)
@@ -660,7 +672,15 @@ try // clang-format on
     if (metrics_opt_in.opt_in_status == OptInStatus::ACCEPTED)
         metrics_provider.send_metrics();
 
-    auto name = name_from(request, *config->name_generator, vm_instances);
+    auto checked_args = validate_create_arguments(request);
+
+    if (!checked_args.option_errors.error_codes().empty())
+    {
+        return grpc::Status(grpc::StatusCode::INVALID_ARGUMENT, "Invalid arguments supplied",
+                            checked_args.option_errors.SerializeAsString());
+    }
+
+    auto name = name_from(checked_args.instance_name, *config->name_generator, vm_instances);
 
     if (vm_instances.find(name) != vm_instances.end() || deleted_instances.find(name) != deleted_instances.end())
     {
@@ -675,14 +695,6 @@ try // clang-format on
 
     config->factory->check_hypervisor_support();
 
-    auto option_errors = validate_create_arguments(request);
-
-    if (!option_errors.error_codes().empty())
-    {
-        return grpc::Status(grpc::StatusCode::INVALID_ARGUMENT, "Invalid arguments supplied",
-                            option_errors.SerializeAsString());
-    }
-
     auto progress_monitor = [server](int progress_type, int percentage) {
         LaunchReply create_reply;
         create_reply.mutable_launch_progress()->set_percent_complete(std::to_string(percentage));
@@ -727,8 +739,9 @@ try // clang-format on
         }
     }
     auto vm_desc =
-        to_machine_desc(request, name, mac_addr, config->ssh_username, vm_image, meta_data_cloud_init_config,
-                        user_data_cloud_init_config, vendor_data_cloud_init_config, *config->ssh_key_provider);
+        to_machine_desc(request, name, checked_args.mem_size, checked_args.disk_space, mac_addr, config->ssh_username,
+                        vm_image, meta_data_cloud_init_config, user_data_cloud_init_config,
+                        vendor_data_cloud_init_config, *config->ssh_key_provider);
 
     config->factory->prepare_instance_image(vm_image, vm_desc);
 

src/utils/utils.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017-2018 Canonical, Ltd.
+ * Copyright (C) 2017-2019 Canonical, Ltd.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -29,6 +29,7 @@
 
 #include <algorithm>
 #include <array>
+#include <cassert>
 #include <cctype>
 #include <fstream>
 #include <random>
@@ -54,18 +55,52 @@ QDir mp::utils::base_dir(const QString& path)
     return info.absoluteDir();
 }
 
-bool mp::utils::valid_memory_value(const QString& mem_string)
+auto mp::utils::in_bytes(const std::string& mem_value) -> optional<long long>
 {
-    QRegExp matcher("\\d+((K|M|G)(B){0,1}){0,1}$");
+    static constexpr auto kilo = 1024LL;
+    static constexpr auto mega = kilo * kilo;
+    static constexpr auto giga = mega * kilo;
 
-    return matcher.exactMatch(mem_string);
+    QRegExp matcher("^(\\d+)([KMG])?B?$", Qt::CaseInsensitive);
+
+    if (matcher.exactMatch(QString::fromStdString(mem_value)))
+    {
+        auto val = matcher.cap(1).toLongLong(); // value is in the second capture (1st one is the whole match)
+        const auto unit = matcher.cap(2);       // unit in the third capture (idem)
+        if (!unit.isEmpty())
+        {
+            switch (unit.at(0).toLower().toLatin1())
+            {
+            case 'g':
+                val *= giga;
+                break;
+            case 'm':
+                val *= mega;
+                break;
+            case 'k':
+                val *= kilo;
+                break;
+            default:
+                assert(false && "Shouldn't be here (invalid unit)");
+            }
+        }
+
+        return val;
+    }
+
+    return {};
+}
+
+std::string mp::utils::in_bytes_string(long long bytes)
+{
+    return std::to_string(bytes) + "B";
 }
 
-bool mp::utils::valid_hostname(const QString& name_string)
+bool mp::utils::valid_hostname(const std::string& name_string)
 {
     QRegExp matcher("^([a-zA-Z]|[a-zA-Z][a-zA-Z0-9\\-]*[a-zA-Z0-9])");
 
-    return matcher.exactMatch(name_string);
+    return matcher.exactMatch(QString::fromStdString(name_string));
 }
 
 bool mp::utils::invalid_target_path(const QString& target_path)

tests/stub_terminal.h

@@ -28,25 +28,23 @@ namespace test
 class StubTerminal : public multipass::Terminal
 {
 public:
-    StubTerminal()
-        : cout_stream{null_stream}
-    {}
-    StubTerminal(std::ostream& cout)
-        : cout_stream{cout}
+    StubTerminal(std::ostream& cout, std::ostream& cerr, std::istream& cin)
+        : cout_stream{cout}, cerr_stream{cerr}, cin_stream{cin}
     {}
+
     ~StubTerminal() override = default;
 
     std::istream& cin() override
     {
-        return null_stream;
+        return cin_stream;
     }
     std::ostream& cout() override
     {
         return cout_stream;
     }
     std::ostream& cerr() override
     {
-        return cout_stream;
+        return cerr_stream;
     }
 
     bool cin_is_live() const override
@@ -59,8 +57,9 @@ class StubTerminal : public multipass::Terminal
         return true;
     }
 private:
-    std::stringstream null_stream;
     std::ostream &cout_stream;
+    std::ostream& cerr_stream;
+    std::istream& cin_stream;
 };
 
 } // namespace test

tests/test_cli_client.cpp

@@ -31,21 +31,18 @@
 
 #include <gmock/gmock.h>
 
+#include <sstream>
+
 namespace mp = multipass;
 namespace mpt = multipass::test;
 using namespace testing;
 
 struct Client : public Test
 {
-    int send_command(const std::vector<std::string>& command)
+    int send_command(const std::vector<std::string>& command, std::ostream& cout = trash_stream,
+                     std::ostream& cerr = trash_stream, std::istream& cin = trash_stream)
     {
-        std::stringstream null_stream;
-        return send_command(command, null_stream);
-    }
-
-    int send_command(const std::vector<std::string>& command, std::ostream &cout)
-    {
-        mpt::StubTerminal term(cout);
+        mpt::StubTerminal term(cout, cerr, cin);
         mp::ClientConfig client_config{server_address, mp::RpcConnectionType::insecure,
                                        std::make_unique<mpt::StubCertProvider>(), &term};
         mp::Client client{client_config};
@@ -66,8 +63,11 @@ struct Client : public Test
     mpt::StubCertProvider cert_provider;
     mpt::StubCertStore cert_store;
     mp::DaemonRpc stub_daemon{server_address, mp::RpcConnectionType::insecure, cert_provider, cert_store};
+    static std::stringstream trash_stream; // this may have contents (that we don't care about)
 };
 
+std::stringstream Client::trash_stream; // replace with inline in C++17
+
 // Tests for no postional args given
 TEST_F(Client, no_command_is_error)
 {
@@ -230,9 +230,10 @@ TEST_F(Client, launch_cmd_cloudinit_option_fails_no_value)
 
 TEST_F(Client, launch_cmd_cloudinit_option_reads_stdin_ok)
 {
-    MockStdCin cin("password: passw0rd");
+    MockStdCin cin("password: passw0rd"); // no effect since terminal encapsulation of streams
 
-    EXPECT_THAT(send_command({"launch", "--cloud-init", "-"}), Eq(mp::ReturnCode::Ok));
+    std::stringstream ss;
+    EXPECT_THAT(send_command({"launch", "--cloud-init", "-"}, trash_stream, trash_stream, ss), Eq(mp::ReturnCode::Ok));
 }
 
 // purge cli tests