Re: [PATCH] rrd_client.c issues

Svante Signell <[email protected]> Mon, 19 Aug 2013 20:10:27 +0200

Newsgroups	gmane.comp.db.rrdtool.devel
Message-ID	<[email protected]>

On Mon, 2013-08-19 at 18:33 +0200, Tobias Oetiker wrote:

> > > I can only tell you when I see the patch ...
> 
> if I am reading your code corectly, you are causing a memory leak
> by makeing get_path return an allocated string instead of a
> constant ... the root cause of the problem is that you are
> changeing the API of get_path ...

Yes, there is a memory leak introduced by allocing the string instead of
having a constant one. Regarding the API the only code calling
get_path() is in rrd_client.c.

> for backward compatibility this is rather sub optimal ...
> 
> better choose a new function name and whoever is using it knows
> that they have to free the data they get from the call.

What about the new version, here get_path is defined differently
depending on whether POSIX.1-2008 is supported or not by #if-ing on
_POSIX_VERSION (see man realpath). However, I can use a different name
for the alloced version compared to the constant version.

Thanks,
Svante

_______________________________________________
rrd-developers mailing list
[email protected]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

test_get_path.c (text/x-csrc, 3.5 KB)

#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <pthread.h>
#include <limits.h>

static char *sd_path = "/tmp"; /* cache the path for sd */
static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;

/* Undefines the _POSIX_VERSION */
#undef _POSIX_VERSION

/* get_path: Return a path name appropriate to be sent to the daemon.
 *
 * When talking to a local daemon (thru a UNIX socket), relative path names
 * are resolved to absolute path names to allow for transparent integration
 * into existing solutions (as requested by Tobi). Else, absolute path names
 * are not allowed, since path name translation is done by the server.
 *
 * One must hold `lock' when calling this function. */
#if (_POSIX_VERSION >= 200809L)
static char *get_path (const char *path) /* {{{ */
{
  char *ret = (char *)path;
  int is_unix = 0;

  printf("get_path: path=%s\n", path);
  if ((path == NULL) || (sd_path == NULL))
    return (NULL);

  if ((*sd_path == '/')
      || (strncmp ("unix:", sd_path, strlen ("unix:")) == 0))
    is_unix = 1;

  if (is_unix)
  {
    ret = realpath(path, NULL);
    if (ret == NULL)
      printf("realpath(%s): %s\n", path, strerror(errno));
    return ret;
  }
  else
  {
    if (*path == '/') /* not absolute path */
    {
      printf("absolute path names not allowed when talking "
          "to a remote daemon\n");
      return NULL;
    }
  }
  return (char *)path;
} /* }}} char *get_path */

#else
static const char *get_path (const char *path, char *resolved_path) /* {{{ */
{
  const char *ret = path;
  int is_unix = 0;

  if ((path == NULL) || (resolved_path == NULL) || (sd_path == NULL))
    return (NULL);

  if ((*sd_path == '/')
      || (strncmp ("unix:", sd_path, strlen ("unix:")) == 0))
    is_unix = 1;

  if (is_unix)
  {
    ret = realpath(path, resolved_path);
    if (ret == NULL)
      printf("realpath(%s): %s\n", path, strerror(errno));
    return ret;
  }
  else
  {
    if (*path == '/') /* not absolute path */
    {
      printf("absolute path names not allowed when talking "
          "to a remote daemon\n");
      return NULL;
    }
  }

  return path;
} /* }}} char *get_path */
#endif

int rrdc_create (const char *filename)
{
#if (_POSIX_VERSION >= 200809L)
  char *file_path;
#else
  char file_path[PATH_MAX];
#endif
  if (filename == NULL) {
    printf("rrdc_create: no filename specified\n");
    return (-1);
  } else
    printf("rrdc_create: filename=%s\n", filename);


  pthread_mutex_lock (&lock);
  printf("filename=%s\n", filename);
#if (_POSIX_VERSION >= 200809L)
  file_path = get_path (filename);
  printf("file_path=%s\n", file_path);
  if (file_path == NULL)
#else
  filename = get_path (filename, file_path);
  printf("filename=%s\n", file_path);
  if (filename == NULL)
#endif
  {
    pthread_mutex_unlock (&lock);
    return (-1);
  }
  pthread_mutex_unlock (&lock);

  /* Cannot be freed if using const */
#if (_POSIX_VERSION >= 200809L)
  free(file_path);
#endif
  return(0);
}

int rrd_create(
    int argc,
    char **argv)
{
    int       rc;
    optind = 0;

    // option handling 
    rc = rrdc_create (argv[optind]);
    printf("rc = %d (0=OK)\n",rc);
    return rc;
}

int main(
    int argc,
    char *argv[])
{
  if (argc != 2)
    {
      printf("error: option argument missing\n");
      return(1);
    }
  if (argv[1])
    {
      printf("calling rrd_create\n");
      rrd_create(argc - 1, &argv[1]);
    }
  else
    {
      printf("error: return\n");
      return(1);
    }

  return (0);

}