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

IRC Client with TLS and SASL support

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

Problem

I wrote this little program to... I don't know. But it connects to IRC servers using OpenSSL and can login via SASL. The code needs refactoring, of that I'm certain, but I need some advice in that regard, so please roast my code.

tallis.h:

#ifndef TALLIS_H
#define TALLIS_H
#include 
#include 
#include 
typedef struct tallis_config_settings_struct
{
    config_t config;
    int has_config, has_nick, has_sasl, has_sasl_password;
} settings_t;

typedef struct tallis_bot_struct
{
    int sfd;
    BIO *bio;
    SSL *ssl_connection;
    SSL_CTX *ssl_context;
    X509_VERIFY_PARAM *param;
    char *remote_host, *remote_port, *nick,
         *nethost, *domain, *sasl_challenge;
    const char *sasl_password;
    size_t sasl_challenge_len;
    settings_t settings;
} tallis_t;
void tallis_shutdown(tallis_t*);
#endif /* TALLIS_H */


tallis.c:

```
#include
#include
#include
#include
#include
#include
#include "tallis.h"
#include "net.h"
#include "conf.h"
#include "error.h"

int main(int argc, char *argv[])
{
int rv;

tallis_t *tallis = malloc(sizeof(tallis_t));
tallis->nick = "tallis";
tallis->remote_host = "irc.freenode.net";
tallis->remote_port = "6697";
tallis->nethost = "eleison";
tallis->domain = "vatican.va";
tallis->sasl_password = NULL;
tallis->sasl_challenge_len = 0;
tallis->bio = NULL;
tallis->ssl_connection = NULL;
tallis_ssl_init();
tallis->ssl_context = SSL_CTX_new(TLS_method());
tallis->ssl_connection = SSL_new(tallis->ssl_context);
tallis->param = SSL_get0_param(tallis->ssl_connection);

char *tallis_conf_path = NULL;
tallis_conf_path = tallis_set_conf_path();

if (tallis_conf_path != NULL)
{
rv = tallis_parse_config(&tallis->settings.config, tallis_conf_path);
free(tallis_conf_path);

if (rv != CONFIG_TRUE)
tallis_config_fail(tallis);
else if (rv == CONFIG_TRUE)
tallis->settings.has_config = 1;
}
else if (tal

Solution

Parsing Incoming Messages

After calling BIO_read on a 512 byte buffer, you simply set buf[bytes_read] = '\0' (which overflows if you just happen to have read exactly 512 bytes) and start parsing the buffer.

However, you have no guarantee that your buffer contains one and only one IRC message. It may contain only the start of a message, multiple full messages or even a few full messages and a partial one at the end. You need to buffer the input and then search for newlines which IRC uses to separate messages.

Compiler Warnings

The compiler shows various warnings when compiling the code. These should be fixed

net.c:69:9: warning: format not a string literal and no format arguments [-Wformat-security]
    fprintf(stderr, ERR_error_string(ERR_get_error(), NULL));

net.c:176:9: warning: unused variable ‘retry_limit’ [-Wunused-variable]
    int retry_limit = 2;

tallis.c:27:39: warning: implicit declaration of function ‘TLS_method’ [-Wimplicit-function-declaration]
    tallis->ssl_context = SSL_CTX_new(TLS_method());


Memory Management

Using pointers doesn't force you to dynamically allocate memory. In you main function, you declare tallis_t *tallis = malloc(sizeof(tallis_t)); only to pass the pointer around and free it later. As it doesn't consume much memory, you can simply allocate it on the stack.

Code Duplication

Every time you call tallis_send, you invoke strlen on the line just before. If you know you'll pass null-terminated strings, make tallis_send or a helper function call strlen every time (perhaps make it even add the newline delimiter).

Code Division

Most of your connection logic is in net.c, but one function seems to have been misplaced inside irc.c. I would except net.c to deal with the communication infrastructure and let irc.c handle all the parsing/generation of IRC specific messages.

The main function also contains a lot of code. It's also confusing to find a lot of hard-coded values, when there is also code that deals with loading a configuration file.

tallis_set_conf_path contains the verb set but it actually returns a value. The memory allocated for the return value is never freed.

Running the Application

Starting the application, it writes an ambiguous No such file or directory and then crashes in tallis_check_sasl as no config file is loaded, yet the application fails to check for this on all branches.

Memory is also leaked:

==5027== HEAP SUMMARY:
==5027==     in use at exit: 98,388 bytes in 376 blocks
==5027==   total heap usage: 739 allocs, 363 frees, 174,824 bytes allocated
==5027==
==5027== 3,744 (824 direct, 2,920 indirect) bytes in 1 blocks are definitely lost in loss record 297 of 303
==5027==    at 0x4C2CB3F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5027==    by 0x5104E87: CRYPTO_malloc (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==5027==    by 0x4E790AF: SSL_new (in /lib/x86_64-linux-gnu/libssl.so.1.0.0)
==5027==    by 0x10ACD5: main (tallis.c:28)
==5027==
==5027== 4,096 bytes in 1 blocks are definitely lost in loss record 300 of 303
==5027==    at 0x4C2CB3F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5027==    by 0x4041AB1: ???
==5027==    by 0x4054D1B: ???
==5027==    by 0x4063A30: ???
==5027==    by 0x403ED12: ???
==5027==    by 0x40648DD: ???
==5027==    by 0x580DA47: gethostbyname_r@@GLIBC_2.2.5 (getXXbyYY_r.c:315)
==5027==    by 0x580D01E: gethostbyname (getXXbyYY.c:116)
==5027==    by 0x51B8701: BIO_get_host_ip (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==5027==    by 0x51B4B33: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==5027==    by 0x51B508F: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==5027==    by 0x51B294A: BIO_write (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==5027==
==5027== LEAK SUMMARY:
==5027==    definitely lost: 4,920 bytes in 2 blocks
==5027==    indirectly lost: 2,920 bytes in 8 blocks
==5027==      possibly lost: 0 bytes in 0 blocks
==5027==    still reachable: 90,548 bytes in 366 blocks
==5027==         suppressed: 0 bytes in 0 blocks

Code Snippets

net.c:69:9: warning: format not a string literal and no format arguments [-Wformat-security]
    fprintf(stderr, ERR_error_string(ERR_get_error(), NULL));

net.c:176:9: warning: unused variable ‘retry_limit’ [-Wunused-variable]
    int retry_limit = 2;

tallis.c:27:39: warning: implicit declaration of function ‘TLS_method’ [-Wimplicit-function-declaration]
    tallis->ssl_context = SSL_CTX_new(TLS_method());
==5027== HEAP SUMMARY:
==5027==     in use at exit: 98,388 bytes in 376 blocks
==5027==   total heap usage: 739 allocs, 363 frees, 174,824 bytes allocated
==5027==
==5027== 3,744 (824 direct, 2,920 indirect) bytes in 1 blocks are definitely lost in loss record 297 of 303
==5027==    at 0x4C2CB3F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5027==    by 0x5104E87: CRYPTO_malloc (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==5027==    by 0x4E790AF: SSL_new (in /lib/x86_64-linux-gnu/libssl.so.1.0.0)
==5027==    by 0x10ACD5: main (tallis.c:28)
==5027==
==5027== 4,096 bytes in 1 blocks are definitely lost in loss record 300 of 303
==5027==    at 0x4C2CB3F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5027==    by 0x4041AB1: ???
==5027==    by 0x4054D1B: ???
==5027==    by 0x4063A30: ???
==5027==    by 0x403ED12: ???
==5027==    by 0x40648DD: ???
==5027==    by 0x580DA47: gethostbyname_r@@GLIBC_2.2.5 (getXXbyYY_r.c:315)
==5027==    by 0x580D01E: gethostbyname (getXXbyYY.c:116)
==5027==    by 0x51B8701: BIO_get_host_ip (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==5027==    by 0x51B4B33: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==5027==    by 0x51B508F: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==5027==    by 0x51B294A: BIO_write (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==5027==
==5027== LEAK SUMMARY:
==5027==    definitely lost: 4,920 bytes in 2 blocks
==5027==    indirectly lost: 2,920 bytes in 8 blocks
==5027==      possibly lost: 0 bytes in 0 blocks
==5027==    still reachable: 90,548 bytes in 366 blocks
==5027==         suppressed: 0 bytes in 0 blocks

Context

StackExchange Code Review Q#156859, answer score: 3

Revisions (0)

No revisions yet.