5
\$\begingroup\$

I am learning C++ and socket programming and OpenSSL. As such, I decided to make a simple client that opens a TLS connection and writes some data as practice. It also serves as a base for more complex applications.

I'm very new to C++, so I don't know if I'm using good naming conventions or other basic practices. The program is using OpenSSL 1.1.0.

#include <errno.h>
#include <unistd.h>
#include <string.h>
#include <resolv.h>
#include <netdb.h>
#include <openssl/ssl.h>
#include <openssl/err.h>

//Not sure what headers are needed or not
//This code (theoretically) writes "Hello World, 123" to a socket over a secure TLS connection
//compiled with g++ -Wall -o client.out client.cpp -L/usr/lib -lssl -lcrypto
//Based off of: https://www.cs.utah.edu/~swalton/listings/articles/ssl_client.c
//The OpenConnection method was heavily based on the answer to this post: https://stackoverflow.com/questions/52727565/client-in-c-use-gethostbyname-or-getaddrinfo

const int ERROR_STATUS = -1;

SSL_CTX *InitSSL_CTX(void)
{
    const SSL_METHOD *method = TLS_client_method(); /* Create new client-method instance */
    SSL_CTX *ctx = SSL_CTX_new(method);

    if (ctx == nullptr)
    {
        ERR_print_errors_fp(stderr);
        exit(EXIT_FAILURE);
    }
    return ctx;
}

int OpenConnection(const char *hostname, const char *port)
{
    struct hostent *host;
    if ((host = gethostbyname(hostname)) == nullptr)
    {
        perror(hostname);
        exit(EXIT_FAILURE);
    }

    struct addrinfo hints = {0}, *addrs;
    hints.ai_family = AF_UNSPEC;
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_protocol = IPPROTO_TCP;

    const int status = getaddrinfo(hostname, port, &hints, &addrs);
    if (status != 0)
    {
        fprintf(stderr, "%s: %s\n", hostname, gai_strerror(status));
        exit(EXIT_FAILURE);
    }

    int sfd, err;
    for (struct addrinfo *addr = addrs; addr != nullptr; addr = addr->ai_next)
    {
        sfd = socket(addrs->ai_family, addrs->ai_socktype, addrs->ai_protocol);
        if (sfd == ERROR_STATUS)
        {
            err = errno;
            continue;
        }

        if (connect(sfd, addr->ai_addr, addr->ai_addrlen) == 0)
        {
            break;
        }

        err = errno;
        sfd = ERROR_STATUS;
        close(sfd);
    }

    freeaddrinfo(addrs);

    if (sfd == ERROR_STATUS)
    {
        fprintf(stderr, "%s: %s\n", hostname, strerror(err));
        exit(EXIT_FAILURE);
    }
    return sfd;
}

void DisplayCerts(SSL *ssl)
{
    X509 *cert = SSL_get_peer_certificate(ssl); /* get the server's certificate */
    if (cert != nullptr)
    {
        printf("Server certificates:\n");
        char *line = X509_NAME_oneline(X509_get_subject_name(cert), 0, 0);
        printf("Subject: %s\n", line);
        delete line;
        line = X509_NAME_oneline(X509_get_issuer_name(cert), 0, 0);
        printf("Issuer: %s\n", line);
        delete line;
        X509_free(cert);
    }
    else
    {
        printf("Info: No client certificates configured.\n");
    }
}

int main(int argc, char const *argv[])
{
    SSL_CTX *ctx = InitSSL_CTX();
    SSL *ssl = SSL_new(ctx);
    if (ssl == nullptr)
    {
        fprintf(stderr, "SSL_new() failed\n");
        exit(EXIT_FAILURE);
    }

    //Host is hardcoded to localhost for testing purposes
    const int sfd = OpenConnection("127.0.0.1", argv[1]);
    SSL_set_fd(ssl, sfd);

    const int status = SSL_connect(ssl);
    if (status != 1)
    {
        SSL_get_error(ssl, status);
        ERR_print_errors_fp(stderr); //High probability this doesn't do anything
        fprintf(stderr, "SSL_connect failed with SSL_get_error code %d\n", status);
        exit(EXIT_FAILURE);
    }

    printf("Connected with %s encryption\n", SSL_get_cipher(ssl));
    const char *chars = "Hello World, 123!";
    SSL_write(ssl, chars, strlen(chars));
    SSL_free(ssl);
    close(sfd);
    SSL_CTX_free(ctx);
    return 0;
}

Here's the same code in a Gist!

| improve this question | | | | |
\$\endgroup\$
  • 1
    \$\begingroup\$ It seems like a reasonable C solution but its not good C++. You need to study RAII make sure any resource allocation is exception safe. Unfortunately I cant write a review on C code as I don't know it that well. Adding the C tag to get C programmers to give it a look. \$\endgroup\$ – Martin York Oct 13 '18 at 7:15
9
\$\begingroup\$

C++ review

You still need a C review!

Idioms/Patterns

RAII idiom

In C++ we have this concept that an object should clean up its own resources. So when an object is created it will create and hold onto resources and when it is destroyed it will clean up those resources.

When learning about the idiom people mostly talk about memory and smart pointers. This is how we stop memory leaks in C++ that happen in C when things don't got as planned.

You should also note that an objects destructor is called when the control leaves the scope in which the object is created. However that scope is left. So even if you throw an exception that causes the stack to unwind as it unwinds the stack the destructors for all automatic objects are being called.

You need to apply this to your code.

 // this is an anti pattern in C++
 Stuff*   data = crate_data();

 ... do stuff with data
 ... do stuff with data

 destroy_data();

If you see this in your code you should be using RAII.

 class StuffHolder
 {
     Stuff*   data;

     public:
         StuffHolder()
             : data(crate_data())
         {}
         ~StuffHolder()
         {
              destroy_data(data);
         }
 };

 // Then in your code
 StuffHolder   dataHolder;

 ... do stuff with data
 ... do stuff with data

That's the pattern you should be using.

Exceptions

Exceptions make code easier to understand. Use them.

In your code you have places that you call exit(). Which is fine as it makes the rest of the code easy to read as you are not writing code to pass error message back through many layers of code so it keeps the code nice and simple.

There is a disadvantage in that you have exit locations scattered through your code. Have you been consistent with the error messages that are generated? You have to search and check every exit location.

A lot of code (especially bigger apps or libraries) don't want want to exit. They are part of a bigger framework so in C code you need to add error logic explicitly to the code to force the stack to unwind to the point where the user can take an action based on the error, or in a server you have a single place that is doing the error logging (so that it is consistent; otherwise each library will end up generating logs in a different way).

Exceptions provide the best of both worlds. You have a single place to catch and log exceptions. You don't have to write explicit error handling code (this is what exceptions do they unwind the code).

When you use exceptions with RAII all your resource are also correctly handled and released so you have no issues with releasing resources.

I can see several different ways of generating error messages in the code:

    ERR_print_errors_fp(stderr);
    exit(EXIT_FAILURE);

    perror(hostname);
    exit(EXIT_FAILURE);

    fprintf(stderr, "%s: %s\n", hostname, gai_strerror(status));
    exit(EXIT_FAILURE);

    SSL_get_error(ssl, status);
    ERR_print_errors_fp(stderr); //High probability this doesn't do anything
    fprintf(stderr, "SSL_connect failed with SSL_get_error code %d\n", status);
    exit(EXIT_FAILURE);

When you start having automated tools run over your logs this will be a real pain. Have a single point generate the error message so it is consistent for all places in the code. Then you can write tools that can easily analyze your error logs.

Code Review.

Would like to point out you don't use a single C++ header file.

#include <errno.h>
#include <unistd.h>
#include <string.h>
#include <resolv.h>
#include <netdb.h>
#include <openssl/ssl.h>
#include <openssl/err.h>

Like the comment (It provides useful information).

