Skip to contentSkip to conversation

convert *_inline attributes to bool

PATCH Superseded Antonio Quartulli 2 participants 3 messages Jan 14, 2017 – Jan 15, 2017

v3 of 8 Superseded Latest is v8 →

lore
Share & tools
lore
#0 lore raw

PATCH v3 1/1 convert *_inline attributes to bool

Carrying around the INLINE_TAG is not really efficient, because it requires a strcmp() to be performed every time we want to understand if the data is stored inline or not.

Convert all the *_inline attributes to bool to make the logic easier and checks more efficient.

---

Based on master + [PATCH (master)] reformatting: fix style in crypto*.{c, h}

Changes from v2:
- fix indentation in ssl_openssl.c
- do not attempt to push inline'd options
- do not attempt to parse inline'd plugin
- introduce check_file_access_inline() and check_file_access_chroot_inline()
- introduce OPT_P_INLINE to specify when an option is allowed to be inline. Options not having this permission will fail to be parsed if is_inline is true.

Changes from v1:
- remove the INLINE_TAG from the options parsing logic at all. Now a boolean variable is passed around.
- add print_if_inline() helper function (to misc.c/h) to make sure we never print the inline data, but only the INLINE tag. Such function checks also for NULL pointers;
- make sure print_if_inline() is always used when printing possibly inline data;
- remove the INLINE_TAG from the options parsing logic at all. Now a boolean variable is passed around.
- fix alignment error in comment;
- remove CHKACC_INLINE from check_file_access() logic: this function is now not invoked at all in case of inline data.

 src/openvpn/crypto.c      |  25 +++--
 src/openvpn/crypto.h      |   5 +-
 src/openvpn/misc.c        |  17 ++-
 src/openvpn/misc.h        |  15 ++-
 src/openvpn/options.c     | 281 ++++++++++++++++++++++++++--------------------
 src/openvpn/options.h     |  21 ++--
 src/openvpn/plugin.c      |   6 +-
 src/openvpn/plugin.h      |   3 +-
 src/openvpn/push.c        |   2 +-
 src/openvpn/push.h        |   3 +-
 src/openvpn/ssl.c         |   6 +-
 src/openvpn/ssl_backend.h |  64 ++++++-----
 src/openvpn/ssl_common.h  |   2 +-
 src/openvpn/ssl_mbedtls.c |  63 +++++------
 src/openvpn/ssl_openssl.c |  91 ++++++++-------
 src/openvpn/tls_crypt.c   |   2 +-
 src/openvpn/tls_crypt.h   |   9 +-
 17 files changed, 343 insertions(+), 272 deletions(-)
src/openvpn/crypto.c +13 -12
index 944f4c5..9a7745f 100644
@@ -36,6 +36,7 @@
 #include "crypto.h"
 #include "error.h"
 #include "integer.h"
+#include "misc.h"
 #include "platform.h"
 
 #include "memdbg.h"
@@ -1165,27 +1166,26 @@ test_crypto(struct crypto_options *co, struct frame *frame)
 
 void
 crypto_read_openvpn_key(const struct key_type *key_type, struct key_ctx_bi *ctx,
-                        const char *key_file, const char *key_inline,
+                        const char *key_file, bool key_inline,
                         const int key_direction, const char *key_name,
                         const char *opt_name)
 {
     struct key2 key2;
     struct key_direction_state kds;
     char log_prefix[128] = { 0 };
+    unsigned int flags = RKF_MUST_SUCCEED;
 
     if (key_inline)
     {
-        read_key_file(&key2, key_inline, RKF_MUST_SUCCEED | RKF_INLINE);
-    }
-    else
-    {
-        read_key_file(&key2, key_file, RKF_MUST_SUCCEED);
+        flags |= RKF_INLINE;
     }
+    read_key_file(&key2, key_file, flags);
 
     if (key2.n != 2)
     {
         msg(M_ERR, "File '%s' does not have OpenVPN Static Key format.  Using "
-            "free-form passphrase file is not supported anymore.", key_file);
+            "free-form passphrase file is not supported anymore.",
+            print_if_inline(key_file, key_inline));
     }
 
     /* check for and fix highly unlikely key problems */
