[Openvpn-devel,08/10] Move file-related functions from misc.c to platform.c

  To avoid having to include misc.c - which is a dependency mess - in the
tls-crypt unit tests, move file-handing related functions to platform.c
(which is where other file-related functions already reside).

Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
---
 src/openvpn/init.c                         |   2 +-
 src/openvpn/misc.c                         | 148 -----------------------------
 src/openvpn/misc.h                         |  12 ---
 src/openvpn/multi.c                        |  25 ++---
 src/openvpn/pf.c                           |   4 +-
 src/openvpn/platform.c                     | 148 +++++++++++++++++++++++++++++
 src/openvpn/platform.h                     |  18 ++++
 src/openvpn/plugin.c                       |   6 +-
 src/openvpn/ssl_verify.c                   |  12 ++-
 tests/unit_tests/openvpn/Makefile.am       |   3 +
 tests/unit_tests/openvpn/mock_get_random.c |  36 +++++++
 11 files changed, 232 insertions(+), 182 deletions(-)
 create mode 100644 tests/unit_tests/openvpn/mock_get_random.c
  

On 08/12/17 20:07, Steffan Karger wrote:
> To avoid having to include misc.c - which is a dependency mess - in the
> tls-crypt unit tests, move file-handing related functions to platform.c
> (which is where other file-related functions already reside).
> 
> Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>

I have applied this patch right after the one about PEM and it triggered
the following:

Falling back to patching base and 3-way merge...

How about rebasing it before the actual tls-crypt-v2 patches so that it
could be applied earlier and shrink down this patchset?


On top of that, would you please extend the commit message a little bit
to include the fact that you are adding a new mock'd component
(get_random) needed by the tls-crypt unit-test?

Actually....If I understand correctly, this new mock'd component will
only be used when the other patches will be merged too.
Therefore, I guess this patch could be divided in 2: one part being the
code move and the second part being the introduction of the mock'd
component. The latter may directly be included in the patch adding the
tls-crypt unit-test.

What do you think?

Other than this, the patch looks good.
Checked with "git show --color-moved=zebra" to ensure that the code
being moved wasn't modified.

And +1 for this patch for cleaning up the misc.c mess some more.


Cheers,


