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

Small library for logging to MySQL

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

Problem

I've thrown together a utility for my programs to log to a database, but the code looks like a mess and I don't know how to clean it up. This library is going to be used from all of my other programs and it is therefore important to keep the code clean and secure.

#include 

#include 
#include 

#include 
#include 

#define STR_BUFFER_SIZE 256

#define NELEMS(x)  (sizeof(x) / sizeof(x[0]))

void finish_with_error (MYSQL *);

int ltdb (LOG_ENTRY *ent)
{
    MYSQL *con = NULL;
    char time[16], date[16];
    char query[STR_BUFFER_SIZE];
    int len;

    con = mysql_init(NULL);
    if(NULL == con){
        fprintf(stderr, "%s\n", mysql_error(con));
        return 1;
    }

    if(NULL == mysql_real_connect(con, "localhost", "user", "password", "database", 0, NULL, 0)){
        finish_with_error(con);
        return 1;
    }

    strftime(time, NELEMS(time), "%H:%M:%S", ent->tm_info);
    strftime(date, NELEMS(date), "%Y-%m-%d", ent->tm_info);

    len = snprintf(query, STR_BUFFER_SIZE,
    "INSERT INTO `logg` ("
    "   severity,event,source,time,date"
    ") VALUES ("
    "   '%d', '%s', '%s', '%s', '%s'"
    ")",    ent->severity,
            ent->event, 
            ent->source,
            time,
            date);

    if(len >= STR_BUFFER_SIZE){
        fprintf(stderr, "MySQL query too long! (> %d)\n", STR_BUFFER_SIZE);
        return 1;
    }

    if(mysql_query(con, query)){
        finish_with_error(con);
        return 1;
    }
    mysql_close(con);

    return 0;
}

void finish_with_error(MYSQL *con)
{
    fprintf(stderr, "%s\n", mysql_error(con));
    mysql_close(con);
}


I'm trying to achieve a fast and simple way to process my log entries, but I'm not really happy with the code structure in general. Any suggestions for improvements would be much appreciated.

Here is the table structure of logg

``
CREATE TABLE IF NOT EXISTS
logg (
id int(10) unsigned NOT NULL AUTO_INCREMENT,
severity tinyint(4) NOT NULL,
event` varchar(128) NOT

Solution

Assuming that LOG_ENTRY.event can have arbitrary string content, your code is vulnerable to an SQL injection attack. You must escape strings in queries, and not copy them blindly into the query.

The 256-byte limit for the query buffer seems artificially limiting. I would expect your library to do whatever it takes to compose and execute the right SQL query without bungling it with an undersized buffer. One way to do that without malloc() or variable-length arrays is to define the struct to match the table definition.

typedef struct {
    int severity;
    char event[129]; /* 128 characters plus NUL. Latin1 means 1 byte per character */
    char source[33];
    struct tm tm_info;
} LogEntry;


The buffer would need to be large enough to accommodate the fixed text and the substituted strings, keeping in mind that mysql_real_escape_string() can double the number of bytes. (I would use sizeof(FIXED_QUERY) + 2 sizeof(LogEntry) + 3 sizeof(int) + 20 as a generous estimate of the needed buffer size.) Out of paranoia for unterminated strings, I'd use placeholders like "%.128s" for snprintf().

ltdb is such a cryptic function name. log_to_database() would be much better.

Requiring the program to be recompiled to change the password is not proper. The caller should have to make a call to initialize the library with the database connection parameters. Another approach, which is still sloppy but slightly better, is for your library to read the connection parameters from a configuration file.

Code Snippets

typedef struct {
    int severity;
    char event[129]; /* 128 characters plus NUL. Latin1 means 1 byte per character */
    char source[33];
    struct tm tm_info;
} LogEntry;

Context

StackExchange Code Review Q#80210, answer score: 3

Revisions (0)

No revisions yet.