[libubootenv] uboot_env: Fix handling of .flags variable when it contains variables which are not created yet
diff mbox series

Message ID 3a0d972a-f06b-40ca-8cae-fa52af360afb@googlegroups.com
State New
Headers show
Series
  • [libubootenv] uboot_env: Fix handling of .flags variable when it contains variables which are not created yet
Related show

Commit Message

Uwe Schuster Aug. 2, 2019, 11:49 a.m. UTC
This tries to fix the problem with variable access or type flags which are 
part of the .flags variable but do not exist as a env variable as reported 
by me in https://groups.google.com/d/msg/swupdate/ShdD10dl7Pc/ULUYSpFDCAAJ:

1. The write once (o) access atrribute flag is not properly handled if the 
> variable referenced by the .flags value string does not exist yet (as it 
> would be the case with write once variables before they are first written 
> or set). Instead this variables flags gets removed from .flags value and 
> attributes are lost.
> This behavior also applies to all other variables which do not exist yet, 
> but in case of write once variables I'd consider it a normal use case.
>

I had to choose an implementation and I chose to put such uncreated 
variables into the metadata database as normal variables but with NULL 
value assigned.

The patch also fixes a problem with the existing parsing function. It 
failed to handle .flags value strings with more then one variable (comma 
separated).

The only known problem what remains is with fw_printenv function. It does 
currently not print out the .flags variable (as normal u-boot would). To 
fix that more changes to the printenv implementation will be required 
(reconcat .flags from internal list structure) so I decided to omit it.


---
 src/fw_printenv.c |  16 +++--
 src/uboot_env.c   | 176 ++++++++++++++++++++++++++++++----------------
 2 files changed, 125 insertions(+), 67 deletions(-)

         buf += snprintf(buf, size, ".flags=");
@@ -780,10 +789,15 @@ int libuboot_env_store(struct uboot_ctx *ctx)
                         attr_tostring(entry->type),
                         access_tostring(entry->access));
             }
+               first = false;
         }
     }
     *buf++ = '\0';
 
+#if !defined(NDEBUG)
+    fprintf(stdout, "Saved environment FLAGS %s\n", flagsbuf);
+#endif
+
     if (ctx->redundant) {
         unsigned char flags = ctx->envdevs[ctx->current].flags;
         switch(ctx->envdevs[ctx->current].flagstype) {
@@ -817,18 +831,19 @@ int libuboot_env_store(struct uboot_ctx *ctx)
     return ret;
 }
 
-static void libuboot_parseflags(struct uboot_ctx *ctx, char* flagsvar)
+static void libuboot_parseflags(struct uboot_ctx *ctx, const char* flags)
 {
     char *pvar;
     char *pval;
     struct var_entry *entry;
+    char *flagsvar = strdup(flags);
 
 #if !defined(NDEBUG)
-    fprintf(stdout, "Environment FLAGS %s\n", flagsvar);
+    fprintf(stdout, "Parsed environment FLAGS %s\n", flags);
 #endif
     pvar = flagsvar;
 
-    while (*pvar && (pvar - flagsvar) < strlen(flagsvar)) {
+    while (pvar && ((pvar - flagsvar) < strlen(flags))) {
         char *pnext;
         pval = strchr(pvar, ':');
         if (!pval)
@@ -841,47 +856,56 @@ static void libuboot_parseflags(struct uboot_ctx 
*ctx, char* flagsvar)
         else
             *pnext++ = '\0';
 
+#if !defined(NDEBUG)
+        fprintf(stdout, "Parsed var %s:%s\n", pvar, pval);
+#endif
         entry = __libuboot_get_env(&ctx->varlist, pvar);
-        if (entry) {
-            for (int i = 0; i < strlen(pval); i++) {
-                switch (pval[i]) {
-                case 's':
-                    entry->type = TYPE_ATTR_STRING;
-                    break;
-                case 'd':
-                    entry->type = TYPE_ATTR_DECIMAL;
-                    break;
-                case 'x':
-                    entry->type = TYPE_ATTR_HEX;
-                    break;
-                case 'b':
-                    entry->type = TYPE_ATTR_BOOL;
-                    break;
-                case 'i':
-                    entry->type = TYPE_ATTR_IP;
-                    break;
-                case 'm':
-                    entry->type = TYPE_ATTR_MAC;
-                    break;
-                case 'a':
-                    entry->access = ACCESS_ATTR_ANY;
-                    break;
-                case 'r':
-                    entry->access = ACCESS_ATTR_READ_ONLY;
-                    break;
-                case 'o':
-                    entry->access = ACCESS_ATTR_WRITE_ONCE;
-                    break;
-                case 'c':
-                    entry->access = ACCESS_ATTR_CHANGE_DEFAULT;
-                    break;
-                default: /* ignore it */
-                    break;
-                }
+        if (!entry) {
+             /* create non-valued entry to conserve its flags */
+            libuboot_set_env(ctx, pvar, NULL);
+            entry = __libuboot_get_env(&ctx->varlist, pvar);
+        }
+
+        for (int i = 0; i < strlen(pval); i++) {
+            switch (pval[i]) {
+            case 's':
+                entry->type = TYPE_ATTR_STRING;
+                break;
+            case 'd':
+                entry->type = TYPE_ATTR_DECIMAL;
+                break;
+            case 'x':
+                entry->type = TYPE_ATTR_HEX;
+                break;
+            case 'b':
+                entry->type = TYPE_ATTR_BOOL;
+                break;
+            case 'i':
+                entry->type = TYPE_ATTR_IP;
+                break;
+            case 'm':
+                entry->type = TYPE_ATTR_MAC;
+                break;
+            case 'a':
+                entry->access = ACCESS_ATTR_ANY;
+                break;
+            case 'r':
+                entry->access = ACCESS_ATTR_READ_ONLY;
+                break;
+            case 'o':
+                entry->access = ACCESS_ATTR_WRITE_ONCE;
+                break;
+            case 'c':
+                entry->access = ACCESS_ATTR_CHANGE_DEFAULT;
+                break;
+            default: /* ignore it */
+                break;
             }
         }
+
         pvar = pnext;
     }
+    free(flagsvar);
 }
 
 static int libuboot_load(struct uboot_ctx *ctx)
@@ -1061,10 +1085,10 @@ int libuboot_load_file(struct uboot_ctx *ctx, const 
char *filename)
 
         name = buf;
 
-            if (!strcmp(name, ".flags"))
-                flagsvar = strdup(value);
+        if (!strcmp(name, ".flags"))
+            flagsvar = strdup(value);
         else
-                libuboot_set_env(ctx, name, value);
+            libuboot_set_env(ctx, name, value);
     }
 
     /*
@@ -1158,12 +1182,19 @@ static bool libuboot_validate_flags(struct 
var_entry *entry, const char *value)
         ok_access = true;
         break;
     case ACCESS_ATTR_READ_ONLY:
-    case ACCESS_ATTR_WRITE_ONCE:
         ok_access = false;
         break;
+    case ACCESS_ATTR_WRITE_ONCE:
+        if (entry->value) {
+            ok_access = false;
+        }
+        break;
     case ACCESS_ATTR_CHANGE_DEFAULT:
         break;
     }
+#if !defined(NDEBUG)
+    fprintf(stdout, "Access flag validation %s\n", ok_access ? "success." 
: "failed.");
+#endif
 
     if (!ok_access)
         return false;
@@ -1201,6 +1232,10 @@ static bool libuboot_validate_flags(struct var_entry 
*entry, const char *value)
             ok_type = false;
         break;
     }
+#if !defined(NDEBUG)
+    fprintf(stdout, "Type flag validation %s\n", ok_type ? "success." : 
"failed.");
+#endif
+
     return ok_type;
 }
 
@@ -1209,13 +1244,30 @@ int libuboot_set_env(struct uboot_ctx *ctx, const 
char *varname, const char *val
     struct var_entry *entry, *elm, *lastentry;
     struct vars *envs = &ctx->varlist;
 
+    /* update all .flags attributes in list with new value */
+    if (!strcmp(varname, ".flags")) {
+        LIST_FOREACH(entry, &ctx->varlist, next) {
+            entry->type = TYPE_ATTR_STRING;
+            entry->access = ACCESS_ATTR_ANY;
+        }
+        if (value)
+            libuboot_parseflags(ctx, strdup(value));
+
+        return 0;
+     }
+
     entry = __libuboot_get_env(envs, varname);
     if (entry) {
         if (libuboot_validate_flags(entry, value)) {
-            if (!value) {
-                free_var_entry(envs, entry);
-            } else {
-                free(entry->value);
+            if (entry->value) {
+                if (!value) {
+                    free_var_entry(envs, entry);
+                } else {
+                    free(entry->value);
+                    entry->value = strdup(value);
+                }
+            }
+            else if (value) {
                 entry->value = strdup(value);
             }
             return 0;
@@ -1224,9 +1276,6 @@ int libuboot_set_env(struct uboot_ctx *ctx, const 
char *varname, const char *val
         }
     }
 
-    if (!value)
-        return 0;
-
     entry = (struct var_entry *)calloc(1, sizeof(*entry));
     if (!entry)
         return -ENOMEM;
@@ -1235,11 +1284,16 @@ int libuboot_set_env(struct uboot_ctx *ctx, const 
char *varname, const char *val
         free(entry);
         return -ENOMEM;
     }
-    entry->value = strdup(value);
-    if (!entry->value) {
-        free(entry->name);
-        free(entry);
-        return -ENOMEM;
+    if (!value) {
+        entry->value = NULL;
+    }
+     else {
+        entry->value = strdup(value);
+        if (!entry->value) {
+            free(entry->name);
+            free(entry);
+            return -ENOMEM;
+        }
     }
 
     lastentry = NULL;

Patch
diff mbox series

diff --git a/src/fw_printenv.c b/src/fw_printenv.c
index 18887f9..8fd1f55 100644
--- a/src/fw_printenv.c
+++ b/src/fw_printenv.c
@@ -137,17 +137,21 @@  int main (int argc, char **argv) {
         if (!argc) {
             tmp = NULL;
             while ((tmp = libuboot_iterator(ctx, tmp)) != NULL) {
-                name = libuboot_getname(tmp);
                 value = libuboot_getvalue(tmp);
-                fprintf(stdout, "%s=%s\n", name, value);
+                if (value) {
+                    name = libuboot_getname(tmp);
+                    fprintf(stdout, "%s=%s\n", name, value);
+                }
             }
         } else {
             for (i = 0; i < argc; i++) {
                 value = libuboot_get_env(ctx, argv[i]);
-                if (noheader)
-                    fprintf(stdout, "%s\n", value ? value : "");
-                else
-                    fprintf(stdout, "%s=%s\n", argv[i], value ? value : 
"");
+                if (value) {
+                    if (noheader)
+                        fprintf(stdout, "%s\n", value ? value : "");
+                    else
+                        fprintf(stdout, "%s=%s\n", argv[i], value ? value 
: "");
+                }
             }
         }
     } else { /* setenv branch */
diff --git a/src/uboot_env.c b/src/uboot_env.c
index 09d210d..bf60635 100644
--- a/src/uboot_env.c
+++ b/src/uboot_env.c
@@ -735,6 +735,9 @@  int libuboot_env_store(struct uboot_ctx *ctx)
     uint8_t offsetdata;
     int ret;
     int copy;
+#if !defined(NDEBUG)
+    char * flagsbuf;
+#endif
 
     /*
      * Allocate the bigger of the case
@@ -752,21 +755,27 @@  int libuboot_env_store(struct uboot_ctx *ctx)
 
     buf = data;
     LIST_FOREACH(entry, &ctx->varlist, next) {
-        size = (ctx->size - offsetdata)  - (buf - data); 
-        if ((strlen(entry->name) + strlen(entry->value) + 2) > size) 
-            return -ENOMEM;
+        if (entry->value) {
+            /* only save variables which have a value (do exist yet) .. */
+            size = (ctx->size - offsetdata)  - (buf - data); 
+            if ((strlen(entry->name) + strlen(entry->value) + 2) > size) 
+                return -ENOMEM;
 
+            buf += snprintf(buf, size, "%s=%s", entry->name, 
entry->value); 
+            buf++;
+        }
+        /* .. but include non yet existing vars into .flags string */
         if (entry->type || entry->access)
             saveflags = true;
-
-        buf += snprintf(buf, size, "%s=%s", entry->name, entry->value); 
-        buf++;
     }
 
     /*
      * Now save the .flags
      */
     if (saveflags) {
+#if !defined(NDEBUG)
+        flagsbuf = buf;
+#endif
         bool first = true;
         size = (ctx->size - offsetdata)  - (buf - data);