Re: rrdtool: FTBFS on hurd-i386
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.
>
> TODO:
> The whole package will be tested with valgrind on GNU/Linux!
> 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)
> {
> + int len;
> assert(tmp != NULL);
> 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);
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.
> } /* }}} static int get_abs_path */
>
> static int flush_file (const char *filename) /* {{{ */
> @@ -1848,8 +1852,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);
> @@ -1858,7 +1862,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);
This needs to be freed before returning, as the function does not seem
to return any pointer to it.
> + 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 *.
> + len = strlen(journal_dir) + 1 + strlen(JOURNAL_BASE) + 5 + 1;
> + old_path = malloc (len);
> + path = malloc (len);
Spaces before (.
> + 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);
> @@ -2365,8 +2380,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;
Space before (.
> + listen_fds[listen_fds_num].addr = malloc(len);
> + strncpy(listen_fds[listen_fds_num].addr, path, len);
> listen_fds_num++;
>
> return (0);
> @@ -2516,6 +2532,7 @@
>
> 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 { }.
> }
>
> 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.
> }
>
> @@ -2796,7 +2816,9 @@
> }
> memset(new, 0, sizeof(listen_socket_t));
>
> - strncpy(new->addr, optarg, sizeof(new->addr)-1);
> + len = strlen(optarg) + 1;
> + new->addr = malloc(len);
Indented with tabs instead of spaces.
> + strncpy(new->addr, optarg, len);
>
> /* Add permissions to the socket {{{ */
> if (permissions_len != 0)
> @@ -2839,8 +2861,10 @@
> &config_listen_address_list_len, new))
> {
> fprintf(stderr, "read_options: rrd_add_ptr failed.\n");
> + free(new);
> return (2);
> }
> + free(new);
free()s indented with tabs instead of spaces.
> }
> break;
>
> @@ -2980,9 +3004,11 @@
>
> case 'b':
> {
> - size_t len;
> +#if _POSIX_C_SOURCE >= 200809L
> + char *base_realpath = NULL;
> +#else
> char base_realpath[PATH_MAX];
> -
> +#endif
Blank line between variable definitions and code removed.
> if (config_base_dir != NULL)
> free (config_base_dir);
> config_base_dir = strdup (optarg);
> @@ -3004,7 +3030,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)
The intendation seems off.
> +#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));
> @@ -3021,6 +3051,9 @@
> if (len < 1)
> {
> fprintf (stderr, "Invalid base directory: %s\n", optarg);
> +#if _POSIX_C_SOURCE >= 200809L
> + free(base_realpath);
Tab.
> +#endif
> return (4);
> }
>
> @@ -3034,15 +3067,21 @@
> }
>
> if (strncmp(config_base_dir,
> - base_realpath, sizeof(base_realpath)) != 0)
> + base_realpath, strlen(base_realpath)) != 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);
> +#if _POSIX_C_SOURCE >= 200809L
> + free(base_realpath);
> +#endif
Tab.
> return 5;
> }
> +#if _POSIX_C_SOURCE >= 200809L
> + free(base_realpath);
> +#endif
Tab.
> }
> break;
>
thanks,
guillem
Reply to: