rrdtool: FTBFS on hurd-386 (for review)
Source: rrdtool
Version: 1.4.7-2
Severity: important
Tags: patch
Usertags: hurd
User: debian-hurd@lists.debian.org
Hello,
Attached is a somewhat large patch for rrtool to successfully build on
GNU/Hurd, since PATH_MAX is not defined. This is fully compliant with
POSIX, see
http://pubs.opengroup.org/onlinepubs/007904875/basedefs/limits.h.html
Dynamic allocation is used all over to avoid defining constant values of
buffer lengths. Since the patch is non-trivial the best would be if
upstream could have a look at the proposed changes, in addition to the
Debian package maintainer.
In the patch there are a few FIXME statements I need comments on, as
well as the whole patch. At least rrddaemon, rrdtool and librrd4
install properly, but the clients should be tried out too. Maybe also
some of the changes should be #ifdef'ed on e.g. _POSIX_VERSION >=
200809L, etc. Let me know your opinion.
Thanks!
diff -ur rrdtool-1.4.7/src/rrd_client.c rrdtool-1.4.7.new/src/rrd_client.c
--- rrdtool-1.4.7/src/rrd_client.c 2011-11-24 11:55:18.000000000 +0100
+++ rrdtool-1.4.7.new/src/rrd_client.c 2012-09-10 18:37:39.000000000 +0200
@@ -68,12 +68,12 @@
* 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 == '/')
@@ -82,7 +82,7 @@
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;
@@ -583,7 +583,6 @@
rrdc_response_t *res;
int status;
int i;
- char file_path[PATH_MAX];
memset (buffer, 0, sizeof (buffer));
buffer_ptr = &buffer[0];
@@ -594,7 +593,7 @@
return (ENOBUFS);
pthread_mutex_lock (&lock);
- filename = get_path (filename, file_path);
+ filename = get_path (filename);
if (filename == NULL)
{
pthread_mutex_unlock (&lock);
@@ -644,7 +643,6 @@
size_t buffer_size;
rrdc_response_t *res;
int status;
- char file_path[PATH_MAX];
if (filename == NULL)
return (-1);
@@ -658,7 +656,7 @@
return (ENOBUFS);
pthread_mutex_lock (&lock);
- filename = get_path (filename, file_path);
+ filename = get_path (filename);
if (filename == NULL)
{
pthread_mutex_unlock (&lock);
diff -ur rrdtool-1.4.7/src/rrd_daemon.c rrdtool-1.4.7.new/src/rrd_daemon.c
--- rrdtool-1.4.7/src/rrd_daemon.c 2011-12-16 08:29:44.000000000 +0100
+++ rrdtool-1.4.7.new/src/rrd_daemon.c 2012-09-11 19:23:36.000000000 +0200
@@ -124,7 +124,7 @@
struct listen_socket_s
{
int fd;
- char addr[PATH_MAX + 1];
+ char *addr;
int family;
/* state for BATCH processing */
@@ -1076,21 +1076,24 @@
/* 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.
+ * by the called function, see below.
*
+ * 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, char **tmp)
{
- assert(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);
- *filename = tmp;
+ len = strlen(config_base_dir) + 1 + strlen(*filename) + 1;
+ *tmp = malloc(len);
+ snprintf(*tmp, len, "%s/%s", config_base_dir, *filename);
+ *filename = *tmp;
} /* }}} static int get_abs_path */
static int flush_file (const char *filename) /* {{{ */
@@ -1181,7 +1184,7 @@
static int handle_request_flush (HANDLER_PROTO) /* {{{ */
{
- char *file, file_tmp[PATH_MAX];
+ char *file, *file_tmp = NULL;
int status;
status = buffer_get_field (&buffer, &buffer_size, &file);
@@ -1195,7 +1198,8 @@
stats_flush_received++;
pthread_mutex_unlock(&stats_lock);
- get_abs_path(&file, file_tmp);
+ get_abs_path(&file, &file_tmp);
+ free(file_tmp);
if (!check_file_access(file, sock)) return 0;
status = flush_file (file);
@@ -1236,14 +1240,15 @@
static int handle_request_pending(HANDLER_PROTO) /* {{{ */
{
int status;
- char *file, file_tmp[PATH_MAX];
+ char *file, *file_tmp = NULL;
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, &file_tmp);
+ free(file_tmp);
pthread_mutex_lock(&cache_lock);
ci = g_tree_lookup(cache_tree, file);
@@ -1264,13 +1269,14 @@
{
int status;
gboolean found;
- char *file, file_tmp[PATH_MAX];
+ char *file, *file_tmp = NULL;
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, &file_tmp);
+ free(file_tmp);
if (!check_file_access(file, sock)) return 0;
pthread_mutex_lock(&cache_lock);
@@ -1311,7 +1317,7 @@
static int handle_request_update (HANDLER_PROTO) /* {{{ */
{
- char *file, file_tmp[PATH_MAX];
+ char *file, *file_tmp = NULL;
int values_num = 0;
int status;
char orig_buf[CMD_MAX];
@@ -1330,7 +1336,8 @@
stats_updates_received++;
pthread_mutex_unlock(&stats_lock);
- get_abs_path(&file, file_tmp);
+ get_abs_path(&file, &file_tmp);
+ free(file_tmp);
if (!check_file_access(file, sock)) return 0;
pthread_mutex_lock (&cache_lock);
@@ -1865,8 +1872,8 @@
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);
@@ -1875,7 +1882,9 @@
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,
@@ -1893,6 +1902,7 @@
/* record the file in the journal set */
rrd_add_strdup(&journal_cur->files, &journal_cur->files_num, new_file);
+ free(new_file);
return;
error:
@@ -1902,6 +1912,7 @@
RRDD_LOG(LOG_CRIT,
"JOURNALING DISABLED: All values will be flushed at shutdown");
+ free(new_file);
close(new_fd);
config_flush_at_shutdown = 1;
@@ -2098,10 +2109,10 @@
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;
@@ -2120,14 +2131,18 @@
* 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;
+ 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);
}
dir = opendir(journal_dir);
@@ -2141,14 +2156,18 @@
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);
}
closedir(dir);
@@ -2676,7 +2695,7 @@
static int daemonize (void) /* {{{ */
{
- int pid_fd;
+ int pid_fd, len;
char *base_dir;
daemon_uid = geteuid();
@@ -2698,9 +2717,9 @@
}
else
{
- strncpy(default_socket.addr, RRDCACHED_DEFAULT_ADDRESS,
- sizeof(default_socket.addr) - 1);
- default_socket.addr[sizeof(default_socket.addr) - 1] = '\0';
+ 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);
@@ -2789,6 +2808,9 @@
free(queue_threads);
free(config_base_dir);
+ /* FIXME: Put at the right place */
+ // see free_listen_sockets(listen_dfs):
+ free(default_socket.addr);
pthread_mutex_lock(&cache_lock);
g_tree_destroy(cache_tree);
@@ -2826,16 +2848,19 @@
case 'l':
{
listen_socket_t *new;
+ int len;
new = malloc(sizeof(listen_socket_t));
+ len = strlen(optarg + 1);
+ new->addr = malloc(len);
if (new == NULL)
{
fprintf(stderr, "read_options: malloc failed.\n");
return(2);
}
- memset(new, 0, sizeof(listen_socket_t));
+ memset(new, 0, sizeof(new));
- strncpy(new->addr, optarg, sizeof(new->addr)-1);
+ strncpy(new->addr, optarg, len);
/* Add permissions to the socket {{{ */
if (default_socket.permissions != 0)
@@ -3005,7 +3030,7 @@
case 'b':
{
size_t len;
- char base_realpath[PATH_MAX];
+ char *base_realpath = NULL;
if (config_base_dir != NULL)
free (config_base_dir);
@@ -3028,7 +3053,7 @@
* 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));
@@ -3065,8 +3090,10 @@
"Please consult rrdcached '-b' documentation!\n"
"Consider specifying the real directory (%s)\n",
base_realpath);
+ free(base_realpath);
return 5;
}
+ free(base_realpath);
}
break;
@@ -3089,9 +3116,8 @@
case 'j':
{
- char journal_dir_actual[PATH_MAX];
- const char *dir;
- dir = journal_dir = strdup(realpath((const char *)optarg, journal_dir_actual));
+
+ const char *dir = (const char *)optarg;
status = rrd_mkdir_p(dir, 0777);
if (status != 0)
@@ -3100,13 +3126,21 @@
dir, rrd_strerror(errno));
return 6;
}
+ journal_dir = realpath((const char *)dir, NULL);
+ if (! journal_dir) {
+ fprintf(stderr, "Failed to canonicalize journal directory '%s': %s\n",
+ dir, rrd_strerror(errno));
+ return 6;
+ }
- if (access(dir, R_OK|W_OK|X_OK) != 0)
+ 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;
}
+ free(journal_dir);
}
break;
Reply to: