From cbe695586937278366b5c4cc07024f4571305138 Mon Sep 17 00:00:00 2001 From: cnweaver Date: Fri, 1 Dec 2017 21:57:41 -0700 Subject: [PATCH] Fix numerous bugs in auxilliary data handling: Correct undefined behavior due to trying to iterate between to separate temporary objects. Clean up extra quote characters inserted by FITS. Ignore COMMENT entries added by cfitsio to prevent them from multiplying endlessly. Introduce a const-correct character pointer type in case of unusual allocators. Stop the user from storing keys which are not allowed in FITS files. Stop the user from trying to store data which will not fit in a FITS keyword record. Add an overload of `splinetable::read_key` which retreives strings including whitespace. --- include/photospline/detail/aux.h | 68 ++++++++++++++++++++++++----- include/photospline/detail/fitsio.h | 19 +++++++- include/photospline/splinetable.h | 12 ++++- src/core/fitsio.cpp | 3 +- test/test_fitsio.cpp | 18 ++++++++ 5 files changed, 104 insertions(+), 16 deletions(-) diff --git a/include/photospline/detail/aux.h b/include/photospline/detail/aux.h index 1b16236..b4218e1 100644 --- a/include/photospline/detail/aux.h +++ b/include/photospline/detail/aux.h @@ -6,8 +6,8 @@ namespace photospline{ template -const char* splinetable::get_aux_value(const char* key) const{ - char* value=NULL; +typename splinetable::const_char_ptr splinetable::get_aux_value(const char* key) const{ + const_char_ptr value=nullptr; for (uint32_t i=0; i < naux; i++) { // NB: aux[i][0] may be a smart pointer if (strcmp(key, &*aux[i][0]) == 0) { @@ -61,36 +61,81 @@ bool splinetable::remove_key(const char* key){ template template bool splinetable::read_key(const char* key, T& result) const{ - const char *value = get_aux_value(key); + const_char_ptr value = get_aux_value(key); if(!value) return (false); - std::istringstream ss(value); + std::istringstream ss(&*value); ss >> result; return (!ss.fail()); } +template +bool splinetable::read_key(const char* key, std::string& result) const{ + const_char_ptr value = get_aux_value(key); + if(!value) + return (false); + result=&*value; + return (true); +} + template template bool splinetable::write_key(const char* key, const T& value){ //check if the key is allowed if (reservedFitsKeyword(key)) throw std::runtime_error("Cannot set key with reserved name "+std::string(key)); + size_t keylen = strlen(key) + 1; + size_t maxdatalen=68; //valid for short keys + if(keylen<=9){ //up to 8 bytes of data + for(size_t i=0; imaxdatalen){ + throw std::runtime_error("Value is too long to be stored as a FITS keyword ('" + +valuedata+"' has length "+std::to_string(valuelen-1) + +", but a maximum of "+std::to_string(maxdatalen)+ + " characters will fit with this key since continued " + "string keywords are not currently implemented.)"); + } //check if the key already exists and we should update it size_t i; for (i=0; i < naux; i++) { if (strcmp(key, &*aux[i][0]) == 0) break; } - std::ostringstream ss; - ss << value; - if(ss.fail()) - return(false); - size_t valuelen = ss.str().size() + 1; if (i!=naux) { //the key was found, so update it //try to allocate the correct amount of space for the new value try{ char_ptr new_value=allocate(valuelen); - std::copy(ss.str().begin(),ss.str().end(),new_value); + std::copy(valuedata.begin(),valuedata.end(),new_value); *(new_value+valuelen-1)=0; deallocate(aux[i][1],strlen(&aux[i][1][0])+1); aux[i][1]=new_value; @@ -101,7 +146,6 @@ bool splinetable::write_key(const char* key, const T& value){ } //otherwise, allocate more space and append else { - size_t keylen = strlen(key) + 1; //see if we can get space for the new data before we touch any existing things char_ptr new_key=nullptr, new_value=nullptr; char_ptr_ptr new_entry=nullptr; @@ -125,7 +169,7 @@ bool splinetable::write_key(const char* key, const T& value){ new_aux[naux][0] = new_key; new_aux[naux][1] = new_value; std::copy(key,key+keylen,new_aux[naux][0]); - std::copy(ss.str().begin(),ss.str().end(),new_value); + std::copy(valuedata.begin(),valuedata.end(),new_value); *(new_value+valuelen-1)=0; deallocate(aux,naux); aux = new_aux; diff --git a/include/photospline/detail/fitsio.h b/include/photospline/detail/fitsio.h index ae45ada..274c9c3 100644 --- a/include/photospline/detail/fitsio.h +++ b/include/photospline/detail/fitsio.h @@ -219,7 +219,24 @@ bool splinetable::read_fits_core(fitsfile* fits, const std::string& fileP aux[i][0] = allocate(keylen); aux[i][1] = allocate(valuelen); std::copy(key,key+keylen,aux[i][0]); - std::copy(value,value+valuelen,aux[i][1]); + //remove stupid quotes mandated by FITS, but not removed by cfitsio on reading + //Note that we do not attempt to remove whitespace, because we cannot + //distinguish whitespace included by the user and whitespace pointlessly + //added by FITS. + if(valuelen>1 && value[0]=='\''){ + if(valuelen>2 && value[valuelen-2]=='\''){ //remove a trailing quote also + std::copy(value+1,value+valuelen-2,aux[i][1]); + aux[i][1][valuelen-3]='\0'; + } + else{ //just remove an opening quote + std::copy(value+1,value+valuelen-1,aux[i][1]); + aux[i][1][valuelen-2]='\0'; + } + } + else{ + std::copy(value,value+valuelen,aux[i][1]); + aux[i][1][valuelen-1]='\0'; + } i++; } } else { diff --git a/include/photospline/splinetable.h b/include/photospline/splinetable.h index b325c53..82b8c57 100644 --- a/include/photospline/splinetable.h +++ b/include/photospline/splinetable.h @@ -131,6 +131,7 @@ class splinetable{ typedef typename allocator_traits::template rebind_traits::pointer double_ptr; typedef typename allocator_traits::template rebind_traits::pointer double_ptr_ptr; typedef typename allocator_traits::template rebind_traits::pointer char_ptr; + typedef typename allocator_traits::template rebind_traits::pointer const_char_ptr; typedef typename allocator_traits::template rebind_traits::pointer char_ptr_ptr; typedef typename allocator_traits::template rebind_traits::pointer char_ptr_ptr_ptr; @@ -276,11 +277,11 @@ class splinetable{ ///Get the count of 'auxiliary' keys size_t get_naux_values() const{ return(naux); } ///Directly get a particular auxiliary key - const char* get_aux_key(size_t i) const{ return(aux[i][0]); } + const_char_ptr get_aux_key(size_t i) const{ return(aux[i][0]); } ///Directly get a particular auxiliary value ///\param key the key whose value should be fetched ///\return the value if key exists, otherwise NULL - const char* get_aux_value(const char* key) const; + const_char_ptr get_aux_value(const char* key) const; ///Delete an auxiliary key and associated value ///\returns true if the key existed and was removed bool remove_key(const char* key); @@ -291,6 +292,13 @@ class splinetable{ /// sucessfully parsed into result, otherwise false template bool read_key(const char* key, T& result) const; + ///Look up the value associated with an auxiliary key + ///\note The data extracted may be padded with extra whitespace if it has + /// been read from a FITS file. + ///\param key the name of the key to look up + ///\param result location to store the value if the key is found + ///\return true if the key was found + bool read_key(const char* key, std::string& result) const; ///Insert or overwrite an auxiliary key,value pair ///\param key the name of the key to store ///\param value the value to store for the key diff --git a/src/core/fitsio.cpp b/src/core/fitsio.cpp index 4f10d54..bb8fe3a 100644 --- a/src/core/fitsio.cpp +++ b/src/core/fitsio.cpp @@ -33,7 +33,8 @@ bool reservedFitsKeyword(const char* key){ strncmp("ORDER", key, 5) == 0 || strncmp("NAXIS", key, 5) == 0 || strncmp("PERIOD", key, 6) == 0 || - strncmp("EXTEND", key, 6) == 0); + strncmp("EXTEND", key, 6) == 0 || + strncmp("COMMENT", key, 7) == 0); } uint32_t countAuxKeywords(fitsfile* fits){ diff --git a/test/test_fitsio.cpp b/test/test_fitsio.cpp index 3efb7d0..3d9f9a5 100644 --- a/test/test_fitsio.cpp +++ b/test/test_fitsio.cpp @@ -48,17 +48,28 @@ void compare_splines(const photospline::splinetable<>& spline1, TEST(write_fits_spline){ photospline::splinetable<> spline("test_data/test_spline_4d.fits"); + spline.write_key("SHORTKEY",123); + spline.write_key("ALongerKey","a string of text"); spline.write_fits("write_test_spline.fits"); photospline::splinetable<> spline2("write_test_spline.fits"); compare_splines(spline,spline2); + int extra_key=0; + ENSURE(spline2.read_key("SHORTKEY",extra_key)); + ENSURE_EQUAL(extra_key,123,"Additional keys must survive FITS serialization"); + std::string extra_key2; + ENSURE(spline2.read_key("ALongerKey",extra_key2)); + ENSURE_EQUAL(extra_key2,"a string of text","Additional keys must survive FITS serialization"); + unlink("write_test_spline.fits"); } TEST(fits_mem_spline){ photospline::splinetable<> spline("test_data/test_spline_4d.fits"); + spline.write_key("SHORTKEY",123); + spline.write_key("ALongerKey","a string of text"); auto buffer=spline.write_fits_mem(); std::unique_ptr data(buffer.first,&free); @@ -67,4 +78,11 @@ TEST(fits_mem_spline){ spline2.read_fits_mem(data.get(),buffer_size); compare_splines(spline,spline2); + + int extra_key=0; + ENSURE(spline2.read_key("SHORTKEY",extra_key)); + ENSURE_EQUAL(extra_key,123,"Additional keys must survive FITS serialization"); + std::string extra_key2; + ENSURE(spline2.read_key("ALongerKey",extra_key2)); + ENSURE_EQUAL(extra_key2,"a string of text","Additional keys must survive FITS serialization"); } \ No newline at end of file