Sending email using libcurl - follow-up

Question

This question is continuation of: Sending email using libcurl

I developed a class to send email. Does this code have any problems? Is it thread-safe?

#include <ctime>
#include <cstring>
#include <curl/curl.h>

class Email 
{
public:
    Email        (const std::string &to,
                  const std::string &from,
                  const std::string &nameFrom,
                  const std::string &subject,
                  const std::string &body,
                  const std::string &cc = "");

    CURLcode send(const std::string &url,
                  const std::string &userName, 
                  const std::string &password);

private:
    struct StringData {
            std::string msg;
            size_t bytesleft;
            StringData(std::string &&m) : msg{ m }, bytesleft{ msg.size() } {}
            StringData(std::string  &m) = delete;
        };

    static std::string _dateTimeNow();
    std::string _generateMessageId() const;
    static size_t _payloadSource(void *ptr, size_t size, size_t nmemb, void *userp);
    std::string _setPayloadText();

    std::string _to, _from, _cc, _nameFrom, _subject, _body;
};

Email::Email(const std::string &to,
             const std::string &from,
             const std::string &nameFrom,
             const std::string &subject,
             const std::string &body,
             const std::string &cc)
{
    _to       = to;
    _from     = from;
    _nameFrom = nameFrom;
    _subject  = subject;
    _body     = body;
    _cc       = cc.empty() ? to : cc;
}

CURLcode Email::send(const std::string &url,
                     const std::string &userName, 
                     const std::string &password)
{
    CURLcode ret = CURLE_OK;

    struct curl_slist *recipients = NULL;

    CURL *curl = curl_easy_init();

    StringData textData { _setPayloadText() };

    if (curl) {
        curl_easy_setopt(curl, CURLOPT_USERNAME,     userName.c_str());
        curl_easy_setopt(curl, CURLOPT_PASSWORD,     password.c_str());
        curl_easy_setopt(curl, CURLOPT_URL,          url     .c_str());

        curl_easy_setopt(curl, CURLOPT_USE_SSL,      (long)CURLUSESSL_ALL);
        //curl_easy_setopt(curl, CURLOPT_CAINFO, "/path/to/certificate.pem");

        curl_easy_setopt(curl, CURLOPT_MAIL_FROM,    ("<" + _from + ">").c_str());
        recipients = curl_slist_append(recipients,   ("<" + _to   + ">").c_str());
        recipients = curl_slist_append(recipients,   ("<" + _cc   + ">").c_str());

        curl_easy_setopt(curl, CURLOPT_MAIL_RCPT,    recipients);
        curl_easy_setopt(curl, CURLOPT_READFUNCTION, _payloadSource);
        curl_easy_setopt(curl, CURLOPT_READDATA,     &textData);
        curl_easy_setopt(curl, CURLOPT_UPLOAD,       1L);
        curl_easy_setopt(curl, CURLOPT_VERBOSE,      1L);

        ret = curl_easy_perform(curl);

        if (ret != CURLE_OK) {
            std::cerr << "curl_easy_perform() failed: " << curl_easy_strerror(ret) << "\n";
        }

        curl_slist_free_all(recipients);
        curl_easy_cleanup(curl);
    }

    return ret;
}

std::string Email::_dateTimeNow()
{
    const int RFC5322_TIME_LEN = 32;

    std::string ret;
    ret.resize(RFC5322_TIME_LEN);

    time_t tt;

#ifdef _MSC_VER
    time(&tt);
    tm *t = localtime(&tt);
#else
    tm tv, *t = &tv;
    tt = time(&tt);
    localtime_r(&tt, t);
#endif

    strftime(&ret[0], RFC5322_TIME_LEN, "%a, %d %b %Y %H:%M:%S %z", t);

    return ret;
}

std::string Email::_generateMessageId() const
{
    const int MESSAGE_ID_LEN = 37;

    tm t;
    time_t tt;
    time(&tt);

    std::string ret;
    ret.resize(15);

#ifdef _MSC_VER
    gmtime_s(&t, &tt);
#else
    gmtime_r(&tt, &t);
#endif

    strftime(const_cast<char *>(ret.c_str()),
             MESSAGE_ID_LEN,
             "%Y%m%d%H%M%S.",
             &t);

    ret.reserve(MESSAGE_ID_LEN);

    static const char alphaNum[] =
        "0123456789"
        "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
        "abcdefghijklmnopqrstuvwxyz";

    while (ret.size() < MESSAGE_ID_LEN) {
        ret += alphaNum[rand() % (sizeof(alphaNum) - 1)];
    }

    return ret;
}

size_t Email::_payloadSource(void *ptr, size_t size, size_t nmemb, void *userp)
{
    StringData *text = reinterpret_cast<StringData *>(userp);

    if ((size == 0) || (nmemb == 0) || ((size*nmemb) < 1) || (text->bytesleft == 0)) {
        return 0;
    }

    if ((nmemb * size) >= text->msg.size()) {
        text->bytesleft = 0;
        return text->msg.copy(reinterpret_cast<char *>(ptr), text->msg.size());
    }

    return 0;
}

std::string Email::_setPayloadText()
{
    std::string ret;

    ret += "Date: "  + _dateTimeNow() + ">\r\n";
    ret += "To: <"   + _to            + ">\r\n";
    ret += "From: <" + _from          + "> (" + _nameFrom + ")\r\n";
    ret += "Cc: <"   + _cc            + "> (" + _nameFrom + ")\r\n";

    ret += "Message-ID: <"  + _generateMessageId() + "@" + _from.substr(_from.find('@') + 1) + ">\r\n";
    ret += "Subject: "      + _subject + "\r\n";
    ret += "\r\n";

    ret += _body + "\r\n";

    ret += "\r\n";
    ret += "\r\n"; // "It could be a lot of lines, could be MIME encoded, whatever.\r\n";
    ret += "\r\n"; // "Check RFC5322.\r\n";

    return ret;
}

int main()
{
    Email email("[email protected]",
                "[email protected]",
                "FromName",
                "Subject",
                "Body");

    email.send ("smtp://smtp.email.x:25",
                "UserName",
                "Password");
}

Answer 1

Here are some things that may help you improve your code.

Use required #includes

The code uses std::string and std::cerr but doesn't have the required headers that define those. The code should include these lines:

#include <string>
#include <iostream>

Don't #include files that are not needed

The code does not any longer use anything from <cstring>, so that header can and should be omitted.

Avoid using a leading underscore for member variables

It's probably safer to use a trailing underscore for member variable names than a leading underscore. Names beginning with an underscore are reserved by the C++ standard to the implementation in global namespace and also elsewhere depending on the scope and the character that follows. For that reason, it's better to avoid this naming scheme for portability. See this question for more details.

Prefer modern initializers for constructors

The constructor use the more modern initializer style rather than the old style you're currently using. Instead of this:

Email::Email(const std::string &to,
             const std::string &from,
             const std::string &nameFrom,
             const std::string &subject,
             const std::string &body,
             const std::string &cc)
{
    _to       = to;
    _from     = from;
    _nameFrom = nameFrom;
    _subject  = subject;
    _body     = body;
    _cc       = cc.empty() ? to : cc;
}

You could use this:

Email::Email(const std::string &to,
             const std::string &from,
             const std::string &nameFrom,
             const std::string &subject,
             const std::string &body,
             const std::string &cc)
    : to_{to},
      from_{from},
      cc_{cc},
      nameFrom_{nameFrom},
      subject_{subject},
      body_{body}
{ }

Note that because members are always initialized in declaration order and cc_ is declared before nameFrom_ in this class, it's good practice to write the constructor so that the fields are in the same order as declaration order.

Read the standard you're trying to implement

If you're trying to implement an email writer, it's useful to understand exactly what should be in an email. The relevant RFCs are RFC 5322 and RFC 6854. They include a number of things that will help you write emails that won't get rejected by servers. In particular, RFC 5322 includes this:

Note: Some legacy implementations used the simple form where the addr-spec appears without the angle brackets, but included the name of the recipient in parentheses as a comment following the addr-spec. Since the meaning of the information in a comment is unspecified, implementations SHOULD use the full name-addr form of the mailbox, instead of the legacy form, to specify the display name associated with a mailbox. Also, because some legacy implementations interpret the comment, comments generally SHOULD NOT be used in address fields to avoid confusing such implementations.

So what that means is that lines like this:

ret += "From: <" + from_          + "> (" + nameFrom_ + ")\r\n";

Should actually be written instead like this:

ret += "From: \""  + nameFrom + "\" <" + from_ + ">\r\n";

Refine your objects

The notion of an address is used several times in an email and in your program. Paraphrasing RFC 5322, an address consists of an optional display name followed by an email address enclosed in angle brackets. Your program would be both neater and more correct if you defined an object to encapsulate that concept and used it within the program. Here's a version I wrote:

