[vlc-devel] [PATCH 07/13] memory_file: split into 2 submodules

Thomas Guillem thomas at gllm.fr
Wed Feb 24 14:25:16 CET 2016


The first module is the main memory keystore, the second one is the file
keystore.

The second module will be always deactivated by default and is not secure.
---
 modules/keystore/memory-file.c | 279 ++++++++++++++++++++++++++++-------------
 test/modules/keystore/test.c   |  88 ++++++++-----
 2 files changed, 248 insertions(+), 119 deletions(-)

diff --git a/modules/keystore/memory-file.c b/modules/keystore/memory-file.c
index 7a02016..7f6c2c8 100644
--- a/modules/keystore/memory-file.c
+++ b/modules/keystore/memory-file.c
@@ -24,6 +24,7 @@
 
 #include <stdarg.h>
 #include <stdio.h>
+#include <sys/stat.h>
 #ifdef HAVE_FLOCK
 #include <sys/file.h>
 #endif
@@ -40,14 +41,26 @@
 static int Open(vlc_object_t *);
 static void Close(vlc_object_t *);
 
+static int OpenFile(vlc_object_t *);
+static void CloseFile(vlc_object_t *);
+
 vlc_module_begin()
-    set_shortname(N_("memory keystore (insecure)"))
-    set_description(N_("secrets are stored in memory without any encryption"))
+    set_shortname(N_("memory keystore"))
+    set_description(N_("secrets are stored in memory"))
     set_category(CAT_ADVANCED)
     set_subcategory(SUBCAT_ADVANCED_MISC)
-    add_string("keystore-file", NULL, NULL, NULL, false )
     set_capability("keystore", 0)
     set_callbacks(Open, Close)
+    add_shortcut("memory")
+    add_submodule()
+        set_shortname(N_("file keystore (plaintext)"))
+        set_description(N_("secrets are stored on a file without any encryption"))
+        set_category(CAT_ADVANCED)
+        set_subcategory(SUBCAT_ADVANCED_MISC)
+        set_callbacks(OpenFile, CloseFile)
+        add_string("keystore-file", NULL, NULL, NULL, false )
+        set_capability("keystore", 0)
+        add_shortcut("file-plaintext")
 vlc_module_end ()
 
 struct list
@@ -59,16 +72,10 @@ struct list
 
 struct vlc_keystore_sys
 {
-    char *      psz_file;
-    FILE *      p_file;
-    int         i_fd;
-    struct list list;
-    bool        b_error;
+    struct list     list;
+    char *          psz_file;
 };
 
-static int list_save(vlc_keystore_sys *p_sys, struct list *p_list);
-static int list_read(vlc_keystore_sys *p_sys, struct list *p_list);
-
 static void
 list_free(struct list *p_list)
 {
@@ -171,7 +178,7 @@ Store(vlc_keystore *p_keystore, const char *const ppsz_values[KEY_MAX],
     if (vlc_keystore_entry_set_secret(p_entry, p_secret, i_secret_len))
         return VLC_EGENERIC;
 
-    return list_save(p_sys, &p_sys->list);
+    return VLC_SUCCESS;
 }
 
 static unsigned int
@@ -222,12 +229,36 @@ Remove(vlc_keystore *p_keystore, const char *const ppsz_values[KEY_MAX])
         i_count++;
     }
 
-    if (list_save(p_sys, &p_sys->list) != VLC_SUCCESS)
-        return 0;
-
     return i_count;
 }
 
