3
\$\begingroup\$

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 <log_entry.h>

#include <my_global.h>
#include <mysql.h>

#include <stdio.h>
#include <string.h>

#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 NULL,
  `source` varchar(32) NOT NULL,
  `time` time NOT NULL,
  `date` date NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB  DEFAULT CHARSET=latin1 AUTO_INCREMENT=18 ;

I limit the length to avoid any potential security risk, and save storage in my database. I don't really need any longer log messages.

Here is also the definition of my LOG_ENTRY struct:

typedef struct {
    int severity;
    char *event ;
    char *source;
    struct tm *tm_info;
} LOG_ENTRY;
\$\endgroup\$
  • 1
    \$\begingroup\$ This is a bit outside the scope of your code, but are you settled on MySQL? There are other (better) open source DBMS, in case you're interested just let me know. \$\endgroup\$ – Phrancis Feb 10 '15 at 23:50
  • \$\begingroup\$ @Phrancis I'm going to stick with MySQL because I have already written a lot of software which depends on it. \$\endgroup\$ – Linus Feb 10 '15 at 23:55
  • \$\begingroup\$ Any reason to not use MySQL C API? \$\endgroup\$ – vnp Feb 11 '15 at 0:19
  • \$\begingroup\$ @Linus OK was just curious. Not so familiar with the different things outside MySQL that can interact with it, just have used "bare" MySQL before and was pretty disappointed. Hope you get some good reviews! \$\endgroup\$ – Phrancis Feb 11 '15 at 1:41
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the good answer, I'm debating whether or not I should let the user specify the password and user or if I should encrypt it in a configuration file. If you are interested I've included the table structure, and the struct definition (see edit above). \$\endgroup\$ – Linus Feb 11 '15 at 10:58
  • \$\begingroup\$ If the estimated buffer size is calculated after the mysql_real_escape_string() and after the snprintf(), why would I need to calculate that? \$\endgroup\$ – Linus Feb 11 '15 at 13:01
  • \$\begingroup\$ The buffer has to be allocated before all of that. We can put a theoretical upper bound on the buffer size. If we can determine the size at compile time, that simplifies the code. \$\endgroup\$ – 200_success Feb 11 '15 at 13:07
  • \$\begingroup\$ Okay, I get 589 as my estimated needed buffer size, that seems like plenty enough for mysql_real_escape_string(). \$\endgroup\$ – Linus Feb 11 '15 at 13:34
2
\$\begingroup\$

ltdb is not a good name for a function, pick a name along the lines of {verb}{noun} LogToDatabase to make it more clear what the function does.


The function should check the argument ent if it is NULL to avoid illegal deref.


When you declare variables it is good to place them one per line and initialize them. If your C compiler allows it declare the variables where they are used and not at the top of the function.


The SQL string is dangerous as it is, I am no expert of MySQL but most database allow for precompiling a statement and then just supplying the arguments in the code. That would also speed up execution.


ltdb returns the same value regardless of what error occurs in the function, it is better to create an enum with various error codes so that the caller of the function can determine what went wrong and give a suitable error message. In the same manner remove the fprint-out in the function, let the caller worry about showing an error message based on the error code.


Having user/pwd in code, especially in clear text is a bad idea. If you still want them in the code at least encrypt them in some way. As they currently stand anybody can with a hexdump program read them.


It seems unnecessary to have the NELEMS macro when you could have just have written a define with the actual size

#define DATETIME_LENGTH 16
...
strftime(time, DATETIME_LENGTH, "%H:%M:%S", ent->tm_info);

or even better (IMHO) skipping the define altogether

strftime(time, sizeof(time), "%H:%M:%S", ent->tm_info);
\$\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.