HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Sending email using libcurl

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
libcurlsendingusingemail

Problem

I developed a function to send email based on this. Does this code have problems? Also, I'm not sure if this code is thread-safe because of the global payloadText variable.

```
#include
#include

static std::string payloadText[11];

std::string dateTimeNow();
std::string generateMessageId();

void setPayloadText(const std::string &to,
const std::string &from,
const std::string &cc,
const std::string &nameFrom,
const std::string &subject,
const std::string &body)
{
payloadText[ 0] = "Date: " + dateTimeNow();
payloadText[ 1] = "To: \r\n";
payloadText[ 2] = "From: (" + nameFrom + ")\r\n";
payloadText[ 3] = "Cc: (" + nameFrom + ")\r\n";
payloadText[ 4] = "Message-ID: \r\n";
payloadText[ 5] = "Subject: " + subject + "\r\n";
payloadText[ 6] = "\r\n";
payloadText[ 7] = body + "\r\n";
payloadText[ 8] = "\r\n";
payloadText[ 9] = "\r\n"; // "It could be a lot of lines, could be MIME encoded, whatever.\r\n";
payloadText[10] = "\r\n"; // "Check RFC5322.\r\n";
}

std::string dateTimeNow()
{
static const char *DAY_NAMES [] = { "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat" };
static const char *MONTH_NAMES[] = { "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec" };

const int RFC1123_TIME_LEN = 29;
time_t t;
struct tm tm;

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

time(&t);
gmtime_s(&tm, &t);

strftime(&ret[0], RFC1123_TIME_LEN + 1, "---, %d --- %Y %H:%M:%S GMT", &tm);
memcpy(&ret[0], DAY_NAMES [tm.tm_wday], 3);
memcpy(&ret[8], MONTH_NAMES[tm.tm_mon], 3);

return ret;
}

std::string generateMessageId()
{
const int MESSAGE_ID_LEN = 37;
time_t t;
struct tm tm;

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

time(&t);
gmtime_s(&tm, &t);

strftime(const_cast(ret.c_str()),
MESSAGE_ID_LEN,
"%Y%m%d%H%

Solution

Here are some things that may help you improve your code.
Use required #includes

The code invokes gmtime_s, memcpy and strlen but doesn't have the required headers that define those. The code should include these lines:

#include 
#include 


First, as noted in http://en.cppreference.com/w/c/chrono/gmtime :

As with all bounds-checked functions, gmtime_s is only guaranteed to be available if __STDC_LIB_EXT1__ is defined by the implementation and if the user defines __STDC_WANT_LIB_EXT1__ to the integer constant 1 before including time.h.

It's also not guaranteed be thread safe, so you might want to use the POSIX gmtime_r instead.
Don't write C code in C++

This is basically C code written with a little bit of C++. It could be greatly improved by using more of a C++ approach, as by using a class to represent an email and having most of the functions implemented as member functions. For example, I did a fairly simple refactoring of this code and came up with this class interface:

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 = ""
         );
    std::string dateTimeNow() const;
    CURLcode send(const std::string &url, 
                  const std::string &username, 
                  const std::string &password);
private:
    // data
    std::string to_, from_, cc_, nameFrom_, subject_, body_;
    // functions
    std::string setPayloadText();
    std::string generateMessageId() const;
    // static functions
    static size_t payload_source(void *ptr, size_t size, size_t nmemb, void *userp);
};


Fix the bugs

The code does not currently format a message correctly because there is no "\r\n" after the date string. It also does not work if there is no "CC" address. Please read the relevant RFCs (e.g. RFC 5322) to understand how an email is properly formatted. For example, the time string format is old. Instead of using the time zone abbreviation "GMT", the preferred format is to use the timezone offset. For that reason, you could just as easily use localtime rather than gmtime.
Use C++11 version of strftime

If you set the locale appropriately, you can eliminate all of the day and month parts from dateTimeNow() and just use this:

std::string dateTimeNow()
{
    const int RFC5322_TIME_LEN = 32;
    time_t t;
    struct tm *tm;

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

    time(&t);
    tm = localtime(&t);

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

    return ret;
}


Eliminate global variables

There is absolutely no reason that payloadText needs to be 11 strings and no reason that it needs to be global. The last parameter of the payload_source is intended to be used for user data. One way to do that would be to define your own class like this:

class stringdata {
public:
    std::string msg;
    size_t bytesleft;

    stringdata(std::string &&m) 
        : msg{m}, bytesleft{msg.size()}
    {}
    stringdata(std::string &m) = delete;
};


The reworked payload_source would be this:

static size_t payload_source(void *ptr, size_t size, size_t nmemb, void *userp)
{
    stringdata *text = reinterpret_cast(userp);

    if ((size == 0) || (nmemb == 0) || ((size*nmemb) bytesleft == 0)) {
        return 0;
    }

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

    return 0;
}


And finally, to use it within sendEmail we can initialize the data like this:

stringdata textdata{setPayloadText(to, from, cc, nameFrom, subject, body)};


(Note that this assumes that setPayloadText is rewritten to return a single std::string -- it's a fairly trivial rewrite.)

Then the options to be able to use these together would look like this:

curl_easy_setopt(curl, CURLOPT_READFUNCTION, payload_source);
curl_easy_setopt(curl, CURLOPT_READDATA,     &textdata);


Use iostreams rather than printf

Instead of this:

if (ret != CURLE_OK) {
    fprintf(stderr, "curl_easy_perform() failed: %s\n", curl_easy_strerror(ret));
}


You could use this:

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

Code Snippets

#include <ctime>
#include <cstring>
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 = ""
         );
    std::string dateTimeNow() const;
    CURLcode send(const std::string &url, 
                  const std::string &username, 
                  const std::string &password);
private:
    // data
    std::string to_, from_, cc_, nameFrom_, subject_, body_;
    // functions
    std::string setPayloadText();
    std::string generateMessageId() const;
    // static functions
    static size_t payload_source(void *ptr, size_t size, size_t nmemb, void *userp);
};
std::string dateTimeNow()
{
    const int RFC5322_TIME_LEN = 32;
    time_t t;
    struct tm *tm;

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

    time(&t);
    tm = localtime(&t);

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

    return ret;
}
class stringdata {
public:
    std::string msg;
    size_t bytesleft;

    stringdata(std::string &&m) 
        : msg{m}, bytesleft{msg.size()}
    {}
    stringdata(std::string &m) = delete;
};
static size_t payload_source(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;
}

Context

StackExchange Code Review Q#139784, answer score: 6

Revisions (0)

No revisions yet.