4
\$\begingroup\$

This is some code I wrote five years ago and I'm now using it partially as instructional material. I think there are some things that are 'off' about it, namely the top of the main function being the definition of a few variables instead of the argument parsing, and the actual generation not being split into its own function.

#include <chrono>
#include <functional>
#include <iostream>
#include <random>
#include <string>

static void show_usage(std::string name)
{
    std::cerr << "Usage: " << name << " <option(s)>\n"
              << "Options:\n"
              << "\t-h,--help\t\tShow this help message\n"
              << "\t-l,--length LENGTH\tSpecify the password length\n"
              << "\t-s,--seed SEED\t\tUse a unique seed for the random engine\n"
              << "\t-v,--version\t\tOutput the program version\n";
}

int main (int argc, char *argv[])
{
    std::string charset ("abcdefghijklmnopqrstuvwxyz"
                         "ABCDEFGHIJKLMNOPQRSTUVWXYZ" 
                         "1234567890!@#$%^&*()");
    int length = 16;
    std::default_random_engine generator;
    std::uniform_int_distribution<int> distribution (0, (int) charset.size() - 1);
    generator.seed (std::chrono::system_clock::now().time_since_epoch().count());

    if (argc >= 2) {
        for (int i = 1; i < argc; i++) {
            std::string arg = argv[i];
            if ((arg ==  "-l") || (arg == "--length")) {
                if (i + 1 < argc) {
                    length = atoi(argv[++i]);
                } else {
                    std::cerr << "-l,--length takes one argument" << std::endl;
                    return 1;
                }
            } else if ((arg == "-s") || (arg == "--seed")) {
                if (i + 1 < argc) {
                    std::string str (argv[++i]);
                    std::seed_seq seed (str.begin(), str.end());
                    generator.seed (seed);
                } else {
                    std::cerr << "-s,--seed takes one argument" << std::endl;
                    return 1;
                }
            } else if ((arg == "-h") || (arg == "--help")) {
                show_usage(argv[0]);
                return 0;
            } else if ((arg == "-v") || (arg == "--version")) {
                std::cout << "Version 0.0.1" << std::endl;
                return 0;
            }
        }
    }

    auto randchar = std::bind (distribution, generator);
    std::string password;
    password.resize(length);

    for (int i = 0; i < length; i++) {
        password.at(i) = charset.at(randchar());
    }

    std::cout << password << std::endl;
    return 0;
}
\$\endgroup\$
5
  • 1
    \$\begingroup\$ By the way, please do not forget to add C++ tag to the question in the future, so it would be visible on C++ questions page. Most C++ programmers browse C++ questions page. \$\endgroup\$ Aug 25 '17 at 4:47
  • \$\begingroup\$ Please don't use std::default_random_engine for cryptographic purposes; especially if this code is used as instruction material. If you don't have an alternative, at least add a huge warning 😊 \$\endgroup\$ Aug 25 '17 at 9:33
  • 1
    \$\begingroup\$ @K. Biermann: Could you expand on that one please? Whats the why? Thanks. \$\endgroup\$
    – Jimbo
    Aug 25 '17 at 12:34
  • 1
    \$\begingroup\$ @Jimbo I would hazard a guess that it's referring to this section of documentation It is the library implemention's selection of a generator that provides at least acceptable engine behavior for relatively casual, inexpert, and/or lightweight use. \$\endgroup\$
    – zenware
    Aug 25 '17 at 17:26
  • 1
    \$\begingroup\$ Sorry; std::default_engine_random is not guaranteed to be „random enough“ in every situation – this means an attacker may be able guess the generated data and thus guess the generated password. See crypto.stackexchange.com/questions/15662/… or en.m.wikipedia.org/wiki/… (the first one is about rand but can also apply to std::default_random_engine) \$\endgroup\$ Aug 25 '17 at 20:11
3
\$\begingroup\$

The code looks good. May be argument parsing could be done better, but in general this is solid simple password generator. There are things to mention though, but they mostly apply on reusability and expandability

Separate concerns

At the moment the main() function does quite a lot of things:

  1. Setup random number generator

  2. Parse program arguments

  3. Generate password

  4. Output it to the user

Encapsulate

I'm not saying reduce usability, but the password generator is very much like random number generator, it just gives out strings with random elements instead of bit blocks.

So, in my opinion, the following code would have sound design:

class random_password_generator
{
    //random engine and distribution
public:
    random_password_generator() 
    {
        //initialize
    }

    //random engine will decide rule of 5

    std::string operator()()
    {
        //create password
    }
}

Parsing arguments can be just a plain function. For complex cases may be using Boost.program_options would be better.


Code Review

atoi(argv[++i]);

atoi is a C standard library function. stoi is a C++ function standard library. In general, C++ library is more compatible with concepts of C++ itself, whereas C library usually needs some wrappers to be usable with the rest of C++.

int length = 16;

My personal preference is to use unsigned int or std::size_t for things that cannot have negative values.

Do not use bind unless necessary

std::bind() might have problems that lambda can't solve, but I'm yet to encounter such problems. In general: use lambda for function objects unless you can't. The reason is that std::bind does heap allocation, type erasure, and possibly other stuff, whereas lambda is just anonymous object with operator().

Range loops

for (int i = 0; i < length; i++)

The above also has a chance to fail in case you'll change the resizing part of the string. It is better to use range for loop:

for (char& c: password)
{
    //assign
}

It will also remove the need to use at(), which does bounds checking, and in general frowned upon in release code.

Even better

Use standard algorithms. Since the functor generates random characters, code could just use std::generate(password.begin(), password.end(), generator).

Do not use std::endl

std::endl is not only a platform independent way to print newline. Printing newline is achieved with streaming '\n'. Even on windows ostream will deal with it. std::endl prints newline and flushes stream, which might cause detrimental performance hit in tight loops.

Don't use C style casts

(int) charset.size()

C style casts are much more dangerous, and compiler will not warn you if it does something dangerous. On the other hand, using one of C++ style casts will ensure compilation error in case something fishy is going on, except reinterpret_cast<>, which is fishy by definition.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ How is std::endl not platform independent ? it's part of the standard, it might not be optimal but it's still well defined see (en.cppreference.com/w/cpp/io/manip/endl) \$\endgroup\$ Aug 25 '17 at 13:42
  • \$\begingroup\$ I will put "not only" instead of not. \$\endgroup\$ Aug 25 '17 at 13:43
0
\$\begingroup\$

Parameters of type std::string should always be passed as const std::string &.

The code for argument parsing is missing the else show_usage().

The usage function typically outputs a one-liner, while the help function outputs the detailed usage. This is to keep error messages short, so they have a chance of being actually read.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.