> ---
>  src/openvpn/init.c                         |   2 +-
>  src/openvpn/misc.c                         | 148 -----------------------------
>  src/openvpn/misc.h                         |  12 ---
>  src/openvpn/multi.c                        |  25 ++---
>  src/openvpn/pf.c                           |   4 +-
>  src/openvpn/platform.c                     | 148 +++++++++++++++++++++++++++++
>  src/openvpn/platform.h                     |  18 ++++
>  src/openvpn/plugin.c                       |   6 +-
>  src/openvpn/ssl_verify.c                   |  12 ++-
>  tests/unit_tests/openvpn/Makefile.am       |   3 +
>  tests/unit_tests/openvpn/mock_get_random.c |  36 +++++++
>  11 files changed, 232 insertions(+), 182 deletions(-)
>  create mode 100644 tests/unit_tests/openvpn/mock_get_random.c
> 
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index a4603b3..4055f28 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -807,7 +807,7 @@ init_static(void)
>  #ifdef STATUS_PRINTF_TEST
>      {
>          struct gc_arena gc = gc_new();
> -        const char *tmp_file = create_temp_file("/tmp", "foo", &gc);
> +        const char *tmp_file = platform_create_temp_file("/tmp", "foo", &gc);
>          struct status_output *so = status_open(tmp_file, 0, -1, NULL, STATUS_OUTPUT_WRITE);
>          status_printf(so, "%s", "foo");
>          status_printf(so, "%s", "bar");
> diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> index ad26f3d..b04a9d7 100644
> --- a/src/openvpn/misc.c
> +++ b/src/openvpn/misc.c
> @@ -313,87 +313,6 @@ openvpn_popen(const struct argv *a,  const struct env_set *es)
>      return ret;
>  }
>  
> -
> -/* return true if filename can be opened for read */
> -bool
> -test_file(const char *filename)
> -{
> -    bool ret = false;
> -    if (filename)
> -    {
> -        FILE *fp = platform_fopen(filename, "r");
> -        if (fp)
> -        {
> -            fclose(fp);
> -            ret = true;
> -        }
> -        else
> -        {
> -            if (openvpn_errno() == EACCES)
> -            {
> -                msg( M_WARN | M_ERRNO, "Could not access file '%s'", filename);
> -            }
> -        }
> -    }
> -
> -    dmsg(D_TEST_FILE, "TEST FILE '%s' [%d]",
> -         filename ? filename : "UNDEF",
> -         ret);
> -
> -    return ret;
> -}
> -
> -/* create a temporary filename in directory */
> -const char *
> -create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)
> -{
> -    int fd;
> -    const char *retfname = NULL;
> -    unsigned int attempts = 0;
> -    char fname[256] = { 0 };
> -    const char *fname_fmt = PACKAGE "_%.*s_%08lx%08lx.tmp";
> -    const int max_prefix_len = sizeof(fname) - (sizeof(PACKAGE) + 7 + (2 * 8));
> -
> -    while (attempts < 6)
> -    {
> -        ++attempts;
> -
> -        if (!openvpn_snprintf(fname, sizeof(fname), fname_fmt, max_prefix_len,
> -                              prefix, (unsigned long) get_random(),
> -                              (unsigned long) get_random()))
> -        {
> -            msg(M_WARN, "ERROR: temporary filename too long");
> -            return NULL;
> -        }
> -
> -        retfname = gen_path(directory, fname, gc);
> -        if (!retfname)
> -        {
> -            msg(M_WARN, "Failed to create temporary filename and path");
> -            return NULL;
> -        }
> -
> -        /* Atomically create the file.  Errors out if the file already
> -         * exists.  */
> -        fd = platform_open(retfname, O_CREAT | O_EXCL | O_WRONLY, S_IRUSR | S_IWUSR);
> -        if (fd != -1)
> -        {
> -            close(fd);
> -            return retfname;
> -        }
> -        else if (fd == -1 && errno != EEXIST)
> -        {
> -            /* Something else went wrong, no need to retry.  */
> -            msg(M_WARN | M_ERRNO, "Could not create temporary file '%s'",
> -                retfname);
> -            return NULL;
> -        }
> -    }
> -
> -    msg(M_WARN, "Failed to create temporary file after %i attempts", attempts);
> -    return NULL;
> -}
> -
>  /*
>   * Prepend a random string to hostname to prevent DNS caching.
>   * For example, foo.bar.gov would be modified to <random-chars>.foo.bar.gov.
> @@ -416,73 +335,6 @@ hostname_randomize(const char *hostname, struct gc_arena *gc)
>  }
>  
>  /*
> - * Put a directory and filename together.
> - */
> -const char *
> -gen_path(const char *directory, const char *filename, struct gc_arena *gc)
> -{
> -#ifdef _WIN32
> -    const int CC_PATH_RESERVED = CC_LESS_THAN|CC_GREATER_THAN|CC_COLON
> -                                 |CC_DOUBLE_QUOTE|CC_SLASH|CC_BACKSLASH|CC_PIPE|CC_QUESTION_MARK|CC_ASTERISK;
> -#else
> -    const int CC_PATH_RESERVED = CC_SLASH;
> -#endif
> -
> -    if (!gc)
> -    {
> -        return NULL; /* Would leak memory otherwise */
> -    }
> -
> -    const char *safe_filename = string_mod_const(filename, CC_PRINT, CC_PATH_RESERVED, '_', gc);
> -
> -    if (safe_filename
> -        && strcmp(safe_filename, ".")
> -        && strcmp(safe_filename, "..")
> -#ifdef _WIN32
> -        && win_safe_filename(safe_filename)
> -#endif
> -        )
> -    {
> -        const size_t outsize = strlen(safe_filename) + (directory ? strlen(directory) : 0) + 16;
> -        struct buffer out = alloc_buf_gc(outsize, gc);
> -        char dirsep[2];
> -
> -        dirsep[0] = OS_SPECIFIC_DIRSEP;
> -        dirsep[1] = '\0';
> -
> -        if (directory)
> -        {
> -            buf_printf(&out, "%s%s", directory, dirsep);
> -        }
> -        buf_printf(&out, "%s", safe_filename);
> -
> -        return BSTR(&out);
> -    }
> -    else
> -    {
> -        return NULL;
> -    }
> -}
> -
> -bool
> -absolute_pathname(const char *pathname)
> -{
> -    if (pathname)
> -    {
> -        const int c = pathname[0];
> -#ifdef _WIN32
> -        return c == '\\' || (isalpha(c) && pathname[1] == ':' && pathname[2] == '\\');
> -#else
> -        return c == '/';
> -#endif
> -    }
> -    else
> -    {
> -        return false;
> -    }
> -}
> -
> -/*
>   * Get and store a username/password
>   */
>  
> diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
> index 3725fcf..82bff9a 100644
> --- a/src/openvpn/misc.h
> +++ b/src/openvpn/misc.h
> @@ -79,18 +79,6 @@ const char **make_extended_arg_array(char **p, struct gc_arena *gc);
>  /* an analogue to the random() function, but use OpenSSL functions if available */
>  long int get_random(void);
>  
> -/* return true if filename can be opened for read */
> -bool test_file(const char *filename);
> -
> -/* create a temporary file in directory, returns the filename of the created file */
> -const char *create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc);
> -
> -/* put a directory and filename together */
> -const char *gen_path(const char *directory, const char *filename, struct gc_arena *gc);
> -
> -/* return true if pathname is absolute */
> -bool absolute_pathname(const char *pathname);
> -
>  /* prepend a random prefix to hostname */
>  const char *hostname_randomize(const char *hostname, struct gc_arena *gc);
>  
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 25b2d09..acc6912 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1639,7 +1639,7 @@ multi_client_connect_post(struct multi_context *m,
>                            unsigned int *option_types_found)
>  {
>      /* Did script generate a dynamic config file? */
> -    if (test_file(dc_file))
> +    if (platform_test_file(dc_file))
>      {
>          options_server_import(&mi->context.options,
>                                dc_file,
> @@ -1826,12 +1826,13 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
>          {
>              const char *ccd_file;
>  
> -            ccd_file = gen_path(mi->context.options.client_config_dir,
> -                                tls_common_name(mi->context.c2.tls_multi, false),
> -                                &gc);
> +            ccd_file = platform_gen_path(mi->context.options.client_config_dir,
> +                                         tls_common_name(mi->context.c2.tls_multi,
> +                                                         false),
> +                                         &gc);
>  
>              /* try common-name file */
> -            if (test_file(ccd_file))
> +            if (platform_test_file(ccd_file))
>              {
>                  options_server_import(&mi->context.options,
>                                        ccd_file,
> @@ -1842,11 +1843,11 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
>              }
>              else /* try default file */
>              {
> -                ccd_file = gen_path(mi->context.options.client_config_dir,
> -                                    CCD_DEFAULT,
> -                                    &gc);
> +                ccd_file = platform_gen_path(mi->context.options.client_config_dir,
> +                                             CCD_DEFAULT,
> +                                             &gc);
>  
> -                if (test_file(ccd_file))
> +                if (platform_test_file(ccd_file))
>                  {
>                      options_server_import(&mi->context.options,
>                                            ccd_file,
> @@ -1876,7 +1877,8 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
>          if (plugin_defined(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
>          {
>              struct argv argv = argv_new();
> -            const char *dc_file = create_temp_file(mi->context.options.tmp_dir, "cc", &gc);
> +            const char *dc_file = platform_create_temp_file(mi->context.options.tmp_dir,
> +                                                            "cc", &gc);
>  
>              if (!dc_file)
>              {
> @@ -1938,7 +1940,8 @@ script_depr_failed:
>  
>              setenv_str(mi->context.c2.es, "script_type", "client-connect");
>  
> -            dc_file = create_temp_file(mi->context.options.tmp_dir, "cc", &gc);
> +            dc_file = platform_create_temp_file(mi->context.options.tmp_dir,
> +                                                "cc", &gc);
>              if (!dc_file)
>              {
>                  cc_succeeded = false;
> diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c
> index 6e4107c..fd9f215 100644
> --- a/src/openvpn/pf.c
> +++ b/src/openvpn/pf.c
> @@ -621,8 +621,8 @@ pf_init_context(struct context *c)
>  #ifdef PLUGIN_PF
>      if (plugin_defined(c->plugins, OPENVPN_PLUGIN_ENABLE_PF))
>      {
> -        c->c2.pf.filename = create_temp_file(c->options.tmp_dir, "pf",
> -                                             &c->c2.gc);
> +        c->c2.pf.filename = platform_create_temp_file(c->options.tmp_dir, "pf",
> +                                                      &c->c2.gc);
>          if (c->c2.pf.filename)
>          {
>              setenv_str(c->c2.es, "pf_file", c->c2.pf.filename);
> diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c
> index e942ba9..5281267 100644
> --- a/src/openvpn/platform.c
> +++ b/src/openvpn/platform.c
> @@ -31,6 +31,7 @@
>  
>  #include "buffer.h"
>  #include "error.h"
> +#include "misc.h"
>  #include "win32.h"
>  
>  #include "memdbg.h"
> @@ -335,3 +336,150 @@ platform_stat(const char *path, platform_stat_t *buf)
>  #endif
>  }
>  
> +/* create a temporary filename in directory */
> +const char *
> +platform_create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)
> +{
> +    int fd;
> +    const char *retfname = NULL;
> +    unsigned int attempts = 0;
> +    char fname[256] = { 0 };
> +    const char *fname_fmt = PACKAGE "_%.*s_%08lx%08lx.tmp";
> +    const int max_prefix_len = sizeof(fname) - (sizeof(PACKAGE) + 7 + (2 * 8));
> +
> +    while (attempts < 6)
> +    {
> +        ++attempts;
> +
> +        if (!openvpn_snprintf(fname, sizeof(fname), fname_fmt, max_prefix_len,
> +                              prefix, (unsigned long) get_random(),
> +                              (unsigned long) get_random()))
> +        {
> +            msg(M_WARN, "ERROR: temporary filename too long");
> +            return NULL;
> +        }
> +
> +        retfname = platform_gen_path(directory, fname, gc);
> +        if (!retfname)
> +        {
> +            msg(M_WARN, "Failed to create temporary filename and path");
> +            return NULL;
> +        }
> +
> +        /* Atomically create the file.  Errors out if the file already
> +         * exists.  */
> +        fd = platform_open(retfname, O_CREAT | O_EXCL | O_WRONLY, S_IRUSR | S_IWUSR);
> +        if (fd != -1)
> +        {
> +            close(fd);
> +            return retfname;
> +        }
> +        else if (fd == -1 && errno != EEXIST)
> +        {
> +            /* Something else went wrong, no need to retry.  */
> +            msg(M_WARN | M_ERRNO, "Could not create temporary file '%s'",
> +                retfname);
> +            return NULL;
> +        }
> +    }
> +
> +    msg(M_WARN, "Failed to create temporary file after %i attempts", attempts);
> +    return NULL;
> +}
> +
> +/*
> + * Put a directory and filename together.
> + */
> +const char *
> +platform_gen_path(const char *directory, const char *filename,
> +                  struct gc_arena *gc)
> +{
> +#ifdef _WIN32
> +    const int CC_PATH_RESERVED = CC_LESS_THAN|CC_GREATER_THAN|CC_COLON
> +                                 |CC_DOUBLE_QUOTE|CC_SLASH|CC_BACKSLASH|CC_PIPE|CC_QUESTION_MARK|CC_ASTERISK;
> +#else
> +    const int CC_PATH_RESERVED = CC_SLASH;
> +#endif
> +
> +    if (!gc)
> +    {
> +        return NULL; /* Would leak memory otherwise */
> +    }
> +
> +    const char *safe_filename = string_mod_const(filename, CC_PRINT, CC_PATH_RESERVED, '_', gc);
> +
> +    if (safe_filename
> +        && strcmp(safe_filename, ".")
> +        && strcmp(safe_filename, "..")
> +#ifdef _WIN32
> +        && win_safe_filename(safe_filename)
> +#endif
> +        )
> +    {
> +        const size_t outsize = strlen(safe_filename) + (directory ? strlen(directory) : 0) + 16;
> +        struct buffer out = alloc_buf_gc(outsize, gc);
> +        char dirsep[2];
> +
> +        dirsep[0] = OS_SPECIFIC_DIRSEP;
> +        dirsep[1] = '\0';
> +
> +        if (directory)
> +        {
> +            buf_printf(&out, "%s%s", directory, dirsep);
> +        }
> +        buf_printf(&out, "%s", safe_filename);
> +
> +        return BSTR(&out);
> +    }
> +    else
> +    {
> +        return NULL;
> +    }
> +}
> +
> +bool
> +platform_absolute_pathname(const char *pathname)
> +{
> +    if (pathname)
> +    {
> +        const int c = pathname[0];
> +#ifdef _WIN32
> +        return c == '\\' || (isalpha(c) && pathname[1] == ':' && pathname[2] == '\\');
> +#else
> +        return c == '/';
> +#endif
> +    }
> +    else
> +    {
> +        return false;
> +    }
> +}
> +
> +/* return true if filename can be opened for read */
> +bool
> +platform_test_file(const char *filename)
> +{
> +    bool ret = false;
> +    if (filename)
> +    {
> +        FILE *fp = platform_fopen(filename, "r");
> +        if (fp)
> +        {
> +            fclose(fp);
> +            ret = true;
> +        }
> +        else
> +        {
> +            if (openvpn_errno() == EACCES)
> +            {
> +                msg( M_WARN | M_ERRNO, "Could not access file '%s'", filename);
> +            }
> +        }
> +    }
> +
> +    dmsg(D_TEST_FILE, "TEST FILE '%s' [%d]",
> +         filename ? filename : "UNDEF",
> +         ret);
> +
> +    return ret;
> +}
> diff --git a/src/openvpn/platform.h b/src/openvpn/platform.h
> index cd2bbc9..e116fd5 100644
> --- a/src/openvpn/platform.h
> +++ b/src/openvpn/platform.h
> @@ -49,6 +49,7 @@
>  #endif
>  
>  #include "basic.h"
> +#include "buffer.h"
>  
>  /* Get/Set UID of process */
>  
> @@ -143,4 +144,21 @@ typedef struct stat platform_stat_t;
>  #endif
>  int platform_stat(const char *path, platform_stat_t *buf);
>  
> +/**
> + * Create a temporary file in directory, returns the filename of the created
> + * file.
> + */
> +const char *platform_create_temp_file(const char *directory, const char *prefix,
> +                                      struct gc_arena *gc);
> +
> +/** Put a directory and filename together. */
> +const char *platform_gen_path(const char *directory, const char *filename,
> +                              struct gc_arena *gc);
> +
> +/** Return true if pathname is absolute. */
> +bool platform_absolute_pathname(const char *pathname);
> +
> +/** Return true if filename can be opened for read. */
> +bool platform_test_file(const char *filename);
> +
>  #endif /* ifndef PLATFORM_H */
> diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c
> index 7387f8b..9dfa744 100644
> --- a/src/openvpn/plugin.c
> +++ b/src/openvpn/plugin.c
> @@ -249,7 +249,7 @@ plugin_init_item(struct plugin *p, const struct plugin_option *o)
>       * was parsed.
>       *
>       */
> -    if (!absolute_pathname(p->so_pathname)
> +    if (!platform_absolute_pathname(p->so_pathname)
>          && p->so_pathname[0] != '.')
>      {
>          char full[PATH_MAX];
> @@ -259,7 +259,7 @@ plugin_init_item(struct plugin *p, const struct plugin_option *o)
>      }
>      else
>      {
> -        rel = !absolute_pathname(p->so_pathname);
> +        rel = !platform_absolute_pathname(p->so_pathname);
>          p->handle = dlopen(p->so_pathname, RTLD_NOW);
>      }
>      if (!p->handle)
> @@ -271,7 +271,7 @@ plugin_init_item(struct plugin *p, const struct plugin_option *o)
>  
>  #else  /* ifndef _WIN32 */
>  
> -    rel = !absolute_pathname(p->so_pathname);
> +    rel = !platform_absolute_pathname(p->so_pathname);
>      p->module = LoadLibraryW(wide_string(p->so_pathname, &gc));
>      if (!p->module)
>      {
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index ebb1da2..3568bad 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -547,7 +547,7 @@ verify_cert_export_cert(openvpn_x509_cert_t *peercert, const char *tmp_dir, stru
>  
>      /* create tmp file to store peer cert */
>      if (!tmp_dir
> -        || !(peercert_filename = create_temp_file(tmp_dir, "pcf", gc)))
> +        || !(peercert_filename = platform_create_temp_file(tmp_dir, "pcf", gc)))
>      {
>          msg (M_WARN, "Failed to create peer cert file");
>          return NULL;
> @@ -887,7 +887,7 @@ key_state_gen_auth_control_file(struct key_state *ks, const struct tls_options *
>      struct gc_arena gc = gc_new();
>  
>      key_state_rm_auth_control_file(ks);
> -    const char *acf = create_temp_file(opt->tmp_dir, "acf", &gc);
> +    const char *acf = platform_create_temp_file(opt->tmp_dir, "acf", &gc);
>      if (acf)
>      {
>          ks->auth_control_file = string_alloc(acf, NULL);
> @@ -1100,7 +1100,8 @@ verify_user_pass_script(struct tls_session *session, const struct user_pass *up)
>          {
>              struct status_output *so;
>  
> -            tmp_file = create_temp_file(session->opt->tmp_dir, "up", &gc);
> +            tmp_file = platform_create_temp_file(session->opt->tmp_dir, "up",
> +                                                 &gc);
>              if (tmp_file)
>              {
>                  so = status_open(tmp_file, 0, -1, NULL, STATUS_OUTPUT_WRITE);
> @@ -1510,8 +1511,9 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session)
>          struct gc_arena gc = gc_new();
>  
>          const char *cn = session->common_name;
> -        const char *path = gen_path(session->opt->client_config_dir_exclusive, cn, &gc);
> -        if (!cn || !strcmp(cn, CCD_DEFAULT) || !test_file(path))
> +        const char *path = platform_gen_path(session->opt->client_config_dir_exclusive,
> +                                             cn, &gc);
> +        if (!cn || !strcmp(cn, CCD_DEFAULT) || !platform_test_file(path))
>          {
>              ks->authenticated = false;
>              wipe_auth_token(multi);
> diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am
> index 7b41363..b69da84 100644
> --- a/tests/unit_tests/openvpn/Makefile.am
> +++ b/tests/unit_tests/openvpn/Makefile.am
> @@ -19,6 +19,7 @@ argv_testdriver_CFLAGS  = @TEST_CFLAGS@ -I$(openvpn_srcdir) -I$(compat_srcdir) \
>  argv_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir) -Wl,--wrap=parse_line \
>  	$(OPTIONAL_CRYPTO_LIBS)
>  argv_testdriver_SOURCES = test_argv.c mock_msg.c \
> +	mock_get_random.c \
>  	$(openvpn_srcdir)/platform.c \
>  	$(openvpn_srcdir)/buffer.c \
>  	$(openvpn_srcdir)/argv.c
> @@ -26,6 +27,7 @@ argv_testdriver_SOURCES = test_argv.c mock_msg.c \
>  buffer_testdriver_CFLAGS  = @TEST_CFLAGS@ -I$(openvpn_srcdir) -I$(compat_srcdir)
>  buffer_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir) -Wl,--wrap=parse_line
>  buffer_testdriver_SOURCES = test_buffer.c mock_msg.c \
> +	mock_get_random.c \
>  	$(openvpn_srcdir)/buffer.c \
>  	$(openvpn_srcdir)/platform.c
>  
> @@ -50,6 +52,7 @@ packet_id_testdriver_CFLAGS  = @TEST_CFLAGS@ \
>  packet_id_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
>  	$(OPTIONAL_CRYPTO_LIBS)
>  packet_id_testdriver_SOURCES = test_packet_id.c mock_msg.c \
> +	mock_get_random.c \
>  	$(openvpn_srcdir)/buffer.c \
>  	$(openvpn_srcdir)/otime.c \
>  	$(openvpn_srcdir)/packet_id.c \
> diff --git a/tests/unit_tests/openvpn/mock_get_random.c b/tests/unit_tests/openvpn/mock_get_random.c
> new file mode 100644
> index 0000000..da92a9b
> --- /dev/null
> +++ b/tests/unit_tests/openvpn/mock_get_random.c
> @@ -0,0 +1,36 @@
> +/*
> + *  OpenVPN -- An application to securely tunnel IP networks
> + *             over a single UDP port, with support for SSL/TLS-based
> + *             session authentication and key exchange,
> + *             packet encryption, packet authentication, and
> + *             packet compression.
> + *
> + *  Copyright (C) 2017 Fox Crypto B.V. <openvpn@fox-it.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2
> + *  as published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include <stdarg.h>
> +#include <stddef.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <setjmp.h>
> +#include <cmocka.h>
> +
> +unsigned long
> +get_random(void)
> +{
> +    /* rand() is not very random, but it's C99 and this is just for testing */
> +    return rand();
> +}
>
  

Hi,

On 15-06-18 09:46, Antonio Quartulli wrote:
> 
> 
> On 08/12/17 20:07, Steffan Karger wrote:
>> To avoid having to include misc.c - which is a dependency mess - in the
>> tls-crypt unit tests, move file-handing related functions to platform.c
>> (which is where other file-related functions already reside).
>>
>> Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
> 
> I have applied this patch right after the one about PEM and it triggered
> the following:
> 
> Falling back to patching base and 3-way merge...
> 
> How about rebasing it before the actual tls-crypt-v2 patches so that it
> could be applied earlier and shrink down this patchset?
> 
> 
> On top of that, would you please extend the commit message a little bit
> to include the fact that you are adding a new mock'd component
> (get_random) needed by the tls-crypt unit-test?
> 
> Actually....If I understand correctly, this new mock'd component will
> only be used when the other patches will be merged too.
> Therefore, I guess this patch could be divided in 2: one part being the
> code move and the second part being the introduction of the mock'd
> component. The latter may directly be included in the patch adding the
> tls-crypt unit-test.
> 
> What do you think?

I think you have a good point.  I'll reorder, split and merge.

> Other than this, the patch looks good.
> Checked with "git show --color-moved=zebra" to ensure that the code
> being moved wasn't modified.
> 
> And +1 for this patch for cleaning up the misc.c mess some more.

Thanks!

-Steffan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
  

Hi,

On 22-06-18 08:40, Steffan Karger wrote:
> On 15-06-18 09:46, Antonio Quartulli wrote:
>> Actually....If I understand correctly, this new mock'd component will
>> only be used when the other patches will be merged too.
>> Therefore, I guess this patch could be divided in 2: one part being the
>> code move and the second part being the introduction of the mock'd
>> component. The latter may directly be included in the patch adding the
>> tls-crypt unit-test.
>>
>> What do you think?
> 
> I think you have a good point.  I'll reorder, split and merge.

Hm, not that easy actually.  The file functions themselves use
get_random, to produce a random filename for the temp files.  So just
reorder, not split/merge.  I'll add a note about this in the commit message.

-Steffan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot