Merge branch 'release'

Applications/Cxx/gdcmclean.cxx

@@ -138,6 +138,10 @@ static void PrintHelp() {
   std::cout << "     --preserve-illegal                             Whether or "
                "not preserve illegal attributes (eg. group 0003...)."
             << std::endl;
+  std::cout
+      << "     --empty-when-scrub-fails                       Fallback to "
+         "emptying any attribute for which scrub operation failed."
+      << std::endl;
   std::cout << "General Options:" << std::endl;
   std::cout << "  -V --verbose                more verbose (warning+error)."
             << std::endl;
@@ -171,6 +175,7 @@ int main(int argc, char *argv[]) {
   int preserveAllMissingPrivateCreator = 0;
   int preserveAllGroupLength = 0;
   int preserveAllIllegal = 0;
+  int emptyWhenScrubFails = 0;
   std::vector<gdcm::DPath> empty_dpaths;
   std::vector<gdcm::DPath> remove_dpaths;
   std::vector<gdcm::DPath> scrub_dpaths;
@@ -196,14 +201,15 @@ int main(int argc, char *argv[]) {
         {"recursive", no_argument, nullptr, 'r'},
         {"empty", required_argument, &empty_tag, 1},        // 3
         {"remove", required_argument, &remove_tag, 1},      // 4
-        {"scrub", required_argument, &scrub_tag, 1},          // 5
+        {"scrub", required_argument, &scrub_tag, 1},        // 5
         {"preserve", required_argument, &preserve_tag, 1},  // 5
         {"continue", no_argument, nullptr, 'c'},
         {"skip-meta", 0, &skipmeta, 1},  // should I document this one ?
         {"preserve-missing-private-creator", 0,
          &preserveAllMissingPrivateCreator, 1},                    //
         {"preserve-group-length", 0, &preserveAllGroupLength, 1},  //
         {"preserve-illegal", 0, &preserveAllIllegal, 1},           //
+        {"empty-when-scrub-fails", 0, &emptyWhenScrubFails, 1},    //
 
         {"verbose", no_argument, nullptr, 'V'},
         {"warning", no_argument, nullptr, 'W'},
@@ -437,6 +443,7 @@ int main(int argc, char *argv[]) {
   cleaner.RemoveAllMissingPrivateCreator(!preserveAllMissingPrivateCreator);
   cleaner.RemoveAllGroupLength(!preserveAllGroupLength);
   cleaner.RemoveAllIllegal(!preserveAllIllegal);
+  cleaner.EmptyWhenScrubFails(!!emptyWhenScrubFails);
   // Preserve
   for (std::vector<gdcm::DPath>::const_iterator it = preserve_dpaths.begin();
        it != preserve_dpaths.end(); ++it) {

Applications/Cxx/gdcmdump.cxx

@@ -1137,8 +1137,9 @@ static int PrintCSA(const std::string & filename)
   int ret = 0;
   if( ds.FindDataElement( t1 ) )
     {
-    csa.LoadFromDataElement( ds.GetDataElement( t1 ) );
-    csa.Print( std::cout );
+    if( csa.LoadFromDataElement( ds.GetDataElement( t1 ) ) )
+      csa.Print( std::cout );
+    else ret = 1;
     found = true;
     if( csa.GetFormat() == gdcm::CSAHeader::ZEROED_OUT )
       {
@@ -1156,8 +1157,9 @@ static int PrintCSA(const std::string & filename)
     }
   if( ds.FindDataElement( t2 ) )
     {
-    csa.LoadFromDataElement( ds.GetDataElement( t2 ) );
-    csa.Print( std::cout );
+    if( csa.LoadFromDataElement( ds.GetDataElement( t2 ) ) )
+      csa.Print( std::cout );
+    else ret = 1;
     found = true;
     if( csa.GetFormat() == gdcm::CSAHeader::ZEROED_OUT )
       {
@@ -1175,8 +1177,9 @@ static int PrintCSA(const std::string & filename)
     }
   if( ds.FindDataElement( t3 ) )
     {
-    csa.LoadFromDataElement( ds.GetDataElement( t3 ) );
-    csa.Print( std::cout );
+    if( csa.LoadFromDataElement( ds.GetDataElement( t3 ) ) )
+      csa.Print( std::cout );
+    else ret = 1;
     found = true;
     if( csa.GetFormat() == gdcm::CSAHeader::ZEROED_OUT )
       {

Source/DataStructureAndEncodingDefinition/gdcmCSAHeader.cxx

@@ -910,7 +910,8 @@ bool check_mapping(uint32_t syngodt, const char *vr)
 {
   static const unsigned int max = sizeof(mapping) / sizeof(equ);
   const equ *p = mapping;
-  assert( syngodt <= mapping[max-1].syngodt ); (void)max;
+  if( syngodt > mapping[max-1].syngodt ) return false;
+  assert( syngodt <= mapping[max-1].syngodt );
   while(p->syngodt < syngodt )
     {
     //std::cout << "mapping:" << p->vr << std::endl;
@@ -1098,6 +1099,11 @@ bool CSAHeader::LoadFromDataElement(DataElement const &de)
     char vr[4];
     ss.read(vr, 4);
     // In dataset without magic signature (OLD FORMAT) vr[3] is garbage...
+    if( vr[2] != 0 )
+    {
+      gdcmErrorMacro( "Garbage data. Stopping CSA parsing." );
+      return false;
+    }
     assert( /*vr[3] == 0 &&*/ vr[2] == 0 );
     csael.SetVR( VR::GetVRTypeFromFile(vr) );
     //std::cout << "VR " << vr << ", ";
@@ -1131,8 +1137,10 @@ bool CSAHeader::LoadFromDataElement(DataElement const &de)
       uint32_t item_xx[4];
       ss.read((char*)&item_xx, 4*sizeof(uint32_t));
       SwapperNoOp::SwapArray(item_xx,4);
+      if( item_xx[2] != 77 && item_xx[2] != 205 ) return false;
       assert( item_xx[2] == 77 || item_xx[2] == 205 );
       uint32_t len = item_xx[1]; // 2nd element
+      if( item_xx[0] != item_xx[1] || item_xx[1] != item_xx[3] ) return false;
       assert( item_xx[0] == item_xx[1] && item_xx[1] == item_xx[3] );
       if( len )
         {

Source/MediaStorageAndFileFormat/gdcmCleaner.cxx

@@ -538,10 +538,12 @@ struct Cleaner::impl {
   bool AllMissingPrivateCreator;
   bool AllGroupLength;
   bool AllIllegal;
+  bool WhenScrubFails;
   impl()
       : AllMissingPrivateCreator(true),
         AllGroupLength(true),
-        AllIllegal(true) {}
+        AllIllegal(true),
+        WhenScrubFails(false) {}
 
   enum ACTION { NONE, EMPTY, REMOVE, SCRUB };
   enum ACTION ComputeAction(File const &file, DataSet &ds,
@@ -668,6 +670,10 @@ struct Cleaner::impl {
   bool RemoveMissingPrivateCreator(Tag const & /*t*/) { return false; }
   void RemoveAllGroupLength(bool remove) { AllGroupLength = remove; }
   void RemoveAllIllegal(bool remove) { AllIllegal = remove; }
+  void EmptyWhenScrubFails(bool empty) { WhenScrubFails = empty; }
+
+  bool CleanCSAImage(DataSet &ds, const DataElement &de);
+  bool CleanCSASeries(DataSet &ds, const DataElement &de);
 };
 
 static VR ComputeDictVR(File &file, DataSet &ds, DataElement const &de) {
@@ -840,7 +846,7 @@ static inline bool bs_is_signature(const ByteValue *bv, const char *str) {
   return false;
 }
 
-static bool CleanCSAImage(DataSet &ds, const DataElement &de) {
+bool Cleaner::impl::CleanCSAImage(DataSet &ds, const DataElement &de) {
   const ByteValue *bv = de.GetByteValue();
   // fast path:
   if (!bv) return true;
@@ -888,6 +894,13 @@ static bool CleanCSAImage(DataSet &ds, const DataElement &de) {
       gdcmDebugMacro("Zero-out CSA header");
       return true;
     }
+    // fallback logic:
+    if (WhenScrubFails && is_signature(bv, sv10)) {
+      // so SV10 header has been identified, but we failed to 'scrub', let's
+      // empty it:
+      ds.Replace(clean);
+      return true;
+    }
     gdcmErrorMacro("Failure to call CleanCSAImage");
     return false;
   }
@@ -900,7 +913,7 @@ static bool CleanCSAImage(DataSet &ds, const DataElement &de) {
   return true;
 }
 
-static bool CleanCSASeries(DataSet &ds, const DataElement &de) {
+bool Cleaner::impl::CleanCSASeries(DataSet &ds, const DataElement &de) {
   const ByteValue *bv = de.GetByteValue();
   // fast path:
   if (!bv) return true;
@@ -944,6 +957,13 @@ static bool CleanCSASeries(DataSet &ds, const DataElement &de) {
       gdcmDebugMacro("Zero-out CSA header");
       return true;
     }
+    // fallback logic:
+    if (WhenScrubFails && is_signature(bv, sv10)) {
+      // so SV10 header has been identified, but we failed to 'scrub', let's
+      // empty it:
+      ds.Replace(clean);
+      return true;
+    }
     gdcmErrorMacro("Failure to call CleanCSASeries");
     return false;
   }
@@ -1065,8 +1085,8 @@ static bool IsDPathInSet(std::set<DPath> const &aset, DPath const dpath) {
 }
 
 Cleaner::impl::ACTION Cleaner::impl::ComputeAction(
-    File const & /*file*/, DataSet &ds, const DataElement &de, VR const &ref_dict_vr,
-    const std::string &tag_path) {
+    File const & /*file*/, DataSet &ds, const DataElement &de,
+    VR const &ref_dict_vr, const std::string &tag_path) {
   const Tag &tag = de.GetTag();
   // Group Length & Illegal cannot be preserved so it is safe to do them now:
   if (tag.IsGroupLength()) {
@@ -1302,6 +1322,9 @@ void Cleaner::RemoveAllGroupLength(bool remove) {
   pimpl->RemoveAllGroupLength(remove);
 }
 void Cleaner::RemoveAllIllegal(bool remove) { pimpl->RemoveAllIllegal(remove); }
+void Cleaner::EmptyWhenScrubFails(bool empty) {
+  pimpl->EmptyWhenScrubFails(empty);
+}
 
 bool Cleaner::Clean() {
   DataSet &ds = F->GetDataSet();

Source/MediaStorageAndFileFormat/gdcmCleaner.h

@@ -65,6 +65,9 @@ class GDCM_EXPORT Cleaner : public Subject {
   /// Should I remove all illegal attribute. Default: true
   void RemoveAllIllegal(bool remove);
 
+  /// Should I empty instead of scrub upon failure
+  void EmptyWhenScrubFails(bool empty);
+
   /// main loop
   bool Clean();
 

Utilities/gdcmext/csa.c

@@ -89,8 +89,15 @@ static void setup_buffer(struct app *self, void *output, const void *input,
   self->out->write = stream_write;
 }
 
+//#define CSA_DEBUG
+
+#ifdef CSA_DEBUG
+#define ERROR_RETURN(X, Y) \
+  if ((X) != (Y)) assert(0);
+#else
 #define ERROR_RETURN(X, Y) \
   if ((X) != (Y)) return false
+#endif
 
 static size_t fread_mirror(void *ptr, size_t size, size_t nmemb,
                            struct app *self) {
@@ -214,16 +221,18 @@ static bool read_info(struct app *self, struct csa_info *i) {
   s = fread_mirror(&i->vm, sizeof i->vm, 1, self);
   ERROR_RETURN(s, 1);
   // vm == 115301884 seems to be ok...
-  assert(i->vm < 0x6df5dfd);
+  ERROR_RETURN(i->vm < 0x6df5dfd, true);
   // vr
   s = fread_mirror(i->vr, sizeof *i->vr, sizeof i->vr / sizeof *i->vr, self);
   ERROR_RETURN(s, 4);
   {
     const char *s = i->vr;
-    assert(s[0] >= 'A' && s[0] <= 'Z');
-    assert(s[1] >= 'A' && s[1] <= 'Z');
-    assert(s[2] == 0);
-    if (self->csa_type == SV10) assert(s[3] == 0);
+    ERROR_RETURN(s[0] >= 'A' && s[0] <= 'Z', true);
+    ERROR_RETURN(s[1] >= 'A' && s[1] <= 'Z', true);
+    ERROR_RETURN(s[2], 0);
+    if (self->csa_type == SV10) {
+      ERROR_RETURN(s[3], 0);
+    }
   }
   // syngodt (signed)
   s = fread_mirror(&i->syngodt, sizeof i->syngodt, 1, self);
@@ -250,15 +259,15 @@ static bool read_data(struct app *self, struct csa_item_data *d) {
     uint32_t unused;
     s = fread_mirror(&unused, sizeof unused, 1, self);
     ERROR_RETURN(s, 1);
-    assert(unused == d->len || unused == 0x4d || unused == 0xcd);
+    ERROR_RETURN(unused == d->len || unused == 0x4d || unused == 0xcd, true);
   }
 
   if (d->len != 0) {
     const uint32_t padding_len = (4 - d->len % 4) % 4;
     const uint32_t padded_len = ((d->len + 3u) / 4u) * 4u;  // (len + 3) & ~0x03
     assert(padded_len == d->len + padding_len);             // programmer error
-    d->buffer = (char *)realloc(d->buffer, padded_len);
     assert(padded_len != 0);
+    d->buffer = (char *)realloc(d->buffer, padded_len);
     ERROR_RETURN(d->buffer != NULL, true);
     s = fread_mirror(d->buffer, sizeof *d->buffer, padded_len, self);
     ERROR_RETURN(s, padded_len);