//Not sure what headers are needed or not
//This code (theoretically) writes "Hello World, 123" to a socket over a secure TLS connection
//compiled with g++ -Wall -o client.out client.cpp -L/usr/lib -lssl -lcrypto
//Based off of: https://www.cs.utah.edu/~swalton/listings/articles/ssl_client.c
//The OpenConnection method was heavily based on the answer to this post: https://stackoverflow.com/questions/52727565/client-in-c-use-gethostbyname-or-getaddrinfo

Returning pointers

Returning pointers in C++ is very uncommon.

SSL_CTX *InitSSL_CTX(void)

You usually return an object (or reference). If you have a resource it is contained within an object that you return. That way the resource is correctly handled and released. If you are returning a pointer to an object that is owned locally by the function/method then you would normally return a reference. This guarantees that the object exists (can never be nullptr) but it also an indication that you are not releasing ownership of the pointer.

Terrible comment.

    // Bad comment
    const SSL_METHOD *method = TLS_client_method(); /* Create new client-method 

    // Another terrible comment
    X509 *cert = SSL_get_peer_certificate(ssl); /* get the server's certificate */

Comments like this are useless. In fact they are worse than useless they are dangerous. The comment tells me no more than the function name so it is not providing any real information. Also in the future as this code is maintained and made better the next developer could change the function call but leave the comment. Then a third developer sees there is a discrepancy between code and comment. Now what does he do? Does he fix the comment? Does he fix the code?

The point is comments de-generate over time and become less useful unless you are very active in maintaining them. So write code that is self documenting. Add comments that is explain WHAT (you are trying to do) or WHY (you are trying to do it) don't write comments that explain HOW (you are doing it). The code explains how (there are exceptions to this rule but you will have people tell you when there is an exception and the code needs to be commented).

One variable declaration per line:

    struct addrinfo hints = {0}, *addrs;

One variable declaration per line. Also in C++ you don't need the struct. In C++ we normally (unlike C) put the * next to the type. Types are exceedingly important in C++ (much more than in C) so we like to know the exact type. To me the above line is hard to parse.

    addrinfo   hints = {0};
    addrinfo*  addrs;

That looks so much neater and I can see the exact type of each variable.

Dead Code

Remove dead code.
This function is not used anywhere. Delete it from your code.

void DisplayCerts(SSL *ssl)

Main is special

Don't need a return in main().

    return 0;

You can do it; but people use no return value to indicate that the app will never generate an error code. So if I see a return 0; at the end then I start looking through main to see if there are any return 1; in the code.

Now I will admit that I would also add return 0; because you have used exit() in your code. But If I had written the code I would not have an exit() and consequently probably not had an return 0;.

int main(int argc, char* argv[])
{
    try {
       std::vector<std::string>   args(argv, argv + argc);
       exampleClient(args);
    }
    catch(std::exception const& e)
    {
        std::cerr << "Exception: " << e.what() << "\n";
        throw;
    }
    catch(...)
    {
        std::cerr << "Unknown Error: Red Alert\n";
        throw;
    }
}

Issues

Not sure if this is an error but it looks like one.

SSL_CTX *ctx = InitSSL_CTX();
SSL *ssl = SSL_new(ctx);
const int sfd = OpenConnection("127.0.0.1", argv[1]);

...

SSL_free(ssl);
close(sfd);
SSL_CTX_free(ctx);

You are creating the resources in a different order to closing them. Seems very code smelly. I would expect resource to be released in the reverse order of creation to make sure there are no interdependencies.

Being Selfish

I have already written a wrapper for C++ clients to use sockets.

https://lokiastari.com/blog/2016/05/26/c-plus-plus-wrapper-for-socket/index.html

The source is here:

https://github.com/Loki-Astari/Examples/tree/master/Version2

Look at client.cpp to see how to use it look at Socket.h for the interface. Would be nice to have a SSL Implementation. It may be as simple as providing an implementation of Protocol.h ie SSLProtocol. Just saying. I would review that.

| improve this answer | | | | |
\$\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.