class EmailAddress 
{
public:
    EmailAddress(const std::string &email) :
        email_{email.size() ? "<"+email+">" : ""}
    {}
    EmailAddress(const std::string &email, const std::string &displayName)
        : email_{email.size() ? "<"+email+">" : ""},
          name_{"\""+displayName+"\""}
    {}
    std::string domain() const { return email_.substr(email_.find('@') + 1); }
    explicit operator const char *() const { return email_.c_str(); }
    friend std::ostream& operator<<(std::ostream &out, const EmailAddress& email) {
        return out << email.name_ << " " << email.email_;
    }
private:
    std::string email_;
    std::string name_;
};

With this, one can write an extractor:

std::ostream& operator<<(std::ostream &out, const Addresses &addrs) {
    if (addrs.size() == 0) 
        return out;
    out << addrs.front(); 
    for (auto it = addrs.begin()+1; it != addrs.end(); ++it) {
        out << "," << *it;
    }
    return out;
}

Now you can use the extractor (with a std::stringstream) for formatting the message body where both the text name and angle-bracketed address are used, and an explicit cast to const char * where only the angle-bracketed address is used, as with all of the curl options:

curl_easy_setopt(curl, CURLOPT_MAIL_FROM, (const char *)from_);

Fix the bugs

The handling of the cc field is still not right. Right now, with no address supplied for the cc field, the message includes the following header line:

Cc: <> (FromName)

This is wrong for three reasons. First, the addr-spec which RFC 5322 says should be enclosed within the angle brackets is incorrect because it's not a valid email address. Second, the grammar specified by RFC 5322 says that comments should not be used in address fields as quoted above. Third, the CC address and the FromName don't necessarily correspond, so it doesn't make sense to combine them here. One easy way to fix that is to replace this line from the original code:

ret += "Cc: <" + cc_ + "> (" + nameFrom_ + ")\r\n";

With this:

if (cc_.length()) {
    ret += "Cc: <" + cc_ + ">\r\n";
}

Also, there is a spurious '>' character after the date string.

Carefully format the message body

There is no reason to have so many blank lines appended to the end of the message. Also, RFC 5322 recommends that lines within the body be no longer than 998 characters. I don't know if curl does anything special to the body of the email if lines are longer than that, but I would doubt it. In that case, it would be nice if your program handled this.

Use string concatenation

The Email::setPayloadText() function has many repeated lines where the std::string operator+ is used multiple times with fixed strings. Those multiple calls don't need to happen. You could simply rely on the fact that C++ merges separate constant strings automatically. For example, here is a recoded version:

std::string Email::setPayloadText()
{
    std::string payloadText = "Date: "  + dateTimeNow() + "\r\n"
     "To: <"   + to_   + ">\r\n"
     "From: \"" + nameFrom_ +"\" <" + from_ + ">\r\n";
     if (cc_.length()) {
        payloadText += "Cc: <" + cc_ + ">\r\n";
    }
    payloadText += "Message-ID: <" + generateMessageId() + "@" + from_.substr(from_.find('@') + 1) + ">\r\n"
     "Subject: " + subject_ + "\r\n"
     "\r\n"
     + body_ + "\r\n"
     "\r\n";
     return payloadText;
}

Consider using a better random number generator

If you are using a compiler that supports at least C++11, consider using a better random number generator. In particular, instead of rand, you might want to look at std::uniform_real_distribution and friends in the <random> header.

std::string Email::_generateMessageId() const
{
    const size_t MESSAGE_ID_LEN = 37;
    tm t;
    time_t tt;
    time(&tt);
#ifdef _MSC_VER
    gmtime_s(&t, &tt);
#else
    gmtime_r(&tt, &t);
#endif
    std::string ret;
    ret.resize(MESSAGE_ID_LEN);
    size_t datelen = std::strftime(&ret[0], MESSAGE_ID_LEN,"%Y%m%d%H%M%S",&t);
    static const std::string alphaNum{
        "0123456789"
        "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
        "abcdefghijklmnopqrstuvwxyz"};
    std::mt19937 gen;
    std::uniform_int_distribution<> dis(0, alphaNum.length()-1);
    std::generate_n(ret.begin()+datelen, MESSAGE_ID_LEN-datelen, [&](){ return alphaNum[dis(gen)];});
    return ret;
}

This will also require two additional include files: <algorithm> and <random>.

Consider handling long messages

If the buffer size handed to the payloadSource() callback from curl isn't big enough to contain the entire message, the current version of the code will simply send nothing. In real code, you would probably want to handle that case by decrementing the bytesleft parameter by either the size of the message or the length of the buffer -- whichever is smallest.