patterncMinor
IRC Client with TLS and SASL support
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:
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
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
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
Memory Management
Using pointers doesn't force you to dynamically allocate memory. In you main function, you declare
Code Duplication
Every time you call
Code Division
Most of your connection logic is in
The
Running the Application
Starting the application, it writes an ambiguous
Memory is also leaked:
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 blocksCode 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 blocksContext
StackExchange Code Review Q#156859, answer score: 3
Revisions (0)
No revisions yet.