@@ -1225,7 +1225,6 @@ read_key_file(struct key2 *key2, const char *file, const unsigned int flags)
     struct buffer in;
     int fd, size;
     uint8_t hex_byte[3] = {0, 0, 0};
-    const char *error_filename = file;
 
     /* parse info */
     const unsigned char *cp;
@@ -1264,7 +1263,6 @@ read_key_file(struct key2 *key2, const char *file, const unsigned int flags)
         /* 'file' is a string containing ascii representation of key */
         size = strlen(file) + 1;
         buf_set_read(&in, (const uint8_t *)file, size);
-        error_filename = INLINE_FILE_TAG;
     }
     else
     {
@@ -1372,7 +1370,8 @@ read_key_file(struct key2 *key2, const char *file, const unsigned int flags)
                 {
                     msg(M_FATAL,
                         isprint(c) ? printable_char_fmt : unprintable_char_fmt,
-                        c, line_num, error_filename, count, onekeylen, keylen);
+                        c, line_num, print_if_inline(file, flags & RKF_INLINE),
+                        count, onekeylen, keylen);
                 }
             }
             ++line_index;
@@ -1394,14 +1393,16 @@ read_key_file(struct key2 *key2, const char *file, const unsigned int flags)
         {
             msg(M_FATAL,
                 "Insufficient key material or header text not found in file '%s' (%d/%d/%d bytes found/min/max)",
-                error_filename, count, onekeylen, keylen);
+                print_if_inline(file, flags & RKF_INLINE), count, onekeylen,
+                keylen);
         }
 
         if (state != PARSE_FINISHED)
         {
             msg(M_FATAL,
                 "Footer text not found in file '%s' (%d/%d/%d bytes found/min/max)",
-                error_filename, count, onekeylen, keylen);
+                print_if_inline(file, flags & RKF_INLINE), count, onekeylen,
+                keylen);
         }
     }
 
src/openvpn/crypto.h +2 -3
index 90f9857..e744c94 100644
@@ -490,9 +490,8 @@ void key2_print(const struct key2 *k,
 
 void crypto_read_openvpn_key(const struct key_type *key_type,
                              struct key_ctx_bi *ctx, const char *key_file,
-                             const char *key_inline,
-                             const int key_direction, const char *key_name,
-                             const char *opt_name);
+                             bool key_inline, const int key_direction,
+                             const char *key_name, const char *opt_name);
 
 /*
  * Inline functions
src/openvpn/misc.c +14 -3
index 87f03be..7658900 100644
@@ -1645,12 +1645,12 @@ make_arg_copy(char **p, struct gc_arena *gc)
 }
 
 const char **
-make_extended_arg_array(char **p, struct gc_arena *gc)
+make_extended_arg_array(char **p, bool is_inline, struct gc_arena *gc)
 {
     const int argc = string_array_len((const char **)p);
-    if (!strcmp(p[0], INLINE_FILE_TAG) && argc == 2)
+    if (is_inline)
     {
-        return make_inline_array(p[1], gc);
+        return make_inline_array(p[0], gc);
     }
     else if (argc == 0)
     {
@@ -1788,6 +1788,17 @@ compat_flag(unsigned int flag)
 
 }
 
+const char *
+print_if_inline(const char *str, bool is_inline)
+{
+    if (is_inline)
+    {
+        return INLINE_FILE_TAG;
+    }
+
+    return np(str);
+}
+
 #if P2MP_SERVER
 
 /* helper to parse peer_info received from multi client, validate
src/openvpn/misc.h +14 -1
index 16be621..9cb42e4 100644
@@ -173,7 +173,8 @@ const char **make_env_array(const struct env_set *es,
 
 const char **make_arg_array(const char *first, const char *parms, struct gc_arena *gc);
 
-const char **make_extended_arg_array(char **p, struct gc_arena *gc);
+const char **make_extended_arg_array(char **p, bool is_inline,
+                                     struct gc_arena *gc);
 
 /* an analogue to the random() function, but use OpenSSL functions if available */
 #ifdef ENABLE_CRYPTO
