[gnuastro-commits] master 31e58fb7: Query: used value of --dataset in new variable

[Mohammad Akhlaghi]

branch: master
commit 31e58fb75f961b657750b17544745fa6f7d480ab
Author: Mohammad Akhlaghi <mohammad@akhlaghi.org>
Commit: Mohammad Akhlaghi <mohammad@akhlaghi.org>

    Query: used value of --dataset in new variable
    
    Until now, we would re-write the user's given value to '--dataset' when it
    was given as a summary (for example 'dr3' instead of 'gaiadr3.gaia_source'
    in the Gaia database). This re-write would cause a change of pointer, and
    thus a core dump when the value was to be written.
    
    With this commit, a new 'datasetuse' variable is defined for the "used"
    dataset string, leaving the original user's string to be 'datasetstr'. This
    avoids any potential re-write of memory before and after the query's
    execution.
    
    In the process, a few minor issues were also fixed:
    
      - gal_options_parse_name_and_value: a 'break' was missing after each
        'case' statement! This was the original cause of bug #64186.
    
      - While running Query with Valgrind, I noticed a few memory leaks that
        are also fixed with this commit.
    
    This bug was reported by Zohreh Ghaffari and Sepideh Eskandarlou.
    
    This fixes bug #64186.
---
 NEWS               |   5 +++
 bin/query/astron.c |   7 ++--
 bin/query/gaia.c   |  32 +++++------------
 bin/query/main.h   |   1 +
 bin/query/ned.c    |  18 +++++-----
 bin/query/query.c  |   6 ++--
 bin/query/tap.c    |  25 +++++++------
 bin/query/ui.c     |   2 ++
 bin/query/vizier.c | 104 +++++++++++++++++------------------------------------
 lib/options.c      |   6 ++--
 lib/wcs.c          |   2 ++
 11 files changed, 83 insertions(+), 125 deletions(-)

diff --git a/NEWS b/NEWS
index 04a7dd25..c7537884 100644
--- a/NEWS
+++ b/NEWS
@@ -38,6 +38,7 @@ See the end of the file for license conditions.
     --sigclip-std           SIGCLIP_STD            SIGCLIP-STD
     --sigclip-mean-sb       SIGCLIP_MEAN_SB        SIGCLIP-MEAN-SB
     --sigclip-mean-sb-delta SIGCLIP_MEAN_SB_DELTA  SIGCLIP-MEAN-SB-ERR
+    --------------------------------------------------------------
 
 ** Bugs fixed
   bug #64138: Arithmetic's mknoise-poisson only using first pixel value.
@@ -49,6 +50,10 @@ See the end of the file for license conditions.
               Infante-Sainz.
   bug #64153: astscript-ds9-region: '--namecol' gives a constant zero in
               image-mode. Reported by Zahra Sharbaf.
+  bug #64186: Query: core dump when custom name given to '--dataset'. First
+              reported by Zohreh Ghaffari as a problem in
+              'astscript-select-stars' (sr #110874) and identified as a
+              problem in Query by Sepideh Eskandarlou.
 
 
 
diff --git a/bin/query/astron.c b/bin/query/astron.c
index ad408e73..8d0c91d2 100644
--- a/bin/query/astron.c
+++ b/bin/query/astron.c
@@ -45,10 +45,9 @@ astron_sanity_checks(struct queryparams *p)
   if(p->datasetstr)
     {
       if( !strcmp(p->datasetstr, "tgssadr") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("tgssadr.main", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("tgssadr.main", &p->datasetuse);
+      else
+        gal_checkset_allocate_copy(p->datasetstr, &p->datasetuse);
     }
 
   /* Currently we assume ASTRON only uses TAP. */
diff --git a/bin/query/gaia.c b/bin/query/gaia.c
index b09bdab6..14024dbc 100644
--- a/bin/query/gaia.c
+++ b/bin/query/gaia.c
@@ -70,35 +70,19 @@ gaia_sanity_checks(struct queryparams *p)
   if(p->datasetstr)
     {
       if( !strcmp(p->datasetstr, "dr3") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("gaiadr3.gaia_source", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("gaiadr3.gaia_source", &p->datasetuse);
       else if( !strcmp(p->datasetstr, "edr3") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("gaiaedr3.gaia_source", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("gaiaedr3.gaia_source", &p->datasetuse);
       else if( !strcmp(p->datasetstr, "dr2") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("gaiadr2.gaia_source", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("gaiadr2.gaia_source", &p->datasetuse);
       else if( !strcmp(p->datasetstr, "dr1") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("gaiadr1.gaia_source", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("gaiadr1.gaia_source", &p->datasetuse);
       else if( !strcmp(p->datasetstr, "hipparcos") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("public.hipparcos", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("public.hipparcos", &p->datasetuse);
       else if( !strcmp(p->datasetstr, "tycho2") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("public.tycho2", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("public.tycho2", &p->datasetuse);
+      else
+        gal_checkset_allocate_copy(p->datasetstr, &p->datasetuse);
     }
 
   /* Currently we assume GAIA only uses TAP. */
diff --git a/bin/query/main.h b/bin/query/main.h
index 6a2e5244..d6b77120 100644
--- a/bin/query/main.h
+++ b/bin/query/main.h
@@ -63,6 +63,7 @@ struct queryparams
   gal_list_str_t      *columns;  /* Columns to extract from database.  */
 
   /* Internal variables. */
+  char             *datasetuse;  /* Used dataset string.               */
   char            *databasestr;  /* Name of input database.            */
   char           *downloadname;  /* Temporary output name.             */
   size_t       outtableinfo[2];  /* To print in output.                */
diff --git a/bin/query/ned.c b/bin/query/ned.c
index 673131be..feb50a41 100644
--- a/bin/query/ned.c
+++ b/bin/query/ned.c
@@ -46,17 +46,17 @@ ned_sanity_checks(struct queryparams *p)
     {
       /* Correct the dataset name if 'objdir' is given. */
       if( !strcmp(p->datasetstr, "objdir") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("NEDTAP.objdir", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("NEDTAP.objdir", &p->datasetuse);
+      else
+        gal_checkset_allocate_copy(p->datasetstr, &p->datasetuse);
+
 
       /* Database-specific checks. For example, if we should use TAP or
          not. Note that the user may give 'NEDTAP.objdir', so we can't use
          the 'if' above (for expanding summarized names). */
-      if( !strcmp(p->datasetstr, "NEDTAP.objdir") )
+      if( !strcmp(p->datasetuse, "NEDTAP.objdir") )
         p->usetap=1;
-      else if( !strcmp(p->datasetstr, "extinction") )
+      else if( !strcmp(p->datasetuse, "extinction") )
         {
           /* Crash for options that are not compatible with extinction. */
           if( p->radius || p->width || p->range || p->noblank || p->columns
@@ -89,8 +89,8 @@ ned_sanity_checks(struct queryparams *p)
   /* Currently NED only has a single table for TAP access, so warn the
      users about this if they ask for any other table. */
   if( p->usetap
-      && ( p->datasetstr==NULL
-           || strcmp(p->datasetstr, "NEDTAP.objdir") ) )
+      && ( p->datasetuse==NULL
+           || strcmp(p->datasetuse, "NEDTAP.objdir") ) )
     error(EXIT_FAILURE, 0, "NED currently only supports a single "
           "dataset with the TAP protocol called 'NEDTAP.objdir' "
           "(which you can also call in Query with '--dataset=objdir'). "
@@ -155,7 +155,7 @@ ned_extinction(struct queryparams *p)
 void
 ned_non_tap(struct queryparams *p)
 {
-  if( !strcmp(p->datasetstr, "extinction") )
+  if( !strcmp(p->datasetuse, "extinction") )
     ned_extinction(p);
 }
 
diff --git a/bin/query/query.c b/bin/query/query.c
index f37c7751..37145f98 100644
--- a/bin/query/query.c
+++ b/bin/query/query.c
@@ -230,7 +230,7 @@ query_output_meta_dataset(struct queryparams *p)
 
       /* Print the basic information. */
       printf("\n--------\ndatabase: %s (URL: %s)\ndataset: %s\n",
-             p->databasestr, p->urls->v, p->datasetstr);
+             p->databasestr, p->urls->v, p->datasetuse);
       gal_table_print_info(allcols, numcols, GAL_BLANK_SIZE_T);
     }
   else
@@ -245,7 +245,7 @@ query_output_meta_dataset(struct queryparams *p)
             "command below (put any search word or phrase in 'SEARCH' to "
             "find your dataset more easily):\n\n"
             "   astquery %s --information --limitinfo=\"SEARCH\"\n\n",
-            p->datasetstr, p->databasestr, p->databasestr);
+            p->datasetuse, p->databasestr, p->databasestr);
     }
 
   /* Clean up and return. */
@@ -314,7 +314,7 @@ query_output_finalize(struct queryparams *p)
       /* Prepare the output dataset. */
       if(p->information)
         {
-          if(p->datasetstr)  query_output_meta_dataset(p);
+          if(p->datasetuse)  query_output_meta_dataset(p);
           else               query_output_meta_database(p);
         }
       else if(isxml==0)      query_output_data(p);
diff --git a/bin/query/tap.c b/bin/query/tap.c
index 58c44f81..c4e4bff9 100644
--- a/bin/query/tap.c
+++ b/bin/query/tap.c
@@ -58,7 +58,7 @@ tap_sanity_checks(struct queryparams *p)
 
       /* If no dataset is explicitly given, let the user know that a
          catalog reference is necessary. */
-      if( p->information==0 && p->datasetstr==NULL )
+      if( p->information==0 && p->datasetuse==NULL )
         error(EXIT_FAILURE, 0, "no '--dataset' specified! To get the "
               "list of available datasets (tables) in this database, "
               "please run with '--information' (or '-i'). Note that some "
@@ -89,18 +89,18 @@ tap_dataset_quote_if_necessary(struct queryparams *p)
   char *c, *quoted;
 
   /* Parse the string for bad characters */
-  for(c=p->datasetstr; *c!='\0'; ++c)
+  for(c=p->datasetuse; *c!='\0'; ++c)
     if(*c=='/') { quote=1; break; }
 
   /* Add quotes around the database string. */
   if(quote)
     {
       /* Allocate the new string with quotes. */
-      if( asprintf(&quoted, "\"%s\"", p->datasetstr)<0 )
+      if( asprintf(&quoted, "\"%s\"", p->datasetuse)<0 )
         error(EXIT_FAILURE, 0, "%s: asprintf allocation ('quoted')",
               __func__);
     }
-  else quoted=p->datasetstr;
+  else quoted=p->datasetuse;
 
   /* Return the possibly quoted string. */
   return quoted;
@@ -118,10 +118,10 @@ tap_query_construct_meta(struct queryparams *p)
 
   /* If a dataset is given, build the query to download the metadata of
      that dataset. Otherwise, get the metadata of the full database. */
-  if(p->datasetstr)
+  if(p->datasetuse)
     {
       if( asprintf(&querystr,  "\"SELECT * FROM TAP_SCHEMA.columns "
-                   "WHERE table_name = '%s'\"", p->datasetstr)<0 )
+                   "WHERE table_name = '%s'\"", p->datasetuse)<0 )
         error(EXIT_FAILURE, 0, "%s: asprintf allocation ('querystr')",
               __func__);
     }
@@ -331,12 +331,12 @@ tap_query_construct_data(struct queryparams *p)
 {
   char *sortstr=NULL;
   char *headstr=NULL, allcols[]="*";
-  char *datasetstr, *valuelimitstr=NULL;
+  char *datasetuse, *valuelimitstr=NULL;
   char *querystr, *columns, *spatialstr=NULL;
 
   /* If the dataset has special characters (like a slash) it needs to
      be quoted. */
-  datasetstr=tap_dataset_quote_if_necessary(p);
+  datasetuse=tap_dataset_quote_if_necessary(p);
 
   /* If certain columns have been requested use them, otherwise
      download all existing columns.*/
@@ -365,7 +365,7 @@ tap_query_construct_data(struct queryparams *p)
   if( asprintf(&querystr,  "'SELECT %s %s FROM %s %s %s %s %s %s'",
                headstr ? headstr : "",
                columns,
-               datasetstr,
+               datasetuse,
                ( valuelimitstr || spatialstr ? "WHERE" : ""),
                valuelimitstr ? valuelimitstr : "",
                ( valuelimitstr && spatialstr ? "AND"   : "" ),
@@ -375,8 +375,10 @@ tap_query_construct_data(struct queryparams *p)
           __func__);
 
   /* Clean up and return. */
-  if(datasetstr!=p->datasetstr) free(datasetstr);
+  if(datasetuse!=p->datasetuse) free(datasetuse);
+  if(valuelimitstr) free(valuelimitstr);
   if(columns!=allcols) free(columns);
+  if(spatialstr) free(spatialstr);
   return querystr;
 }
 
@@ -437,4 +439,7 @@ tap_download(struct queryparams *p)
 
   /* Keep the executed command (to put in the final file's meta-data). */
   p->finalcommand=command;
+
+  /* Clean up. */
+  if(querystr!=p->query) free(querystr);
 }
diff --git a/bin/query/ui.c b/bin/query/ui.c
index e0e2f3d3..d00d43c3 100644
--- a/bin/query/ui.c
+++ b/bin/query/ui.c
@@ -616,5 +616,7 @@ ui_free_report(struct queryparams *p, struct timeval *t1)
   /* Free the allocated arrays: */
   free(p->cp.hdu);
   free(p->cp.output);
+  free(p->datasetstr);
+  free(p->datasetuse);
   free(p->downloadname);
 }
diff --git a/bin/query/vizier.c b/bin/query/vizier.c
index b62d7746..d5056308 100644
--- a/bin/query/vizier.c
+++ b/bin/query/vizier.c
@@ -56,97 +56,57 @@ vizier_sanity_checks(struct queryparams *p)
   /* Set the summarized names. */
   if(p->datasetstr)
     {
+      /* Check if the dataset name is a known summary. */
       if( !strcmp(p->datasetstr, "2mass") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("II/246/out", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("II/246/out", &p->datasetuse);
       else if( !strcmp(p->datasetstr, "akarifis") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("II/298/fis", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("II/298/fis", &p->datasetuse);
       else if( !strcmp(p->datasetstr, "allwise") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("II/328/allwise", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("II/328/allwise", &p->datasetuse);
       else if( !strcmp(p->datasetstr, "apass9") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("II/336/apass9", &p->datasetstr);
-        }
-      else if( !strcmp(p->datasetstr, "catwise") )
-        {
-          free(p->datasetstr);
-          if(p->ra_name==NULL) p->ra_name="RA_ICRS";
-          if(p->dec_name==NULL) p->dec_name="DE_ICRS";
-          gal_checkset_allocate_copy("II/365/catwise", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("II/336/apass9", &p->datasetuse);
       else if( !strcmp(p->datasetstr, "des1") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("II/357/des_dr1", &p->datasetstr);
-        }
-      else if( !strcmp(p->datasetstr, "gaiadr2") )
-        {
-          free(p->datasetstr);
-          if(p->ra_name==NULL) p->ra_name="ra_epoch2000";
-          if(p->dec_name==NULL) p->dec_name="dec_epoch2000";
-          gal_checkset_allocate_copy("I/345/gaia2", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("II/357/des_dr1", &p->datasetuse);
       else if( !strcmp(p->datasetstr, "gaiaedr3") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("I/350/gaiaedr3", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("I/350/gaiaedr3", &p->datasetuse);
       else if( !strcmp(p->datasetstr, "gaiadr3") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("I/355/gaiadr3", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("I/355/gaiadr3", &p->datasetuse);
       else if( !strcmp(p->datasetstr, "galex5") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("II/312/ais", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("II/312/ais", &p->datasetuse);
       else if( !strcmp(p->datasetstr, "nomad") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("I/297/out", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("I/297/out", &p->datasetuse);
       else if( !strcmp(p->datasetstr, "panstarrs1") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("II/349/ps1", &p->datasetstr);
-        }
+        gal_checkset_allocate_copy("II/349/ps1", &p->datasetuse);
       else if( !strcmp(p->datasetstr, "pmx1") )
+        gal_checkset_allocate_copy("I/317/sample", &p->datasetuse);
+      else if( !strcmp(p->datasetstr, "usnob1") )
+        gal_checkset_allocate_copy("I/284/out", &p->datasetuse);
+      else if( !strcmp(p->datasetstr, "ucac5") )
+        gal_checkset_allocate_copy("I/340/ucac5", &p->datasetuse);
+      else if( !strcmp(p->datasetstr, "unwise") )
+        gal_checkset_allocate_copy("II/363/unwise", &p->datasetuse);
+      else if( !strcmp(p->datasetstr, "catwise") )
         {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("I/317/sample", &p->datasetstr);
-        }
-      else if( !strcmp(p->datasetstr, "sdss12") )
-        {
-          free(p->datasetstr);
           if(p->ra_name==NULL) p->ra_name="RA_ICRS";
           if(p->dec_name==NULL) p->dec_name="DE_ICRS";
-          gal_checkset_allocate_copy("V/147/sdss12", &p->datasetstr);
+          gal_checkset_allocate_copy("II/365/catwise", &p->datasetuse);
         }
-      else if( !strcmp(p->datasetstr, "usnob1") )
-        {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("I/284/out", &p->datasetstr);
-        }
-      else if( !strcmp(p->datasetstr, "ucac5") )
+      else if( !strcmp(p->datasetstr, "gaiadr2") )
         {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("I/340/ucac5", &p->datasetstr);
+          if(p->ra_name==NULL) p->ra_name="ra_epoch2000";
+          if(p->dec_name==NULL) p->dec_name="dec_epoch2000";
+          gal_checkset_allocate_copy("I/345/gaia2", &p->datasetuse);
         }
-      else if( !strcmp(p->datasetstr, "unwise") )
+      else if( !strcmp(p->datasetstr, "sdss12") )
         {
-          free(p->datasetstr);
-          gal_checkset_allocate_copy("II/363/unwise", &p->datasetstr);
+          if(p->ra_name==NULL) p->ra_name="RA_ICRS";
+          if(p->dec_name==NULL) p->dec_name="DE_ICRS";
+          gal_checkset_allocate_copy("V/147/sdss12", &p->datasetuse);
         }
+
+      /* The given dataset is not a known summary. */
+      else
+        gal_checkset_allocate_copy(p->datasetstr, &p->datasetuse);
     }
 
   /* Currently we assume VizieR only uses TAP. */
diff --git a/lib/options.c b/lib/options.c
index 1b0c2ccd..05730855 100644
--- a/lib/options.c
+++ b/lib/options.c
@@ -1671,9 +1671,9 @@ gal_options_parse_name_and_values(struct argp_option 
*option, char *arg,
                   PACKAGE_BUGREPORT, GAL_OPTIONS_STATIC_MEM_FOR_VALUES);
           switch(str0_f641_sz2)
             {
-            case 0: nc += sprintf(sstr+nc, "%s,",  strarr[i]);
-            case 1: nc += sprintf(sstr+nc, "%g,",  darray[i]);
-            case 2: nc += sprintf(sstr+nc, "%zu,", sizarr[i]);
+            case 0: nc += sprintf(sstr+nc, "%s,",  strarr[i]); break;
+            case 1: nc += sprintf(sstr+nc, "%g,",  darray[i]); break;
+            case 2: nc += sprintf(sstr+nc, "%zu,", sizarr[i]); break;
             } /* No default necessary: valid value confirmed above. */
         }
 
diff --git a/lib/wcs.c b/lib/wcs.c
index 7872426d..c29073ce 100644
--- a/lib/wcs.c
+++ b/lib/wcs.c
@@ -2275,6 +2275,8 @@ gal_wcs_coverage(char *filename, char *hdu, size_t *ondim,
 
   /* Clean up and return success. */
   free(dsize);
+  if(name) free(name);
+  if(unit) free(unit);
   wcsfree(wcs); free(wcs);
   gal_list_data_free(coords);
   return 1;