+static void
+Close(vlc_object_t *p_this)
+{
+    vlc_keystore *p_keystore = (vlc_keystore *)p_this;
+    vlc_keystore_sys *p_sys = p_keystore->p_sys;
+
+    list_free(&p_sys->list);
+    free(p_sys);
+}
+
+static int
+Open(vlc_object_t *p_this)
+{
+    vlc_keystore *p_keystore = (vlc_keystore *)p_this;
+
+    p_keystore->p_sys = calloc(1, sizeof(vlc_keystore_sys));
+    if (!p_keystore->p_sys)
+        return VLC_EGENERIC;
+
+    p_keystore->pf_store = Store;
+    p_keystore->pf_find = Find;
+    p_keystore->pf_remove = Remove;
+    p_keystore->b_secure = true;
+
+    return VLC_SUCCESS;
+}
+
 static const char *const ppsz_keys[] = {
     "protocol",
     "user",
@@ -302,13 +333,17 @@ truncate0(int i_fd)
 
 /* a line is "{key1:VALUE1_B64,key2:VALUE2_B64}:PASSWORD_B64" */
 static int
-list_save(vlc_keystore_sys *p_sys, struct list *p_list)
+file_save(vlc_keystore *p_keystore, FILE *p_file, int i_fd, struct list *p_list)
 {
+    vlc_keystore_sys *p_sys = p_keystore->p_sys;
     int i_ret = VLC_EGENERIC;
-    rewind(p_sys->p_file);
 
-    if (truncate0(p_sys->i_fd))
-        goto end;
+    rewind(p_file);
+    if (truncate0(i_fd))
+    {
+        vlc_unlink(p_sys->psz_file);
+        return i_ret;
+    }
 
     for (unsigned int i = 0; i < p_list->i_count; ++i)
     {
@@ -316,22 +351,21 @@ list_save(vlc_keystore_sys *p_sys, struct list *p_list)
         if (!p_entry->p_secret)
             continue;
 
-        if (str_write(p_sys->p_file, "{"))
+        if (str_write(p_file, "{"))
             goto end;
-        if (values_write(p_sys->p_file,
-                         (const char *const *) p_entry->ppsz_values))
+        if (values_write(p_file, (const char *const *) p_entry->ppsz_values))
             goto end;
-        if (str_write(p_sys->p_file, "}:"))
+        if (str_write(p_file, "}:"))
             goto end;
         char *psz_b64 = vlc_b64_encode_binary(p_entry->p_secret,
                                               p_entry->i_secret_len);
-        if (!psz_b64 || str_write(p_sys->p_file, psz_b64))
+        if (!psz_b64 || str_write(p_file, psz_b64))
         {
             free(psz_b64);
             goto end;
         }
         free(psz_b64);
-        if (i < p_list->i_count - 1 && str_write(p_sys->p_file, "\n"))
+        if (i < p_list->i_count - 1 && str_write(p_file, "\n"))
             goto end;
     }
     i_ret = VLC_SUCCESS;
@@ -339,25 +373,29 @@ end:
 
     if (i_ret != VLC_SUCCESS)
     {
-        p_sys->b_error = true;
-        list_free(p_list);
+        if (truncate0(i_fd))
+            vlc_unlink(p_sys->psz_file);
     }
     return i_ret;
 }
 
 static int
-list_read(vlc_keystore_sys *p_sys, struct list *p_list)
+file_read(vlc_keystore *p_keystore, FILE *p_file, int i_fd, struct list *p_list)
 {
+    vlc_keystore_sys *p_sys = p_keystore->p_sys;
     char *psz_line = NULL;
     size_t i_line_len = 0;
     ssize_t i_read;
     bool b_valid = false;
 
-    while ((i_read = getline(&psz_line, &i_line_len, p_sys->p_file)) != -1)
+    while ((i_read = getline(&psz_line, &i_line_len, p_file)) != -1)
     {
         char *p = psz_line;
         if (*(p++) != '{')
+        {
+            getchar();
             goto end;
+        }
 
         vlc_keystore_entry *p_entry = list_new_entry(p_list);
         if (!p_entry)
@@ -413,96 +451,165 @@ end:
     free(psz_line);
     if (!b_valid)
     {
-        p_sys->b_error = true;
-        list_free(p_list);
+        if (truncate0(i_fd))
+            vlc_unlink(p_sys->psz_file);
     }
     return VLC_SUCCESS;
 }
 
-
-static void
-Close(vlc_object_t *p_this)
+static int
+file_open(const char *psz_file, const char *psz_mode, FILE **pp_file)
 {
-    vlc_keystore *p_keystore = (vlc_keystore *)p_this;
-    vlc_keystore_sys *p_sys = p_keystore->p_sys;
+    FILE *p_file = vlc_fopen(psz_file, psz_mode);
+    if (p_file == NULL)
+        return -1;
+
+    int i_fd = fileno(p_file);
+    if (i_fd == -1)
+        return -1;
 
-    if (p_sys->p_file)
+#ifdef HAVE_FLOCK
+    if (flock(i_fd, LOCK_EX) != 0)
     {
-        if (p_sys->b_error)
-        {
-            if (p_sys->i_fd != -1 && truncate0(p_sys->i_fd))
-                vlc_unlink(p_sys->psz_file);
-        }
+        fclose(p_file);
+    }
+#endif
+    *pp_file = p_file;
+    return i_fd;
+}
 
+static void
+file_close(FILE *p_file, int i_fd)
+{
 #ifdef HAVE_FLOCK
-        if (p_sys->i_fd != -1)
-            flock(p_sys->i_fd, LOCK_UN);
+    flock(i_fd, LOCK_UN);
 #endif
-        fclose(p_sys->p_file);
-    }
+    fclose(p_file);
+}
+
+static int
+StoreFile(vlc_keystore *p_keystore, const char *const ppsz_values[KEY_MAX],
+          const uint8_t *p_secret, size_t i_secret_len, const char *psz_label)
+{
+    vlc_keystore_sys *p_sys = p_keystore->p_sys;
+    int i_ret = VLC_EGENERIC;
+    FILE *p_file;
+    int i_fd = file_open(p_sys->psz_file, "r+", &p_file);
+    if (i_fd == -1)
+        return i_ret;
+
+    if (file_read(p_keystore, p_file, i_fd, &p_sys->list) != VLC_SUCCESS)
+        goto end;
+
+    i_ret = Store(p_keystore, ppsz_values, p_secret, i_secret_len, psz_label);
+    if (i_ret == VLC_SUCCESS)
+        i_ret = file_save(p_keystore, p_file, i_fd, &p_sys->list);
 
+end:
+    file_close(p_file, i_fd);
+    list_free(&p_sys->list);
+    return i_ret;
+}
+
+static unsigned int
+FindFile(vlc_keystore *p_keystore, const char *const ppsz_values[KEY_MAX],
+         vlc_keystore_entry **pp_entries)
+{
+    vlc_keystore_sys *p_sys = p_keystore->p_sys;
+    unsigned int i_ret = 0;
+    FILE *p_file;
+    int i_fd = file_open(p_sys->psz_file, "r", &p_file);
+    if (i_fd == -1)
+        return i_ret;
+
+    if (file_read(p_keystore, p_file, i_fd, &p_sys->list) != VLC_SUCCESS)
+        goto end;
+
+    i_ret = Find(p_keystore, ppsz_values, pp_entries);
+
+end:
+    file_close(p_file, i_fd);
+    list_free(&p_sys->list);
+    return i_ret;
+}
+
+static unsigned int
+RemoveFile(vlc_keystore *p_keystore, const char *const ppsz_values[KEY_MAX])
+{
+    vlc_keystore_sys *p_sys = p_keystore->p_sys;
+    unsigned int i_ret = 0;
+    FILE *p_file;
+    int i_fd = file_open(p_sys->psz_file, "r+", &p_file);
+    if (i_fd == -1)
+        return i_ret;
+
+    if (file_read(p_keystore, p_file, i_fd, &p_sys->list) != VLC_SUCCESS)
+        goto end;
+
+    i_ret = Remove(p_keystore, ppsz_values);
+
+    if (i_ret > 0
+     && file_save(p_keystore, p_file, i_fd, &p_sys->list) != VLC_SUCCESS)
+        i_ret = 0;
+
+end:
+    file_close(p_file, i_fd);
     list_free(&p_sys->list);
+    return i_ret;
+}
+
+static void
+CloseFile(vlc_object_t *p_this)
+{
+    vlc_keystore *p_keystore = (vlc_keystore *)p_this;
+    vlc_keystore_sys *p_sys = p_keystore->p_sys;
+
     free(p_sys->psz_file);
     free(p_sys);
 }
 
 static int
-Open(vlc_object_t *p_this)
+OpenFile(vlc_object_t *p_this)
 {
+    int i_ret = Open(p_this);
+
+    if (i_ret != VLC_SUCCESS)
+        return i_ret;
+
     vlc_keystore *p_keystore = (vlc_keystore *)p_this;
-    vlc_keystore_sys *p_sys;
+    vlc_keystore_sys *p_sys = p_keystore->p_sys;
 
-    char *psz_file = var_InheritString(p_this, "keystore-memory-file");
+    char *psz_file = var_InheritString(p_this, "keystore-file");
     if (!psz_file)
-        return VLC_EGENERIC;
-
-    p_sys = calloc(1, sizeof(vlc_keystore_sys));
-    if (!p_sys)
     {
-        free(psz_file);
+        free(p_sys);
         return VLC_EGENERIC;
     }
 
     p_sys->psz_file = psz_file;
-    p_sys->p_file = vlc_fopen(p_sys->psz_file, "a+");
-    p_sys->i_fd = -1;
-
-    if (!p_sys->p_file)
+    struct stat stat;
+    bool b_file_exists = false;
+    if (vlc_stat(p_sys->psz_file, &stat) != 0)
     {
-        Close(p_sys);
-        return VLC_EGENERIC;
-    }
-
-    int i_fd = fileno(p_sys->p_file);
-    if (i_fd == -1)
-    {
-        Close(p_sys);
-        return VLC_EGENERIC;
-    }
-
-    /* Fail if an other LibVLC process acquired the file lock.
-     * If HAVE_FLOCK is not defined, the running OS is most likely Windows
-     * and a lock was already acquired when the file was opened. */
-#ifdef HAVE_FLOCK
-    if (flock(i_fd, LOCK_EX|LOCK_NB) != 0)
-    {
-        Close(p_sys);
-        return VLC_EGENERIC;
+        FILE *p_file = vlc_fopen(p_sys->psz_file, "a+");
+        if (p_file != NULL)
+        {
+            b_file_exists= true;
+            fclose(p_file);
+        }
     }
-#endif
-    p_sys->i_fd = i_fd;
+    else
+        b_file_exists = true;
 
-    if (list_read(p_sys, &p_sys->list) != VLC_SUCCESS)
+    if (!b_file_exists)
     {
-        Close(p_sys);
+        CloseFile(p_this);
         return VLC_EGENERIC;
     }
 
-    p_keystore->p_sys = p_sys;
-
-    p_keystore->pf_store = Store;
-    p_keystore->pf_find = Find;
-    p_keystore->pf_remove = Remove;
+    p_keystore->pf_store = StoreFile;
+    p_keystore->pf_find = FindFile;
+    p_keystore->pf_remove = RemoveFile;
     p_keystore->b_secure = false;
 
     return VLC_SUCCESS;
diff --git a/test/modules/keystore/test.c b/test/modules/keystore/test.c
index aa5497b..2b90239 100644
--- a/test/modules/keystore/test.c
+++ b/test/modules/keystore/test.c
@@ -44,15 +44,18 @@
 static const struct
 {
     const char *psz_module;
+    const char *psz_shortcut;
     bool b_test_default;
+    bool b_persistant;
 } keystore_args[] =
 {
-    { "memory_file", true },
+    { "memory_file", "memory", true, false },
+    { "memory_file", "file-plaintext", true, true },
     /* Following keystores are tested only when asked explicitly by the tester
      * (with "-a" argv) */
-    { "secret", false },
-    { "kwallet", false },
-    { "keychain", false }
+    { "secret", "secret", false, true },
+    { "kwallet", "kwallet", false, true },
+    { "keychain", "keychain", false, true }
 };
 
 static void
@@ -108,7 +111,7 @@ ks_store(vlc_keystore *p_keystore, const char *const ppsz_values[KEY_MAX],
 }
 
 static void
-test_module(const char *psz_module, bool b_test_all,
+test_module(const char *psz_shortcut, bool b_test_all, bool b_persistant,
             int argc, const char * const *argv)
 {
 #define VALUES_INSERT(i_key, psz_value) ppsz_values[i_key] = psz_value
@@ -121,7 +124,7 @@ test_module(const char *psz_module, bool b_test_all,
 } while(0)
 #define KS_STORE() ks_store(p_keystore, ppsz_values, (const uint8_t *)psz_secret, -1)
 
-    printf("\n== Testing %s keystore module ==\n\n", psz_module);
+    printf("\n== Testing %s keystore module ==\n\n", psz_shortcut);
 
     printf("creating libvlc\n");
     libvlc_instance_t *p_libvlc = libvlc_new(argc, argv);
@@ -130,7 +133,7 @@ test_module(const char *psz_module, bool b_test_all,
     vlc_interrupt_t *ctx = vlc_interrupt_create();
     assert(ctx != NULL);
 
-    printf("creating %s keystore\n", psz_module);
+    printf("creating %s keystore\n", psz_shortcut);
     vlc_keystore *p_keystore = vlc_keystore_create(p_libvlc->p_libvlc_int);
     assert(p_keystore);
 
@@ -173,34 +176,52 @@ test_module(const char *psz_module, bool b_test_all,
     KS_FIND();
     assert(i_entries == 2);
 
-    printf("testing that entries are still present after a module unload\n");
-    vlc_keystore_release(p_keystore);
-    p_keystore = vlc_keystore_create(p_libvlc->p_libvlc_int);
-    assert(p_keystore);
-    KS_FIND();
-    assert(i_entries == 2);
+    if (b_persistant)
+    {
+        printf("testing that entries are still present after a module unload\n");
+        vlc_keystore_release(p_keystore);
+        p_keystore = vlc_keystore_create(p_libvlc->p_libvlc_int);
+        assert(p_keystore);
+        KS_FIND();
+        assert(i_entries == 2);
+    }
 
-    printf("testing adding a third entry from a second running instance\n");
     VALUES_REINIT();
     VALUES_INSERT(KEY_PROTOCOL, "smb");
     VALUES_INSERT(KEY_SERVER, "example");
     VALUES_INSERT(KEY_PATH, "/example.mkv");
     VALUES_INSERT(KEY_USER, "user1");
-    KS_FIND();
-    assert(i_entries == 0);
 
-    vlc_keystore *old_keystore = p_keystore;
-    p_keystore = vlc_keystore_create(p_libvlc->p_libvlc_int);
-    assert(p_keystore);
+    if (b_persistant)
+    {
+        printf("testing adding a third entry from a second running instance\n");
 
-    KS_STORE();
-    KS_FIND();
-    assert(i_entries == 1);
+        KS_FIND();
+        assert(i_entries == 0);
+
+        vlc_keystore *old_keystore = p_keystore;
+        p_keystore = vlc_keystore_create(p_libvlc->p_libvlc_int);
+        assert(p_keystore);
+
+        KS_STORE();
+        KS_FIND();
+        assert(i_entries == 1);
+
+        vlc_keystore_release(p_keystore);
+        p_keystore = old_keystore;
+        KS_FIND();
+        assert(i_entries == 1);
+    }
+    else
+    {
+        printf("testing adding a third entry\n");
+        KS_FIND();
+        assert(i_entries == 0);
+        KS_STORE();
+        KS_FIND();
+        assert(i_entries == 1);
+    }
 
-    vlc_keystore_release(p_keystore);
-    p_keystore = old_keystore;
-    KS_FIND();
-    assert(i_entries == 1);
 
     printf("testing adding a fourth entry (without user/path)\n");
     VALUES_REINIT();
@@ -231,7 +252,7 @@ test_module(const char *psz_module, bool b_test_all,
     KS_FIND();
     assert(i_entries == 4);
 
-    if (b_test_all)
+    if (b_test_all && b_persistant)
     {
         printf("\nPress ENTER to remove entries\n");
         getchar();
@@ -284,6 +305,7 @@ main(int i_argc, char *ppsz_argv[])
     for (unsigned int i = 0; i < sizeof(keystore_args)/sizeof(*keystore_args); ++i)
     {
         const char *psz_module = keystore_args[i].psz_module;
+        const char *psz_shortcut = keystore_args[i].psz_shortcut;
 
         if ((b_test_all || keystore_args[i].b_test_default)
          && module_exists(psz_module))
@@ -294,24 +316,24 @@ main(int i_argc, char *ppsz_argv[])
             char psz_tmp_path[] = "/tmp/libvlc_XXXXXX";
 
             assert(asprintf(&ppsz_vlc_argv[0], "--keystore=%s,none",
-                   psz_module) != -1);
+                            psz_shortcut) != -1);
 
-            if (strcmp(psz_module, "plaintext") == 0)
+            if (strcmp(psz_shortcut, "file-plaintext") == 0)
             {
                 assert((i_tmp_fd = mkstemp(psz_tmp_path)) != -1);
                 printf("plaintext tmp file: '%s'\n", psz_tmp_path);
                 assert(asprintf(&ppsz_vlc_argv[1],
-                       "--keystore-plaintext-file=%s", psz_tmp_path) != -1);
+                       "--keystore-file=%s", psz_tmp_path) != -1);
                 i_vlc_argc++;
             }
-            else if (strcmp(psz_module, "kwallet") == 0)
+            else if (strcmp(psz_shortcut, "kwallet") == 0)
             {
                 /* See TODO in kwallet.cpp, VLCKWallet::connect() */
                 assert(libvlc_InternalAddIntf(p_libvlc->p_libvlc_int, "qt") == VLC_SUCCESS);
             }
 
-            test_module(psz_module, b_test_all, i_vlc_argc,
-                        (const char * const *)ppsz_vlc_argv);
+            test_module(psz_shortcut, b_test_all, keystore_args[i].b_persistant,
+                        i_vlc_argc, (const char * const *)ppsz_vlc_argv);
 
             if (i_tmp_fd != -1)
             {
-- 
2.7.0



More information about the vlc-devel mailing list