@@ -348,4 +349,16 @@ void output_peer_info_env(struct env_set *es, const char *peer_info);
 
 #endif /* P2MP_SERVER */
 
+/**
+ * To be used when printing a string that may contain inline data.
+ *
+ * If "is_inline" is true, return the inline tag.
+ * If "is_inline" is false and "str" is not NULL, return "str".
+ * Return the constant string "[NULL]" otherwise.
+ *
+ * @param str       the original string to return when is_inline is false
+ * @param is_inline true when str contains an inline data of some sort
+ */
+const char *print_if_inline(const char *str, bool is_inline);
+
 #endif /* ifndef MISC_H */
src/openvpn/options.c +161 -120

525 lines not on this page · raw message

src/openvpn/options.h +11 -10
index dc63aed..a14f2ab 100644
@@ -468,7 +468,7 @@ struct options
 #ifdef ENABLE_CRYPTO
     /* Cipher parms */
     const char *shared_secret_file;
-    const char *shared_secret_file_inline;
+    bool shared_secret_file_inline;
     int key_direction;
     const char *ciphername;
     bool ncp_enabled;
@@ -506,13 +506,13 @@ struct options
     const char *tls_export_cert;
     const char *crl_file;
 
-    const char *ca_file_inline;
-    const char *cert_file_inline;
-    const char *extra_certs_file_inline;
-    const char *crl_file_inline;
-    char *priv_key_file_inline;
-    const char *dh_file_inline;
-    const char *pkcs12_file_inline; /* contains the base64 encoding of pkcs12 file */
+    bool ca_file_inline;
+    bool cert_file_inline;
+    bool extra_certs_file_inline;
+    bool crl_file_inline;
+    bool priv_key_file_inline;
+    bool dh_file_inline;
+    bool pkcs12_file_inline;
 
     int ns_cert_type; /* set to 0, NS_CERT_CHECK_SERVER, or NS_CERT_CHECK_CLIENT */
     unsigned remote_cert_ku[MAX_PARMS];
@@ -559,11 +559,11 @@ struct options
 
     /* Shared secret used for TLS control channel authentication */
     const char *tls_auth_file;
-    const char *tls_auth_file_inline;
+    bool tls_auth_file_inline;
 
     /* Shared secret used for TLS control channel authenticated encryption */
     const char *tls_crypt_file;
-    const char *tls_crypt_inline;
+    bool tls_crypt_inline;
 
     /* Allow only one session */
     bool single_session;
@@ -640,6 +640,7 @@ struct options
 #define OPT_P_SOCKFLAGS       (1<<26)
 #define OPT_P_CONNECTION      (1<<27)
 #define OPT_P_PEER_ID         (1<<28)
+#define OPT_P_INLINE          (1<<29)
 
 #define OPT_P_DEFAULT   (~(OPT_P_INSTANCE|OPT_P_PULL_MODE))
 
src/openvpn/plugin.c +3 -3
index 17eb2d8..6e862d3 100644
@@ -160,13 +160,13 @@ plugin_option_list_new(struct gc_arena *gc)
     return ret;
 }
 
