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