Hello,
Here are updated patches for rrdtool/rrd_client.c and
rrdtool/rrd_daemon.c. We had some discussions in August last year. I
created a branch and the diffs are against latest git. I've run the
code, especially rrd_daemon with valgrind under Linux, but need some
help to check that also rrd_client works OK (maybe rrd_daemon too). Can
you help me with test cases to run the execute the code modified paths.
I know one application using rrdtool, lm-sensors (build-dependency on
librrd2-dev), but am not sure my computers have the sensors to be a good
test case.
Note that the modified functions get_path() and get_abs_path() are
static, so they don't change the API.
Thanks,
Svante Signell
--- a/src/rrd_daemon.c
+++ b/src/rrd_daemon.c
@@ -125,7 +125,7 @@ typedef enum { RESP_ERR = -1, RESP_OK =
struct listen_socket_s
{
int fd;
- char addr[PATH_MAX + 1];
+ char *addr;
int family;
/* state for BATCH processing */
@@ -1159,23 +1159,28 @@ err:
} /* }}} static int check_file_access */
/* when using a base dir, convert relative paths to absolute paths.
- * if necessary, modifies the "filename" pointer to point
- * to the new path created in "tmp". "tmp" is provided
- * by the caller and sizeof(tmp) must be >= PATH_MAX.
+ * if necessary, modify the "filename" pointer to point
+ * to the new path created in "tmp". "tmp" is malloced
+ * and freed by the called function.
*
+ * FIXME: Check this!
* this allows us to optimize for the expected case (absolute path)
* with a no-op.
*/
-static void get_abs_path(char **filename, char *tmp)
+static void get_abs_path(char **filename)
{
- assert(tmp != NULL);
+ char *tmp = NULL;
+ int len = 0;
assert(filename != NULL && *filename != NULL);
if (config_base_dir == NULL || **filename == '/')
return;
- snprintf(tmp, PATH_MAX, "%s/%s", config_base_dir, *filename);
+ len = strlen(config_base_dir) + 1 + strlen(*filename) + 1;
+ tmp = malloc(len);
+ snprintf(tmp, len, "%s/%s", config_base_dir, *filename);
*filename = tmp;
+ free(tmp);
} /* }}} static int get_abs_path */
static int flush_file (const char *filename) /* {{{ */
@@ -1266,7 +1271,7 @@ static int handle_request_stats (HANDLER
static int handle_request_flush (HANDLER_PROTO) /* {{{ */
{
- char *file, file_tmp[PATH_MAX];
+ char *file;
int status;
status = buffer_get_field (&buffer, &buffer_size, &file);
@@ -1280,7 +1285,7 @@ static int handle_request_flush (HANDLER
stats_flush_received++;
pthread_mutex_unlock(&stats_lock);
- get_abs_path(&file, file_tmp);
+ get_abs_path(&file);
if (!check_file_access(file, sock)) return 0;
status = flush_file (file);
@@ -1321,14 +1326,14 @@ static int handle_request_flushall(HANDL
static int handle_request_pending(HANDLER_PROTO) /* {{{ */
{
int status;
- char *file, file_tmp[PATH_MAX];
+ char *file;
cache_item_t *ci;
status = buffer_get_field(&buffer, &buffer_size, &file);
if (status != 0)
return syntax_error(sock,cmd);
- get_abs_path(&file, file_tmp);
+ get_abs_path(&file);
pthread_mutex_lock(&cache_lock);
ci = g_tree_lookup(cache_tree, file);
@@ -1349,13 +1354,13 @@ static int handle_request_forget(HANDLER
{
int status;
gboolean found;
- char *file, file_tmp[PATH_MAX];
+ char *file;
status = buffer_get_field(&buffer, &buffer_size, &file);
if (status != 0)
return syntax_error(sock,cmd);
- get_abs_path(&file, file_tmp);
+ get_abs_path(&file);
if (!check_file_access(file, sock)) return 0;
pthread_mutex_lock(&cache_lock);
@@ -1396,7 +1401,7 @@ static int handle_request_queue (HANDLER
static int handle_request_update (HANDLER_PROTO) /* {{{ */
{
- char *file, file_tmp[PATH_MAX];
+ char *file;
int values_num = 0;
int status;
char orig_buf[RRD_CMD_MAX];
@@ -1415,7 +1420,7 @@ static int handle_request_update (HANDLE
stats_updates_received++;
pthread_mutex_unlock(&stats_lock);
- get_abs_path(&file, file_tmp);
+ get_abs_path(&file);
if (!check_file_access(file, sock)) return 0;
pthread_mutex_lock (&cache_lock);
@@ -1558,7 +1563,7 @@ static int handle_request_update (HANDLE
static int handle_request_fetch (HANDLER_PROTO) /* {{{ */
{
- char *file, file_tmp[PATH_MAX];
+ char *file;
char *cf;
char *start_str;
@@ -1612,7 +1617,7 @@ static int handle_request_fetch (HANDLER
if (status != 0)
return (syntax_error(sock,cmd));
- get_abs_path(&file, file_tmp);
+ get_abs_path(&file);
if (!check_file_access(file, sock)) return 0;
status = flush_file (file);
@@ -1776,7 +1781,7 @@ static int handle_request_wrote (HANDLER
static int handle_request_info (HANDLER_PROTO) /* {{{ */
{
- char *file, file_tmp[PATH_MAX];
+ char *file;
int status;
rrd_info_t *info;
@@ -1785,7 +1790,7 @@ static int handle_request_info (HANDLER_
if (status != 0)
return syntax_error(sock,cmd);
/* get full pathname */
- get_abs_path(&file, file_tmp);
+ get_abs_path(&file);
if (!check_file_access(file, sock)) {
return send_response(sock, RESP_ERR, "Cannot read: %s\n", file);
}
@@ -1825,7 +1830,7 @@ static int handle_request_info (HANDLER_
static int handle_request_first (HANDLER_PROTO) /* {{{ */
{
- char *i, *file, file_tmp[PATH_MAX];
+ char *i, *file;
int status;
int idx;
time_t t;
@@ -1835,7 +1840,7 @@ static int handle_request_first (HANDLER
if (status != 0)
return syntax_error(sock,cmd);
/* get full pathname */
- get_abs_path(&file, file_tmp);
+ get_abs_path(&file);
if (!check_file_access(file, sock)) {
return send_response(sock, RESP_ERR, "Cannot read: %s\n", file);
}
@@ -1860,7 +1865,7 @@ static int handle_request_first (HANDLER
static int handle_request_last (HANDLER_PROTO) /* {{{ */
{
- char *file, file_tmp[PATH_MAX];
+ char *file;
int status;
time_t t, from_file, step;
rrd_file_t * rrd_file;
@@ -1872,7 +1877,7 @@ static int handle_request_last (HANDLER_
if (status != 0)
return syntax_error(sock,cmd);
/* get full pathname */
- get_abs_path(&file, file_tmp);
+ get_abs_path(&file);
if (!check_file_access(file, sock)) {
return send_response(sock, RESP_ERR, "Cannot read: %s\n", file);
}
@@ -1902,8 +1907,8 @@ static int handle_request_last (HANDLER_
static int handle_request_create (HANDLER_PROTO) /* {{{ */
{
- char *file, file_tmp[PATH_MAX];
- char *file_copy, *dir, *dir_tmp[PATH_MAX];
+ char *file;
+ char *file_copy, *dir, *dir_tmp;
struct stat st;
char *tok;
int ac = 0;
@@ -1919,7 +1924,7 @@ static int handle_request_create (HANDLE
if (status != 0)
return syntax_error(sock,cmd);
/* get full pathname */
- get_abs_path(&file, file_tmp);
+ get_abs_path(&file);
file_copy = strdup(file);
if (file_copy == NULL) {
@@ -1932,7 +1937,8 @@ static int handle_request_create (HANDLE
pthread_mutex_lock(&rrdfilecreate_lock);
dir = dirname(file_copy);
- if (realpath(dir, dir_tmp) == NULL && errno == ENOENT) {
+ dir_tmp = realpath(dir, NULL);
+ if (dir_tmp == NULL && errno == ENOENT) {
if (!config_allow_recursive_mkdir) {
return send_response(sock, RESP_ERR,
"No permission to recursively create: %s\nDid you pass -R to the daemon?\n",
@@ -2401,8 +2407,8 @@ static void journal_close(void) /* {{{ *
static void journal_new_file(void) /* {{{ */
{
struct timeval now;
- int new_fd;
- char new_file[PATH_MAX + 1];
+ int new_fd, len;
+ char *new_file = NULL;
assert(journal_dir != NULL);
assert(journal_cur != NULL);
@@ -2411,7 +2417,9 @@ static void journal_new_file(void) /* {{
gettimeofday(&now, NULL);
/* this format assures that the files sort in strcmp() order */
- snprintf(new_file, PATH_MAX, "%s/%s.%010d.%06d",
+ len = strlen(journal_dir) + 1 + strlen(JOURNAL_BASE) + 10 + 6 + 1;
+ new_file = malloc(len);
+ snprintf(new_file, len, "%s/%s.%010d.%06d",
journal_dir, JOURNAL_BASE, (int)now.tv_sec, (int)now.tv_usec);
new_fd = open(new_file, O_WRONLY|O_CREAT|O_APPEND,
@@ -2429,6 +2437,7 @@ static void journal_new_file(void) /* {{
/* record the file in the journal set */
rrd_add_strdup(&journal_cur->files, &journal_cur->files_num, new_file);
+ free(new_file);
return;
error:
@@ -2438,6 +2447,7 @@ error:
RRDD_LOG(LOG_CRIT,
"JOURNALING DISABLED: All values will be flushed at shutdown");
+ free(new_file);
close(new_fd);
config_flush_at_shutdown = 1;
@@ -2500,7 +2510,6 @@ static void journal_done(void) /* {{{ */
journal_set_free(journal_cur);
journal_set_free(journal_old);
- free(journal_dir);
} /* }}} static void journal_done */
@@ -2634,10 +2643,10 @@ static int journal_sort(const void *v1,
static void journal_init(void) /* {{{ */
{
- int had_journal = 0;
+ int had_journal = 0, len = 0;
DIR *dir;
struct dirent *dent;
- char path[PATH_MAX+1];
+ char *path = NULL;
if (journal_dir == NULL) return;
@@ -2656,19 +2665,27 @@ static void journal_init(void) /* {{{ */
* correct sort order. TODO: remove after first release
*/
{
- char old_path[PATH_MAX+1];
- snprintf(old_path, PATH_MAX, "%s/%s", journal_dir, JOURNAL_BASE ".old" );
- snprintf(path, PATH_MAX, "%s/%s", journal_dir, JOURNAL_BASE ".0000");
+ char *old_path = NULL;
+ if (journal_dir != NULL) {
+ len = strlen(journal_dir) + 1 + strlen(JOURNAL_BASE) + 5 + 1;
+ old_path = malloc(len);
+ path = malloc(len);
+ snprintf(old_path, len, "%s/%s", journal_dir, JOURNAL_BASE ".old" );
+ snprintf(path, len, "%s/%s", journal_dir, JOURNAL_BASE ".0000");
rename(old_path, path);
- snprintf(old_path, PATH_MAX, "%s/%s", journal_dir, JOURNAL_BASE );
- snprintf(path, PATH_MAX, "%s/%s", journal_dir, JOURNAL_BASE ".0001");
+ snprintf(old_path, len, "%s/%s", journal_dir, JOURNAL_BASE );
+ snprintf(path, len, "%s/%s", journal_dir, JOURNAL_BASE ".0001");
rename(old_path, path);
+ free(old_path);
+ }
}
+ /* FIXME: Check all free(path) !! */
dir = opendir(journal_dir);
if (!dir) {
RRDD_LOG(LOG_CRIT, "journal_init: opendir(%s) failed\n", journal_dir);
+ free(path);
return;
}
while ((dent = readdir(dir)) != NULL)
@@ -2677,15 +2694,20 @@ static void journal_init(void) /* {{{ */
if (strncmp(dent->d_name, JOURNAL_BASE, strlen(JOURNAL_BASE)))
continue;
- snprintf(path, PATH_MAX, "%s/%s", journal_dir, dent->d_name);
+ len = strlen(journal_dir) + 1 + strlen(dent->d_name) + 1;
+ path = realloc(path, len);
+ snprintf(path, len, "%s/%s", journal_dir, dent->d_name);
if (!rrd_add_strdup(&journal_cur->files, &journal_cur->files_num, path))
{
RRDD_LOG(LOG_CRIT, "journal_init: cannot add journal file %s!",
dent->d_name);
+ free(path);
break;
}
+ free(path);
}
+ free(path);
closedir(dir);
qsort(journal_cur->files, journal_cur->files_num,
@@ -2712,6 +2734,7 @@ static void free_listen_socket(listen_so
free(sock->rbuf); sock->rbuf = NULL;
free(sock->wbuf); sock->wbuf = NULL;
+ free(sock->addr); sock->addr = NULL;
free(sock);
} /* }}} void free_listen_socket */
@@ -2841,6 +2864,8 @@ out_close:
static int open_listen_socket_unix (const listen_socket_t *sock) /* {{{ */
{
+ // FIXME
+ //int fd, len;
int fd;
struct sockaddr_un sa;
listen_socket_t *temp;
@@ -2868,8 +2893,8 @@ static int open_listen_socket_unix (cons
dir, rrd_strerror(errno));
return (-1);
}
-
- free(path_copy);
+ // FIXME
+ //free(path_copy);
temp = (listen_socket_t *) rrd_realloc (listen_fds,
sizeof (listen_fds[0]) * (listen_fds_num + 1));
@@ -2937,8 +2962,11 @@ static int open_listen_socket_unix (cons
listen_fds[listen_fds_num].fd = fd;
listen_fds[listen_fds_num].family = PF_UNIX;
- strncpy(listen_fds[listen_fds_num].addr, path,
- sizeof (listen_fds[listen_fds_num].addr) - 1);
+ // FIXME
+ //len = strlen(path) + 1;
+ //listen_fds[listen_fds_num].addr = malloc(len);
+ strcpy(listen_fds[listen_fds_num].addr, path_copy);
+ free(path_copy);
listen_fds_num++;
return (0);
@@ -3152,9 +3180,9 @@ static void open_listen_sockets_traditio
}
else
{
- strncpy(default_socket.addr, RRDCACHED_DEFAULT_ADDRESS,
- sizeof(default_socket.addr) - 1);
- default_socket.addr[sizeof(default_socket.addr) - 1] = '\0';
+ int len = strlen(RRDCACHED_DEFAULT_ADDRESS) + 1;
+ default_socket.addr = malloc(len);
+ strcpy(default_socket.addr, RRDCACHED_DEFAULT_ADDRESS);
if (default_socket.permissions == 0)
socket_permission_set_all (&default_socket);
@@ -3173,6 +3201,8 @@ static int close_listen_sockets (void) /
if (listen_fds[i].family == PF_UNIX)
unlink(listen_fds[i].addr);
+ // FIXME
+ free (listen_fds[i].addr);
}
free (listen_fds);
@@ -3394,6 +3424,9 @@ static int cleanup (void) /* {{{ */
free(queue_threads);
free(config_base_dir);
+ // FIXME: Put at the right place
+ // see free_listen_sockets(listen_fds):
+ //free(default_socket.addr);
pthread_mutex_lock(&cache_lock);
g_tree_destroy(cache_tree);
@@ -3434,7 +3467,9 @@ static int read_options (int argc, char
case 'l':
{
+ // FIXME: struct member being malloced
listen_socket_t *new;
+ int len;
new = malloc(sizeof(listen_socket_t));
if (new == NULL)
@@ -3444,7 +3479,15 @@ static int read_options (int argc, char
}
memset(new, 0, sizeof(listen_socket_t));
- strncpy(new->addr, optarg, sizeof(new->addr)-1);
+ len = strlen(optarg) + 1;
+ // FIXME: Find out where to free new->addr and new
+ new->addr = malloc(len);
+ if (new->addr == NULL)
+ {
+ fprintf(stderr, "read_options: malloc failed.\n");
+ return(2);
+ }
+ strcpy(new->addr, optarg);
/* Add permissions to the socket {{{ */
if (default_socket.permissions != 0)
@@ -3465,8 +3508,15 @@ static int read_options (int argc, char
&config_listen_address_list_len, new))
{
fprintf(stderr, "read_options: rrd_add_ptr failed.\n");
+ // FIXME
+ //free(new->addr);
+ //free(new);
return (2);
}
+ // FIXME: This removes unix:... in open_listen_socket_unix
+ // FIXME: This makes vagrind unhappy
+ //free(new->addr);
+ //free(new);
}
break;
@@ -3618,7 +3668,7 @@ static int read_options (int argc, char
case 'b':
{
size_t len;
- char base_realpath[PATH_MAX];
+ char *base_realpath = NULL;
if (config_base_dir != NULL)
free (config_base_dir);
@@ -3641,7 +3691,7 @@ static int read_options (int argc, char
* assumptions possible (we don't have to resolve paths
* that start with a "/")
*/
- if (realpath(config_base_dir, base_realpath) == NULL)
+ if ((base_realpath = realpath(config_base_dir, NULL)) == NULL)
{
fprintf (stderr, "Failed to canonicalize the base directory '%s': "
"%s\n", config_base_dir, rrd_strerror(errno));
@@ -3671,15 +3721,17 @@ static int read_options (int argc, char
}
if (strncmp(config_base_dir,
- base_realpath, sizeof(base_realpath)) != 0)
+ base_realpath, len) != 0)
{
fprintf(stderr,
"Base directory (-b) resolved via file system links!\n"
"Please consult rrdcached '-b' documentation!\n"
"Consider specifying the real directory (%s)\n",
base_realpath);
+ free(base_realpath);
return 5;
}
+ free(base_realpath);
}
break;
@@ -3702,8 +3754,7 @@ static int read_options (int argc, char
case 'j':
{
- char journal_dir_actual[PATH_MAX];
- journal_dir = realpath((const char *)optarg, journal_dir_actual);
+ journal_dir = realpath((const char *)optarg, NULL);
if (journal_dir)
{
// if we were able to properly resolve the path, lets have a copy
@@ -3714,17 +3765,20 @@ static int read_options (int argc, char
{
fprintf(stderr, "Failed to create journal directory '%s': %s\n",
journal_dir, rrd_strerror(errno));
+ free(journal_dir);
return 6;
}
if (access(journal_dir, R_OK|W_OK|X_OK) != 0)
{
fprintf(stderr, "Must specify a writable directory with -j! (%s)\n",
errno ? rrd_strerror(errno) : "");
+ free(journal_dir);
return 6;
}
} else {
fprintf(stderr, "Unable to resolve journal path (%s,%s)\n", optarg,
errno ? rrd_strerror(errno) : "");
+ free(journal_dir);
return 6;
}
}
--- a/src/rrd_client.c
+++ b/src/rrd_client.c
@@ -81,12 +81,12 @@ static size_t inbuf_used = 0;
* are not allowed, since path name translation is done by the server.
*
* One must hold `lock' when calling this function. */
-static const char *get_path (const char *path, char *resolved_path) /* {{{ */
+static const char *get_path (const char *path) /* {{{ */
{
const char *ret = path;
int is_unix = 0;
- if ((path == NULL) || (resolved_path == NULL) || (sd_path == NULL))
+ if ((path == NULL) || (sd_path == NULL))
return (NULL);
if ((*sd_path == '/')
@@ -95,7 +95,7 @@ static const char *get_path (const char
if (is_unix)
{
- ret = realpath(path, resolved_path);
+ ret = realpath(path, NULL);
if (ret == NULL)
rrd_set_error("realpath(%s): %s", path, rrd_strerror(errno));
return ret;
@@ -840,7 +840,6 @@ int rrdc_update (const char *filename, i
rrdc_response_t *res;
int status;
int i;
- char file_path[PATH_MAX];
memset (buffer, 0, sizeof (buffer));
buffer_ptr = &buffer[0];
@@ -851,7 +850,7 @@ int rrdc_update (const char *filename, i
return (ENOBUFS);
mutex_lock (&lock);
- filename = get_path (filename, file_path);
+ filename = get_path (filename);
if (filename == NULL)
{
mutex_unlock (&lock);
@@ -902,7 +901,6 @@ static int rrdc_filebased_command (const
size_t buffer_size;
rrdc_response_t *res;
int status;
- char file_path[PATH_MAX];
if (filename == NULL)
return (-1);
@@ -916,7 +914,7 @@ static int rrdc_filebased_command (const
return (ENOBUFS);
mutex_lock (&lock);
- filename = get_path (filename, file_path);
+ filename = get_path (filename);
if (filename == NULL)
{
mutex_unlock (&lock);
@@ -964,7 +962,6 @@ rrd_info_t * rrdc_info (const char *file
size_t buffer_size;
rrdc_response_t *res;
int status;
- char file_path[PATH_MAX];
rrd_info_t *data = NULL, *cd;
rrd_infoval_t info;
unsigned int l;
@@ -987,7 +984,7 @@ rrd_info_t * rrdc_info (const char *file
}
mutex_lock (&lock);
- filename = get_path (filename, file_path);
+ filename = get_path (filename);
if (filename == NULL)
{
mutex_unlock (&lock);
@@ -1064,7 +1061,6 @@ time_t rrdc_last (const char *filename)
size_t buffer_size;
rrdc_response_t *res;
int status;
- char file_path[PATH_MAX];
time_t lastup;
if (filename == NULL) {
@@ -1083,7 +1079,7 @@ time_t rrdc_last (const char *filename)
}
mutex_lock (&lock);
- filename = get_path (filename, file_path);
+ filename = get_path (filename);
if (filename == NULL)
{
mutex_unlock (&lock);
@@ -1125,7 +1121,6 @@ time_t rrdc_first (const char *filename,
size_t buffer_size;
rrdc_response_t *res;
int status;
- char file_path[PATH_MAX];
time_t firstup;
if (filename == NULL) {
@@ -1144,7 +1139,7 @@ time_t rrdc_first (const char *filename,
}
mutex_lock (&lock);
- filename = get_path (filename, file_path);
+ filename = get_path (filename);
if (filename == NULL)
{
mutex_unlock (&lock);
@@ -1198,7 +1193,6 @@ int rrdc_create (const char *filename, /
size_t buffer_size;
rrdc_response_t *res;
int status;
- char file_path[PATH_MAX];
int i;
if (filename == NULL) {
@@ -1217,7 +1211,7 @@ int rrdc_create (const char *filename, /
}
mutex_lock (&lock);
- filename = get_path (filename, file_path);
+ filename = get_path (filename);
if (filename == NULL)
{
mutex_unlock (&lock);
@@ -1282,7 +1276,6 @@ int rrdc_fetch (const char *filename, /*
size_t buffer_free;
size_t buffer_size;
rrdc_response_t *res;
- char path_buffer[PATH_MAX];
const char *path_ptr;
char *str_tmp;
@@ -1315,7 +1308,7 @@ int rrdc_fetch (const char *filename, /*
return (ENOBUFS);
/* change to path for rrdcached */
- path_ptr = get_path (filename, path_buffer);
+ path_ptr = get_path (filename);
if (path_ptr == NULL)
return (EINVAL);
_______________________________________________
rrd-developers mailing list
rrd-developers@lists.oetiker.ch
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