-bool
-plugin_option_list_add(struct plugin_option_list *list, char **p, struct gc_arena *gc)
+bool plugin_option_list_add(struct plugin_option_list *list, char **p,
+                            struct gc_arena *gc)
 {
     if (list->n < MAX_PLUGINS)
     {
         struct plugin_option *o = &list->plugins[list->n++];
-        o->argv = make_extended_arg_array(p, gc);
+        o->argv = make_extended_arg_array(p, false, gc);
         if (o->argv[0])
         {
             o->so_pathname = o->argv[0];
src/openvpn/plugin.h +2 -1
index 4ded529..938d6b3 100644
@@ -107,7 +107,8 @@ struct plugin_return
 
 struct plugin_option_list *plugin_option_list_new(struct gc_arena *gc);
 
-bool plugin_option_list_add(struct plugin_option_list *list, char **p, struct gc_arena *gc);
+bool plugin_option_list_add(struct plugin_option_list *list, char **p,
+                            struct gc_arena *gc);
 
 #ifndef ENABLE_SMALL
 void plugin_option_list_print(const struct plugin_option_list *list, int msglevel);
src/openvpn/push.c +1 -1
index c9c04a6..4d09207 100644
@@ -566,7 +566,7 @@ clone_push_list(struct options *o)
 void
 push_options(struct options *o, char **p, int msglevel, struct gc_arena *gc)
 {
-    const char **argv = make_extended_arg_array(p, gc);
+    const char **argv = make_extended_arg_array(p, false, gc);
     char *opt = print_argv(argv, gc, 0);
     push_option(o, opt, msglevel);
 }
src/openvpn/push.h +2 -1
index 86900c8..1f95266 100644
@@ -58,7 +58,8 @@ void clone_push_list(struct options *o);
 
 void push_option(struct options *o, const char *opt, int msglevel);
 
-void push_options(struct options *o, char **p, int msglevel, struct gc_arena *gc);
+void push_options(struct options *o, char **p, int msglevel,
+                  struct gc_arena *gc);
 
 void push_reset(struct options *o);
 
src/openvpn/ssl.c +3 -3
index de66325..9c9d777 100644
@@ -542,7 +542,7 @@ tls_version_parse(const char *vstr, const char *extra)
  */
 static void
 tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file,
-                   const char *crl_file_inline)
+                   bool crl_file_inline)
 {
     /* if something goes wrong with stat(), we'll store 0 as mtime */
     platform_stat_t crl_stat = {0};
@@ -645,8 +645,8 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
         {
             char *external_certificate = management_query_cert(management,
                                                                options->management_certificate);
-            tls_ctx_use_external_private_key(new_ctx, INLINE_FILE_TAG,
-                                             external_certificate);
+            tls_ctx_use_external_private_key(new_ctx, external_certificate,
+                                             true);
             free(external_certificate);
         }
     }
src/openvpn/ssl_backend.h +35 -29

137 lines not on this page · raw message

src/openvpn/ssl_common.h +1 -1
index 9a16d77..75b129f 100644
@@ -266,7 +266,7 @@ struct tls_options
     int verify_x509_type;
     const char *verify_x509_name;
     const char *crl_file;
-    const char *crl_file_inline;
+    bool crl_file_inline;
     int ns_cert_type;
     unsigned remote_cert_ku[MAX_PARMS];
     const char *remote_cert_eku;
src/openvpn/ssl_mbedtls.c +31 -32

166 lines not on this page · raw message

src/openvpn/ssl_openssl.c +44 -47

257 lines not on this page · raw message

src/openvpn/tls_crypt.c +1 -1
index c227b09..383c8ca 100644
@@ -44,7 +44,7 @@ tls_crypt_buf_overhead(void)
 
 void
 tls_crypt_init_key(struct key_ctx_bi *key, const char *key_file,
-                   const char *key_inline, bool tls_server) {
+                   bool key_inline, bool tls_server) {
     const int key_direction = tls_server ?
                               KEY_DIRECTION_NORMAL : KEY_DIRECTION_INVERSE;
 
src/openvpn/tls_crypt.h +5 -4
index 47f75d0..801127b 100644
@@ -92,13 +92,14 @@
  *
  * @param key           The key context to initialize
  * @param key_file      The file to read the key from (or the inline tag to
- *                      indicate and inline key).
- * @param key_inline    Array containing (zero-terminated) inline key, or NULL
- *                      if not used.
+ *                      indicate and inline key) or an array containing
+ *                      (zero-terminated) inline key.
+ * @param key_inline    True if key_file contains an inline key, False
+ *                      otherwise.
  * @param tls_server    Must be set to true is this is a TLS server instance.
  */
 void tls_crypt_init_key(struct key_ctx_bi *key, const char *key_file,
-                        const char *key_inline, bool tls_server);
+                        bool key_inline, bool tls_server);
 
 /**
  * Returns the maximum overhead (in bytes) added to the destination buffer by
Signature · 1 line
2.11.0
#1 lore raw

Hi,

Thanks, I like each version better. Even though it adds more lines than it removes, quite some of it is because of correcting wrapping and adding useful comments. I think it really improves the readability of the code. Some (hopefully final) minor comments still though:

Quoted 62 lines · Antonio Quartulli

Carrying around the INLINE_TAG is not really efficient, because it requires a strcmp() to be performed every time we want to understand if the data is stored inline or not.

Convert all the *_inline attributes to bool to make the logic easier and checks more efficient.

---

Based on master + [PATCH (master)] reformatting: fix style in crypto*.{c, h}

Changes from v2:
- fix indentation in ssl_openssl.c
- do not attempt to push inline'd options
- do not attempt to parse inline'd plugin
- introduce check_file_access_inline() and check_file_access_chroot_inline()
- introduce OPT_P_INLINE to specify when an option is allowed to be inline. Options not having this permission will fail to be parsed if is_inline is true.

Changes from v1:
- remove the INLINE_TAG from the options parsing logic at all. Now a boolean variable is passed around.
- add print_if_inline() helper function (to misc.c/h) to make sure we never print the inline data, but only the INLINE tag. Such function checks also for NULL pointers;
- make sure print_if_inline() is always used when printing possibly inline data;
- remove the INLINE_TAG from the options parsing logic at all. Now a boolean variable is passed around.
- fix alignment error in comment;
- remove CHKACC_INLINE from check_file_access() logic: this function is now not invoked at all in case of inline data.

 src/openvpn/crypto.c      |  25 +++--
 src/openvpn/crypto.h      |   5 +-
 src/openvpn/misc.c        |  17 ++-
 src/openvpn/misc.h        |  15 ++-
 src/openvpn/options.c     | 281 ++++++++++++++++++++++++++--------------------
 src/openvpn/options.h     |  21 ++--
 src/openvpn/plugin.c      |   6 +-
 src/openvpn/plugin.h      |   3 +-
 src/openvpn/push.c        |   2 +-
 src/openvpn/push.h        |   3 +-
 src/openvpn/ssl.c         |   6 +-
 src/openvpn/ssl_backend.h |  64 ++++++-----
 src/openvpn/ssl_common.h  |   2 +-
 src/openvpn/ssl_mbedtls.c |  63 +++++------
 src/openvpn/ssl_openssl.c |  91 ++++++++-------
 src/openvpn/tls_crypt.c   |   2 +-
 src/openvpn/tls_crypt.h   |   9 +-
 17 files changed, 343 insertions(+), 272 deletions(-)
src/openvpn/crypto.c +0 -0
index 944f4c5..9a7745f 100644

src/openvpn/crypto.c line 39

Quoted 4 lines
src/openvpn/crypto.c +1 -0
@@ -36,6 +36,7 @@
 #include "crypto.h"
 #include "error.h"
 #include "integer.h"
+#include "misc.h"

Adding a dependency to misc.o breaks the unit tests. Just try to run 'make check' now (after initializing the cmocka submodule).

I see that this added dependency is because you added 'print_if_inline()' in misc.c. It seems to be used only to print key filenames though. What about renaming it to print_key_filename() and moving it to crypto.c?

Quoted 22 lines
src/openvpn/crypto.c +22 -0
+/*
+ * A wrapper for check_file_access_chroot() that returns false immediately if
+ * the file is inline (and therefore there is no access to check)
+ */
+static bool
+check_file_access_chroot_inline(bool is_inline, const char *chroot,
+                                const int type, const char *file,
+                                const int mode, const char *opt)
+{
+    if (is_inline)
+    {
+        return false;
+    }
+
+    return check_file_access_chroot(chroot, type, file, mode, opt);
+}
+
+/*
+ * A wrapper for check_file_access() that returns false immediately if the file
+ * is inline (and therefore there is no access to check)
+ */
+static bool

