Re: rrdtool: FTBFS on hurd-i386
In the meanwhile a new upstream was packaged, 1.4.7. The diff is against
that one. Updated patch attached, and comments to the old below!
On Fri, 2012-01-27 at 10:51 +0100, Guillem Jover wrote:
> Hi!
>
> On Tue, 2012-01-24 at 20:18:27 +0100, Svante Signell wrote:
> > FIXME:
> > Still have to free filename at the correct place. And some issues are
> > still not completely clear.
Getting closer!
> > TODO:
> > The whole package will be tested with valgrind on GNU/Linux!
Not yet!
>
> > diff -ur rrdtool-1.4.3/src/rrd_daemon.c rrdtool-1.4.3.modified/src/rrd_daemon.c
> > --- rrdtool-1.4.3/src/rrd_daemon.c 2010-03-22 15:51:06.000000000 +0100
> > +++ rrdtool-1.4.3.modified/src/rrd_daemon.c 2012-01-24 20:06:18.000000000 +0100
> > @@ -1087,14 +1087,18 @@
> > */
> > static void get_abs_path(char **filename, char *tmp)
> > + 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);
>
> If tmp is passed back through *filename, it should not be freed, in
> addition it will leak because the caller passed the current filename
> there, but that pointer is most probably on the stack. The function
> signature might need to be changed to handle this properly.
You are right, it has to be freed in the calling functions, in four
places!
> > + new_file = malloc(len);
>
> This needs to be freed before returning, as the function does not seem
> to return any pointer to it.
Fixed!
> > + 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,
> > @@ -2103,14 +2109,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;
>
> Space after *.
Ditto!
> > + old_path = malloc (len);
> > + path = malloc (len);
>
> Spaces before (.
Ditto!
> > + len = strlen (path) + 1;
>
> Space before (.
Ditto!
> > if (listen_fds[i].family == PF_UNIX)
> > unlink(listen_fds[i].addr);
> > + free(listen_fds[i].addr);
>
> This either needs to be reindented two spaces to the left, or the if
> body needs to be surrounded with { }.
Fixed!
> > }
> >
> > free (listen_fds);
>
> > @@ -2664,7 +2681,9 @@
> > {
> > listen_socket_t sock;
> > memset(&sock, 0, sizeof(sock));
> > - strncpy(sock.addr, RRDCACHED_DEFAULT_ADDRESS, sizeof(sock.addr)-1);
> > + len = strlen(RRDCACHED_DEFAULT_ADDRESS) + 1;
> > + sock.addr = malloc(len);
> > + strncpy(sock.addr, RRDCACHED_DEFAULT_ADDRESS, len);
> > open_listen_socket (&sock);
>
> I assume open_listen_socket does not keep a reference to addr,
> otherwise it needs to be freed here.
Done!
> > - strncpy(new->addr, optarg, sizeof(new->addr)-1);
> > + len = strlen(optarg) + 1;
> > + new->addr = malloc(len);
>
> Indented with tabs instead of spaces.
Fixed!
> > + free(new);
> > return (2);
> > }
> > + free(new);
>
> free()s indented with tabs instead of spaces.
Ditto!
> > char base_realpath[PATH_MAX];
> > -
> > +#endif
>
> Blank line between variable definitions and code removed.
OK!
> > +#if _POSIX_C_SOURCE >= 200809L
> > + if ((base_realpath = realpath(config_base_dir, NULL)) == NULL)
>
> The intendation seems off.
It was!
> > fprintf (stderr, "Invalid base directory: %s\n", optarg);
> > +#if _POSIX_C_SOURCE >= 200809L
> > + free(base_realpath);
>
> Tab.
OK!
> > +#if _POSIX_C_SOURCE >= 200809L
> > + free(base_realpath);
> > +#endif
>
> Tab.
Again!
> > return 5;
> > }
> > +#if _POSIX_C_SOURCE >= 200809L
> > + free(base_realpath);
> > +#endif
>
> Tab.
Yes!
Thanks!
diff -ur rrdtool-1.4.7/src/rrd_daemon.c rrdtool-1.4.7.modified/src/rrd_daemon.c
--- rrdtool-1.4.7/src/rrd_daemon.c 2011-12-16 08:29:44.000000000 +0100
+++ rrdtool-1.4.7.modified/src/rrd_daemon.c 2012-01-30 17:02:59.000000000 +0100
@@ -21,6 +21,7 @@
* kevin brintnall <kbrint@rufus.net>
**/
+
#if 0
/*
* First tell the compiler to stick to the C99 and POSIX standards as close as
@@ -124,7 +125,7 @@
struct listen_socket_s
{
int fd;
- char addr[PATH_MAX + 1];
+ char *addr;
int family;
/* state for BATCH processing */
@@ -1081,16 +1082,19 @@
* 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;
+
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 +1185,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 +1199,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 +1241,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 +1270,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 +1318,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 +1337,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 +1873,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 +1883,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) + 2 + 2*3*sizeof(int) + 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 +1903,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:
@@ -1903,6 +1914,7 @@
"JOURNALING DISABLED: All values will be flushed at shutdown");
close(new_fd);
+ free(new_file);
config_flush_at_shutdown = 1;
} /* }}} journal_new_file */
@@ -2098,10 +2110,10 @@
static void journal_init(void) /* {{{ */
{
- int had_journal = 0;
+ int had_journal = 0, len;
DIR *dir;
struct dirent *dent;
- char path[PATH_MAX+1];
+ char *path = NULL;
if (journal_dir == NULL) return;
@@ -2120,14 +2132,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,7 +2157,11 @@
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;
+ if (path != NULL)
+ free(path);
+ path = malloc(len);
+ snprintf(path, len, "%s/%s", journal_dir, dent->d_name);
if (!rrd_add_strdup(&journal_cur->files, &journal_cur->files_num, path))
{
@@ -2150,6 +2170,7 @@
break;
}
}
+ free(path);
closedir(dir);
qsort(journal_cur->files, journal_cur->files_num,
@@ -2308,7 +2329,7 @@
int fd;
struct sockaddr_un sa;
listen_socket_t *temp;
- int status;
+ int status, len;
const char *path;
char *path_copy, *dir;
@@ -2401,8 +2422,9 @@
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);
+ len = strlen(path) + 1;
+ listen_fds[listen_fds_num].addr = malloc(len);
+ strncpy(listen_fds[listen_fds_num].addr, path, len);
listen_fds_num++;
return (0);
@@ -2551,7 +2573,10 @@
close (listen_fds[i].fd);
if (listen_fds[i].family == PF_UNIX)
+ {
unlink(listen_fds[i].addr);
+ free(listen_fds[i].addr);
+ }
}
free (listen_fds);
@@ -2676,7 +2701,7 @@
static int daemonize (void) /* {{{ */
{
- int pid_fd;
+ int pid_fd, len;
char *base_dir;
daemon_uid = geteuid();
@@ -2698,14 +2723,16 @@
}
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);
+ default_socket.addr = malloc(len);
+ strncpy(default_socket.addr, RRDCACHED_DEFAULT_ADDRESS, len);
+ default_socket.addr[len] = '\0';
if (default_socket.permissions == 0)
socket_permission_set_all (&default_socket);
open_listen_socket (&default_socket);
+ free(default_socket.addr);
}
if (listen_fds_num < 1)
@@ -2808,6 +2835,7 @@
static int read_options (int argc, char **argv) /* {{{ */
{
int option;
+ size_t len;
int status = 0;
socket_permission_clear (&default_socket);
@@ -2835,7 +2863,9 @@
}
memset(new, 0, sizeof(listen_socket_t));
- strncpy(new->addr, optarg, sizeof(new->addr)-1);
+ len = strlen(optarg) + 1;
+ new->addr = malloc(len);
+ strncpy(new->addr, optarg, len);
/* Add permissions to the socket {{{ */
if (default_socket.permissions != 0)
@@ -2856,8 +2886,10 @@
&config_listen_address_list_len, new))
{
fprintf(stderr, "read_options: rrd_add_ptr failed.\n");
+ free(new);
return (2);
}
+ free(new);
}
break;
@@ -3004,8 +3036,11 @@
case 'b':
{
- size_t len;
+#if _POSIX_C_SOURCE >= 200809L
+ char *base_realpath = NULL;
+#else
char base_realpath[PATH_MAX];
+#endif
if (config_base_dir != NULL)
free (config_base_dir);
@@ -3028,7 +3063,11 @@
* assumptions possible (we don't have to resolve paths
* that start with a "/")
*/
+#if _POSIX_C_SOURCE >= 200809L
+ if ((base_realpath = realpath(config_base_dir, NULL)) == NULL)
+#else
if (realpath(config_base_dir, base_realpath) == NULL)
+#endif
{
fprintf (stderr, "Failed to canonicalize the base directory '%s': "
"%s\n", config_base_dir, rrd_strerror(errno));
@@ -3045,6 +3084,9 @@
if (len < 1)
{
fprintf (stderr, "Invalid base directory: %s\n", optarg);
+#if _POSIX_C_SOURCE >= 200809L
+ free(base_realpath);
+#endif
return (4);
}
@@ -3065,8 +3107,14 @@
"Please consult rrdcached '-b' documentation!\n"
"Consider specifying the real directory (%s)\n",
base_realpath);
+#if _POSIX_C_SOURCE >= 200809L
+ free(base_realpath);
+#endif
return 5;
}
+#if _POSIX_C_SOURCE >= 200809L
+ free(base_realpath);
+#endif
}
break;
@@ -3089,15 +3137,20 @@
case 'j':
{
- char journal_dir_actual[PATH_MAX];
+ char *journal_dir_actual = NULL;
const char *dir;
+#if _POSIX_C_SOURCE >= 200809L
+ journal_dir_actual = realpath((const char *)optarg, NULL);
+ dir = journal_dir = strdup(journal_dir_actual);
+#else
dir = journal_dir = strdup(realpath((const char *)optarg, journal_dir_actual));
-
+#endif
status = rrd_mkdir_p(dir, 0777);
if (status != 0)
{
fprintf(stderr, "Failed to create journal directory '%s': %s\n",
dir, rrd_strerror(errno));
+ free(journal_dir_actual);
return 6;
}
@@ -3105,8 +3158,10 @@
{
fprintf(stderr, "Must specify a writable directory with -j! (%s)\n",
errno ? rrd_strerror(errno) : "");
+ free(journal_dir_actual);
return 6;
}
+ free(journal_dir_actual);
}
break;
Reply to: