[gnuastro-commits] master 2ccd136: All options checked for printing only when necessary

[Mohammad Akhlaghi]

branch: master
commit 2ccd136020fa87330d0d1815f9ce1f9cc5afb840
Author: Mohammad Akhlaghi <address@hidden>
Commit: Mohammad Akhlaghi <address@hidden>

    All options checked for printing only when necessary
    
    Until now, the `gal_options_print_state' would first go through all the
    options to see how many printing options were requested. It would then
    check (and abort) if more than one such option was called. Then it would go
    over all the options again and print the options if necessary in the proper
    place.
    
    In the meantime, the definition of the `--onlyversion' option was unique in
    that its `value' pointer was set to `NULL' instead of pointing to somewhere
    in memory (in `gal_options_common_params').
    
    As a result, when `--onlyversion' was called, its `set' value will be `1',
    but its pointer was still NULL. This caused a crash when we were checking
    to see if the user wants to print the option values or not (in the second
    round of checks mentioned in the first paragraph above).
    
    Therefore the first fix of this commit is to avoid the second check when no
    printing was necessary (to increase efficiency). The main fix was then to
    set `onlyversion's `value' pointer definition to a new (but redundant)
    `cp->onlyversion' element. Since this structure is initialized to zero in
    `main.c', when reading the option values `OPTIONS_UINT8VAL' will read it as
    a zero and the main crash above is thus avoided.
    
    This fixes bug #53147.
---
 NEWS                               |  2 +
 lib/gnuastro-internal/commonopts.h | 10 ++---
 lib/gnuastro-internal/options.h    |  1 +
 lib/options.c                      | 82 ++++++++++++++++++++++++--------------
 4 files changed, 60 insertions(+), 35 deletions(-)

diff --git a/NEWS b/NEWS
index 800152d..7f3782f 100644
--- a/NEWS
+++ b/NEWS
@@ -17,6 +17,8 @@ GNU Astronomy Utilities NEWS                          -*- 
outline -*-
 
   Crash when printing values with the `--onlyversion' option (bug #53142).
 
+  NULL value of onlyversion option causing a crash (bug #53147).
+
 
 
 
diff --git a/lib/gnuastro-internal/commonopts.h 
b/lib/gnuastro-internal/commonopts.h
index 06421e4..1e41a03 100644
--- a/lib/gnuastro-internal/commonopts.h
+++ b/lib/gnuastro-internal/commonopts.h
@@ -227,10 +227,10 @@ struct argp_option gal_commonopts_options[] =
       0,
       "Type of output: e.g., int16, float32, etc...",
       GAL_OPTIONS_GROUP_OUTPUT,
-      &cp->type,
-      GAL_TYPE_STRING,
-      GAL_OPTIONS_RANGE_GT_0,
-      GAL_OPTIONS_NOT_MANDATORY,
+      &cp->type,                /* Internally, `cp->type' is actually an   */
+      GAL_TYPE_STRING,          /* `uint8_t', but the user gives a string. */
+      GAL_OPTIONS_RANGE_GT_0,   /* So for the sanity checks to pass, we    */
+      GAL_OPTIONS_NOT_MANDATORY,/* use `GAL_TYPE_STRING' for this option.  */
       GAL_OPTIONS_NOT_SET,
       gal_options_read_type
     },
@@ -427,7 +427,7 @@ struct argp_option gal_commonopts_options[] =
       0,
       "Only run if the program version is STR.",
       GAL_OPTIONS_GROUP_OPERATING_MODE,
-      NULL,
+      &cp->onlyversion,
       GAL_TYPE_STRING,
       GAL_OPTIONS_RANGE_0_OR_1,
       GAL_OPTIONS_NOT_MANDATORY,
diff --git a/lib/gnuastro-internal/options.h b/lib/gnuastro-internal/options.h
index 5f4037e..e2777b7 100644
--- a/lib/gnuastro-internal/options.h
+++ b/lib/gnuastro-internal/options.h
@@ -195,6 +195,7 @@ struct gal_options_common_params
   size_t            numthreads; /* Number of threads to use.              */
   size_t            minmapsize; /* Minimum bytes necessary to use mmap.   */
   uint8_t                  log; /* Make a log file.                       */
+  char            *onlyversion; /* Redundant, kept/set for generality.    */
 
   /* Configuration files. */
   uint8_t          printparams; /* To print the full list of parameters.  */
diff --git a/lib/options.c b/lib/options.c
index 9fb9e48..322bbbd 100644
--- a/lib/options.c
+++ b/lib/options.c
@@ -184,8 +184,15 @@ gal_options_check_version(struct argp_option *option, char 
*arg,
   /* First see if we are reading or writing. */
   if(lineno==-1)
     {
-      /* Note that `PACKAGE_VERSION' is a static string. But the output
-         must be an allocated string so we can free it. */
+      /* `PACKAGE_VERSION' is a static/literal string, but the pointer
+         returned by this function will be freed, so we must allocate space
+         for it.
+
+         We didn't allocate and give this option a value when we read it
+         because it is redundant and much more likely for the option to
+         just be present (for a check in a reproduction pipeline for
+         example) than for it to be printed. So we don't want to waste
+         resources in allocating a redundant value. */
       gal_checkset_allocate_copy(PACKAGE_VERSION, &str);
       return str;
     }
@@ -2137,37 +2144,52 @@ gal_options_print_state(struct 
gal_options_common_params *cp)
         case GAL_OPTIONS_KEY_PRINTPARAMS:
         case GAL_OPTIONS_KEY_SETDIRCONF:
         case GAL_OPTIONS_KEY_SETUSRCONF:
+
+          /* Note that these options can have a value of 1 (enabled) or 0
+             (explicitly disabled). Therefore the printing should only be
+             done if they have a value of 1. This is why we have defined
+             the `OPTIONS_UINT8VAL' macro above. */
           sum += OPTIONS_UINT8VAL;
         }
-  if(sum>1)
-    error(EXIT_FAILURE, 0, "only one of the `printparams', `setdirconf' "
-          "and `setusrconf' options can be called in each run");
 
 
-  /* Print the required configuration files. Note that simply having a
-     non-NULL value is not enough. They can have a value of 1 or 0, and the
-     respective file should only be created if we have a value of 1. */
-  for(i=0; !gal_options_is_last(&cp->coptions[i]); ++i)
-    if(cp->coptions[i].set && OPTIONS_UINT8VAL)
-      switch(cp->coptions[i].key)
-        {
-        case GAL_OPTIONS_KEY_PRINTPARAMS:
-          options_print_all(cp, NULL, NULL);
-          break;
-
-        case GAL_OPTIONS_KEY_SETDIRCONF:
-          if( asprintf(&dirname, ".%s", PACKAGE)<0 )
-            error(EXIT_FAILURE, 0, "%s: asprintf allocation", __func__);
-          options_print_all(cp, dirname, cp->coptions[i].name);
-          free(dirname);
-          break;
+  /* See if the values should be printed and if so, where. */
+  switch(sum)
+    {
+    /* No printing option has been called, so just return. */
+    case 0:  return;
+
+    /* (Only) one of the printing options has been called, so we'll need to
+       print the values in the proper place. */
+    case 1:
+      for(i=0; !gal_options_is_last(&cp->coptions[i]); ++i)
+        if(cp->coptions[i].set && OPTIONS_UINT8VAL)
+          switch(cp->coptions[i].key)
+            {
+            case GAL_OPTIONS_KEY_PRINTPARAMS:
+              options_print_all(cp, NULL, NULL);
+              break;
+
+            case GAL_OPTIONS_KEY_SETDIRCONF:
+              if( asprintf(&dirname, ".%s", PACKAGE)<0 )
+                error(EXIT_FAILURE, 0, "%s: asprintf allocation", __func__);
+              options_print_all(cp, dirname, cp->coptions[i].name);
+              free(dirname);
+              break;
+
+            case GAL_OPTIONS_KEY_SETUSRCONF:
+              home=options_get_home();
+              if( asprintf(&dirname, "%s/%s", home, USERCONFIG_DIR)<0 )
+                error(EXIT_FAILURE, 0, "%s: asprintf allocation", __func__);
+              options_print_all(cp, dirname, cp->coptions[i].name);
+              free(dirname);
+              break;
+            }
+      break;
 
-        case GAL_OPTIONS_KEY_SETUSRCONF:
-          home=options_get_home();
-          if( asprintf(&dirname, "%s/%s", home, USERCONFIG_DIR)<0 )
-            error(EXIT_FAILURE, 0, "%s: asprintf allocation", __func__);
-          options_print_all(cp, dirname, cp->coptions[i].name);
-          free(dirname);
-          break;
-        }
+    /* More than one of the printing options has been called. */
+    default:
+      error(EXIT_FAILURE, 0, "only one of the `printparams', `setdirconf' "
+            "and `setusrconf' options can be called in each run");
+    }
 }