src/openvpn/crypto.c

Quoted 10 lines
src/openvpn/crypto.c +10 -0
+check_file_access_inline(bool is_inline, const int type, const char *file,
+                         const int mode, const char *opt)
+{
+    if (is_inline)
+    {
+        return false;
+    }
+
+    return check_file_access(type, file, mode, opt);
+}

If you start these with /**, rather than /*, they will end up nicely in the doxygen.

Quoted 17 lines

static bool

src/openvpn/crypto.c +5 -1
-check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
+check_inline_file(struct in_src *is, char *p[], bool *is_inline,
+                  struct gc_arena *gc)
 {
     bool ret = false;
+
+    *is_inline = false;
     if (p[0] && !p[1])
     {
         char *arg = p[0];
         if (arg[0] == '<' && arg[strlen(arg)-1] == '>')
         {
             struct buffer close_tag;
+            *is_inline = true;
             arg[strlen(arg)-1] = '\0';
             p[0] = string_alloc(arg+1, gc);

src/openvpn/crypto.c

Quoted 10 lines
src/openvpn/crypto.c +2 -3
-            p[1] = string_alloc(INLINE_FILE_TAG, gc);
             close_tag = alloc_buf(strlen(p[0]) + 4);
             buf_printf(&close_tag, "</%s>", p[0]);
-            p[2] = read_inline_file(is, BSTR(&close_tag), gc);
-            p[3] = NULL;
+            p[1] = read_inline_file(is, BSTR(&close_tag), gc);
+            p[2] = NULL;
             free_buf(&close_tag);
             ret = true;
         }

This function now returns the same value as is set to *is_line. I think we should just use the return value. (For some reason the return value wasn't used anywhere previously, but let's just do so now.)

src/openvpn/crypto.c read_config_file(struct options *options, · line 4564

Quoted 12 lines

static void
add_option(struct options *options,

            char *p[],
src/openvpn/crypto.c +2 -0
+           bool is_inline,
            const char *file,
            int line,
            const int level,
@@ -4506,6 +4561,7 @@ read_config_file(struct options *options,
                  struct env_set *es)
 {
     const int max_recursive_levels = 10;
+    bool is_inline;

is_inline is only used below, in the if(parse_line()) scope. I think it should be declared in that scope.

Quoted 25 lines
src/openvpn/crypto.c +5 -2
     FILE *fp;
     int line_num;
     char line[OPTION_LINE_SIZE+1];
@@ -4544,8 +4600,10 @@ read_config_file(struct options *options,
                 if (parse_line(line + offset, p, SIZE(p), file, line_num, msglevel, &options->gc))
                 {
                     bypass_doubledash(&p[0]);
-                    check_inline_file_via_fp(fp, p, &options->gc);
-                    add_option(options, p, file, line_num, level, msglevel, permission_mask, option_types_found, es);
+                    check_inline_file_via_fp(fp, p, &is_inline, &options->gc);
+                    add_option(options, p, is_inline, file, line_num, level,
+                               msglevel, permission_mask, option_types_found,
+                               es);
                 }
             }
             if (fp != stdin)
@@ -4578,6 +4636,7 @@ read_config_string(const char *prefix,
     char line[OPTION_LINE_SIZE];
     struct buffer multiline;
     int line_num = 0;
+    bool is_inline;
     buf_set_read(&multiline, (uint8_t *)config, strlen(config));
 
@@ -4589,8 +4648,9 @@ read_config_string(const char *prefix,
         if (parse_line(line, p, SIZE(p), prefix, line_num, msglevel, &options->gc))

src/openvpn/crypto.c read_config_string(const char *prefix, · line 4656

Quoted 10 lines
src/openvpn/crypto.c +3 -2
         {
             bypass_doubledash(&p[0]);
-            check_inline_file_via_buf(&multiline, p, &options->gc);
-            add_option(options, p, prefix, line_num, 0, msglevel, permission_mask, option_types_found, es);
+            check_inline_file_via_buf(&multiline, p, &is_inline, &options->gc);
+            add_option(options, p, is_inline, prefix, line_num, 0, msglevel,
+                       permission_mask, option_types_found, es);
         }
         CLEAR(p);
     }

Same as above, move is_inline to the scope where it's used.

src/openvpn/plugin.c plugin_option_list_new(struct gc_arena *gc) · line 164

Quoted 10 lines
src/openvpn/plugin.c +2 -2
@@ -160,13 +160,13 @@ plugin_option_list_new(struct gc_arena *gc)
     return ret;
 }
 
-bool
-plugin_option_list_add(struct plugin_option_list *list, char **p, struct gc_arena *gc)
+bool plugin_option_list_add(struct plugin_option_list *list, char **p,
+                            struct gc_arena *gc)

The return type should stay on it's own line.

-Steffan

#2 lore raw
Quoted 13 lines · Steffan Karger

src/openvpn/crypto.c line 39

Quoted 9 lines
src/openvpn/crypto.c +1 -0
index 944f4c5..9a7745f 100644
@@ -36,6 +36,7 @@
 #include "crypto.h"
 #include "error.h"
 #include "integer.h"
+#include "misc.h"

Adding a dependency to misc.o breaks the unit tests. Just try to run 'make check' now (after initializing the cmocka submodule).

Quoted 4 lines

I see that this added dependency is because you added 'print_if_inline()' in misc.c. It seems to be used only to print key filenames though. What about renaming it to print_key_filename() and moving it to crypto.c?

Yeah, makes sense. I'll move it to crypto.c

Quoted 28 lines
Quoted 28 lines
src/openvpn/crypto.c +27 -0
+/*
+ * A wrapper for check_file_access_chroot() that returns false immediately if
+ * the file is inline (and therefore there is no access to check)
+ */
+static bool
+check_file_access_chroot_inline(bool is_inline, const char *chroot,
+                                const int type, const char *file,
+                                const int mode, const char *opt)
+{
+    if (is_inline)
+    {
+        return false;
+    }
+
+    return check_file_access_chroot(chroot, type, file, mode, opt);
+}
+
+/*
+ * A wrapper for check_file_access() that returns false immediately if the file
+ * is inline (and therefore there is no access to check)
+ */
+static bool
+check_file_access_inline(bool is_inline, const int type, const char *file,
+                         const int mode, const char *opt)
+{
+    if (is_inline)
+    {
Quoted 8 lines

src/openvpn/crypto.c

Quoted 5 lines
src/openvpn/crypto.c +5 -0
+        return false;
+    }
+
+    return check_file_access(type, file, mode, opt);
+}

If you start these with /**, rather than /*, they will end up nicely in the doxygen.

Yeah .. I usually do it, but this time I forgot :) will fix that.

Quoted 24 lines
Quoted 24 lines

static bool

src/openvpn/crypto.c +6 -4
-check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
+check_inline_file(struct in_src *is, char *p[], bool *is_inline,
+                  struct gc_arena *gc)
 {
     bool ret = false;
+
+    *is_inline = false;
     if (p[0] && !p[1])
     {
         char *arg = p[0];
         if (arg[0] == '<' && arg[strlen(arg)-1] == '>')
         {
             struct buffer close_tag;
+            *is_inline = true;
             arg[strlen(arg)-1] = '\0';
             p[0] = string_alloc(arg+1, gc);
-            p[1] = string_alloc(INLINE_FILE_TAG, gc);
             close_tag = alloc_buf(strlen(p[0]) + 4);
             buf_printf(&close_tag, "</%s>", p[0]);
-            p[2] = read_inline_file(is, BSTR(&close_tag), gc);
-            p[3] = NULL;
+            p[1] = read_inline_file(is, BSTR(&close_tag), gc);
Quoted 8 lines
Quoted 4 lines
+            p[2] = NULL;
             free_buf(&close_tag);
             ret = true;
         }

This function now returns the same value as is set to *is_line. I think we should just use the return value. (For some reason the return value wasn't used anywhere previously, but let's just do so now.)

eheh nice catch. I will remove the parameter and re-use the return value.

Quoted 16 lines

src/openvpn/crypto.c read_config_file(struct options *options, · line 4564

Quoted 13 lines

static void
add_option(struct options *options,

            char *p[],
src/openvpn/crypto.c +2 -0
+           bool is_inline,
            const char *file,
            int line,
            const int level,
@@ -4506,6 +4561,7 @@ read_config_file(struct options *options,
                  struct env_set *es)
 {
     const int max_recursive_levels = 10;
+    bool is_inline;

is_inline is only used below, in the if(parse_line()) scope. I think it should be declared in that scope.

oh ok. I am not used to that, so I completely missed it. will do!

Quoted 24 lines
Quoted 24 lines
src/openvpn/crypto.c +5 -2
     FILE *fp;
     int line_num;
     char line[OPTION_LINE_SIZE+1];
@@ -4544,8 +4600,10 @@ read_config_file(struct options *options,
                 if (parse_line(line + offset, p, SIZE(p), file, line_num, msglevel, &options->gc))
                 {
                     bypass_doubledash(&p[0]);
-                    check_inline_file_via_fp(fp, p, &options->gc);
-                    add_option(options, p, file, line_num, level, msglevel, permission_mask, option_types_found, es);
+                    check_inline_file_via_fp(fp, p, &is_inline, &options->gc);
+                    add_option(options, p, is_inline, file, line_num, level,
+                               msglevel, permission_mask, option_types_found,
+                               es);
                 }
             }
             if (fp != stdin)
@@ -4578,6 +4636,7 @@ read_config_string(const char *prefix,
     char line[OPTION_LINE_SIZE];
     struct buffer multiline;
     int line_num = 0;
+    bool is_inline;
     buf_set_read(&multiline, (uint8_t *)config, strlen(config));
 
Quoted 14 lines

src/openvpn/crypto.c read_config_string(const char *prefix, · line 4656

Quoted 12 lines
src/openvpn/crypto.c +3 -2
@@ -4589,8 +4648,9 @@ read_config_string(const char *prefix,
         if (parse_line(line, p, SIZE(p), prefix, line_num, msglevel, &options->gc))
         {
             bypass_doubledash(&p[0]);
-            check_inline_file_via_buf(&multiline, p, &options->gc);
-            add_option(options, p, prefix, line_num, 0, msglevel, permission_mask, option_types_found, es);
+            check_inline_file_via_buf(&multiline, p, &is_inline, &options->gc);
+            add_option(options, p, is_inline, prefix, line_num, 0, msglevel,
+                       permission_mask, option_types_found, es);
         }
         CLEAR(p);
     }

Same as above, move is_inline to the scope where it's used.

copy that!

Quoted 7 lines
Quoted 7 lines
src/openvpn/plugin.c +0 -0
@@ -160,13 +160,13 @@ plugin_option_list_new(struct gc_arena *gc)
     return ret;
 }
 
Quoted 7 lines

src/openvpn/plugin.c plugin_option_list_new(struct gc_arena *gc) · line 164

Quoted 4 lines
src/openvpn/plugin.c +2 -2
-bool
-plugin_option_list_add(struct plugin_option_list *list, char **p, struct gc_arena *gc)
+bool plugin_option_list_add(struct plugin_option_list *list, char **p,
+                            struct gc_arena *gc)

The return type should stay on it's own line.

argh, my bad again!

Will fix these minor glitches and send v4!

Thank you for the feedback!

Cheers,

Signature · 1 line
Antonio Quartulli