Fixed poss. NULL pointer dereference/double free.

Thanks to Jinsheng Ba <[email protected]> for the report and some patches.

dcmnet/libsrc/dulstruc.h → dcmnet/include/dcmtk/dcmnet/dulstruc.h

@@ -71,6 +71,11 @@
 #include "dcmtk/config/osconfig.h"    /* make sure OS specific configuration is included first */
 #include "dcmtk/dcmnet/extneg.h"
 #include "dcmtk/dcmnet/dcuserid.h"
+#include "dcmtk/dcmnet/dicom.h"
+#include "dcmtk/dcmnet/dntypes.h"
+#include "dcmtk/dcmnet/dul.h"
+#include "dcmtk/dcmnet/lst.h"
+
 
 class DcmTransportConnection;
 class DcmTransportLayer;
@@ -290,6 +295,9 @@ typedef struct dul_datapdu {
     DUL_PRESENTATIONDATAVALUE presentationDataValue;
 }   DUL_DATAPDU;
 
+
+
+
 #define DUL_PROTOCOL      (unsigned short) 0x01
 
 #define DUL_TYPEAPPLICATIONCONTEXT  (unsigned char)0x10

dcmnet/include/dcmtk/dcmnet/helpers.h

@@ -0,0 +1,38 @@
+/*
+ *
+ *  Copyright (C) 2021, OFFIS e.V.
+ *  All rights reserved.  See COPYRIGHT file for details.
+ *
+ *  This software and supporting documentation were partly developed by
+ *
+ *    OFFIS e.V.
+ *    R&D Division Health
+ *    Escherweg 2
+ *    D-26121 Oldenburg, Germany
+ *
+ *
+ *  Module: dcmnet
+ *
+ *  Author: Michael Onken
+ *
+ *  Purpose: Collection of helper functions
+ *
+ */
+
+#ifndef DCMHET_HELPERS_H
+#define DCMHET_HELPERS_H
+
+#include "dcmtk/ofstd/ofcond.h"
+#include "dcmtk/dcmnet/dulstruc.h"
+
+struct T_ASC_Parameters;
+class LST_HEAD;
+
+
+void
+destroyPresentationContextList(LST_HEAD ** l);
+
+void
+destroyUserInformationLists(DUL_USERINFO * userInfo);
+
+#endif

dcmnet/libsrc/CMakeLists.txt

@@ -35,6 +35,7 @@ DCMTK_ADD_LIBRARY(dcmnet
   dulpres.cc
   dwrap.c
   extneg.cc
+  helpers.cc
   lst.cc
   scp.cc
   scpcfg.cc

dcmnet/libsrc/Makefile.in

@@ -25,7 +25,7 @@ objs = assoc.o cond.o dcompat.o dimcancl.o dimcmd.o dimdump.o dimecho.o \
 	dulfsm.o dulparse.o dulpres.o dul.o lst.o extneg.o dimget.o dcmlayer.o \
 	dcmtrans.o dcasccfg.o dcasccff.o dccfuidh.o dccftsmp.o dccfpcmp.o \
 	dccfrsmp.o dccfenmp.o dccfprmp.o dfindscu.o dstorscp.o dstorscu.o \
-	dcuserid.o scu.o scp.o scpcfg.o scpthrd.o scppool.o dwrap.o
+	dcuserid.o helper.o scu.o scp.o scpcfg.o scpthrd.o scppool.o dwrap.o
 
 library = libdcmnet.$(LIBEXT)
 

dcmnet/libsrc/assoc.cc

@@ -118,6 +118,7 @@
 #include "dcmtk/ofstd/ofconsol.h"
 #include "dcmtk/ofstd/ofstd.h"
 #include "dcmtk/dcmnet/dcmtrans.h"
+#include "dcmtk/dcmnet/helpers.h"
 
 /*
 ** Constant Definitions
@@ -336,25 +337,6 @@ ASC_createAssociationParameters(T_ASC_Parameters ** params,
     return EC_Normal;
 }
 
-static void
-destroyPresentationContextList(LST_HEAD ** lst)
-{
-    DUL_PRESENTATIONCONTEXT *pc;
-    DUL_TRANSFERSYNTAX *ts;
-
-    if ((lst == NULL) || (*lst == NULL))
-        return;
-    while ((pc = (DUL_PRESENTATIONCONTEXT*) LST_Dequeue(lst)) != NULL) {
-        if (pc->proposedTransferSyntax != NULL) {
-            while ((ts = (DUL_TRANSFERSYNTAX*) LST_Dequeue(&pc->proposedTransferSyntax)) != NULL) {
-                free(ts);
-            }
-            LST_Destroy(&pc->proposedTransferSyntax);
-        }
-        free(pc);
-    }
-    LST_Destroy(lst);
-}
 
 OFCondition
 ASC_destroyAssociationParameters(T_ASC_Parameters ** params)
@@ -1699,8 +1681,7 @@ ASC_destroyAssociation(T_ASC_Association ** association)
     }
 
     if ((*association)->params != NULL) {
-        cond = ASC_destroyAssociationParameters(&(*association)->params);
-        if (cond.bad()) return cond;
+        ASC_destroyAssociationParameters(&(*association)->params);
     }
 
     if ((*association)->sendPDVBuffer != NULL)

dcmnet/libsrc/dcuserid.cc

@@ -1,6 +1,6 @@
 /*
  *
- *  Copyright (C) 1997-2018, OFFIS e.V.
+ *  Copyright (C) 1997-2021, OFFIS e.V.
  *  All rights reserved.  See COPYRIGHT file for details.
  *
  *  This software and supporting documentation were developed by
@@ -23,7 +23,7 @@
 #include "dcmtk/config/osconfig.h" /* make sure OS specific configuration is included first */
 #include "dcmtk/dcmnet/dcuserid.h"
 #include "dcmtk/dcmnet/dul.h"
-#include "dulstruc.h"
+#include "dcmtk/dcmnet/dulstruc.h"
 
 /* ************************************************************************* */
 /*       Implementation of class UserIdentityNegotiationSubItem              */

dcmnet/libsrc/dul.cc

@@ -125,7 +125,7 @@ END_EXTERN_C
 #include "dcmtk/ofstd/ofstd.h"
 
 #include "dcmtk/dcmnet/dul.h"
-#include "dulstruc.h"
+#include "dcmtk/dcmnet/dulstruc.h"
 #include "dulpriv.h"
 #include "dulfsm.h"
 #include "dcmtk/dcmnet/dcmtrans.h"

dcmnet/libsrc/dulconst.cc

@@ -72,7 +72,7 @@
 #include "dcmtk/dcmnet/diutil.h"
 #include "dcmtk/dcmnet/lst.h"
 #include "dcmtk/dcmnet/dul.h"
-#include "dulstruc.h"
+#include "dcmtk/dcmnet/dulstruc.h"
 #include "dulpriv.h"
 #include "dcmtk/ofstd/ofconsol.h"
 

dcmnet/libsrc/dulextra.cc

@@ -96,7 +96,7 @@
 #include "dcmtk/dcmnet/lst.h"
 #include "dcmtk/dcmnet/cond.h"
 #include "dcmtk/dcmnet/dul.h"
-#include "dulstruc.h"
+#include "dcmtk/dcmnet/dulstruc.h"
 #include "dulpriv.h"
 #include "dcmtk/dcmnet/dcmtrans.h"
 #include "dcmtk/dcmnet/diutil.h"

dcmnet/libsrc/dulfsm.cc

@@ -105,7 +105,7 @@ END_EXTERN_C
 #include "dcmtk/dcmnet/lst.h"
 #include "dcmtk/dcmnet/cond.h"
 #include "dcmtk/dcmnet/dul.h"
-#include "dulstruc.h"
+#include "dcmtk/dcmnet/dulstruc.h"
 #include "dulpriv.h"
 #include "dulfsm.h"
 #include "dcmtk/ofstd/ofbmanip.h"
@@ -114,6 +114,7 @@ END_EXTERN_C
 #include "dcmtk/dcmnet/dcmtrans.h"
 #include "dcmtk/dcmnet/dcmlayer.h"
 #include "dcmtk/dcmnet/diutil.h"
+#include "dcmtk/dcmnet/helpers.h"
 #include "dcmtk/ofstd/ofsockad.h" /* for class OFSockAddr */
 #include <ctime>
 
@@ -304,9 +305,6 @@ findPresentationCtx(LST_HEAD ** lst, DUL_PRESENTATIONCONTEXTID contextID);
 PRV_SCUSCPROLE *
 findSCUSCPRole(LST_HEAD ** lst, char *abstractSyntax);
 
-void destroyPresentationContextList(LST_HEAD ** l);
-void destroyUserInformationLists(DUL_USERINFO * userInfo);
-
 static volatile FSM_Event_Description Event_Table[] = {
     {A_ASSOCIATE_REQ_LOCAL_USER, "A-ASSOCIATE request (local user)"},
     {TRANS_CONN_CONFIRM_LOCAL_USER, "Transport conn confirmation (local)"},
@@ -3978,48 +3976,3 @@ findSCUSCPRole(LST_HEAD ** lst, char *abstractSyntax)
     }
     return NULL;
 }
-
-void
-destroyPresentationContextList(LST_HEAD ** l)
-{
-    PRV_PRESENTATIONCONTEXTITEM
-    * prvCtx;
-    DUL_SUBITEM
-        * subItem;
-
-    if (*l == NULL)
-        return;
-
-    prvCtx = (PRV_PRESENTATIONCONTEXTITEM*)LST_Dequeue(l);
-    while (prvCtx != NULL) {
-        subItem = (DUL_SUBITEM*)LST_Dequeue(&prvCtx->transferSyntaxList);
-        while (subItem != NULL) {
-            free(subItem);
-            subItem = (DUL_SUBITEM*)LST_Dequeue(&prvCtx->transferSyntaxList);
-        }
-        LST_Destroy(&prvCtx->transferSyntaxList);
-        free(prvCtx);
-        prvCtx = (PRV_PRESENTATIONCONTEXTITEM*)LST_Dequeue(l);
-    }
-    LST_Destroy(l);
-}
-
-void
-destroyUserInformationLists(DUL_USERINFO * userInfo)
-{
-    PRV_SCUSCPROLE
-    * role;
-
-    role = (PRV_SCUSCPROLE*)LST_Dequeue(&userInfo->SCUSCPRoleList);
-    while (role != NULL) {
-        free(role);
-        role = (PRV_SCUSCPROLE*)LST_Dequeue(&userInfo->SCUSCPRoleList);
-    }
-    LST_Destroy(&userInfo->SCUSCPRoleList);
-
-    /* extended negotiation */
-    delete userInfo->extNegList; userInfo->extNegList = NULL;
-
-    /* user identity negotiation */
-    delete userInfo->usrIdent; userInfo->usrIdent = NULL;
-}

dcmnet/libsrc/dulparse.cc

@@ -70,7 +70,8 @@
 #include "dcmtk/dcmnet/lst.h"
 #include "dcmtk/dcmnet/dul.h"
 #include "dcmtk/dcmnet/diutil.h"
-#include "dulstruc.h"
+#include "dcmtk/dcmnet/dulstruc.h"
+#include "dcmtk/dcmnet/helpers.h"
 #include "dulpriv.h"
 #include "dcmtk/ofstd/ofconsol.h"
 
@@ -142,9 +143,6 @@ parseAssociate(unsigned char *buf, unsigned long pduLength,
         * context;
 
     (void) memset(assoc, 0, sizeof(*assoc));
-    if ((assoc->presentationContextList = LST_Create()) == NULL) return EC_MemoryExhausted;
-    if ((assoc->userInfo.SCUSCPRoleList = LST_Create()) == NULL) return EC_MemoryExhausted;
-
     // Check if the PDU actually is long enough for the fields we read
     if (pduLength < 2 + 2 + 16 + 16 + 32)
         return makeLengthError("associate PDU", pduLength, 2 + 2 + 16 + 16 + 32);
@@ -205,6 +203,8 @@ parseAssociate(unsigned char *buf, unsigned long pduLength,
             << "Called AP Title:  " << assoc->calledAPTitle << OFendl
             << "Calling AP Title: " << assoc->callingAPTitle);
     }
+    if ((assoc->presentationContextList = LST_Create()) == NULL) return EC_MemoryExhausted;
+    if ((assoc->userInfo.SCUSCPRoleList = LST_Create()) == NULL) return EC_MemoryExhausted;
     while ((cond.good()) && (pduLength > 0))
     {
         type = *buf;
@@ -219,44 +219,80 @@ parseAssociate(unsigned char *buf, unsigned long pduLength,
             {
                 buf += itemLength;
                 if (!OFStandard::safeSubtract(pduLength, itemLength, pduLength))
-                  return makeUnderflowError("Application Context item", pduLength, itemLength);
-                DCMNET_TRACE("Successfully parsed Application Context");
+                {
+                    cond = makeUnderflowError("Application Context item", pduLength, itemLength);
+                }
+                else
+                {
+                    DCMNET_TRACE("Successfully parsed Application Context");
+                }
             }
             break;
         case DUL_TYPEPRESENTATIONCONTEXTRQ:
         case DUL_TYPEPRESENTATIONCONTEXTAC:
             context = (PRV_PRESENTATIONCONTEXTITEM*)malloc(sizeof(PRV_PRESENTATIONCONTEXTITEM));
-            if (context == NULL) return EC_MemoryExhausted;
-            (void) memset(context, 0, sizeof(*context));
-            cond = parsePresentationContext(type, context, buf, &itemLength, pduLength);
-            if (cond.bad()) return cond;
-            buf += itemLength;
-            if (!OFStandard::safeSubtract(pduLength, itemLength, pduLength))
-              return makeUnderflowError("Presentation Context item", pduLength, itemLength);
-            LST_Enqueue(&assoc->presentationContextList, (LST_NODE*)context);
-            DCMNET_TRACE("Successfully parsed Presentation Context");
+            if (context != NULL)
+            {
+                (void) memset(context, 0, sizeof(*context));
+                cond = parsePresentationContext(type, context, buf, &itemLength, pduLength);
+                if (cond.bad())
+                {
+                    free(context);
+                }
+                else
+                {
+                    buf += itemLength;
+                    if (!OFStandard::safeSubtract(pduLength, itemLength, pduLength))
+                    {
+                        cond =  makeUnderflowError("Presentation Context item", pduLength, itemLength);
+                    }
+                    else
+                    {
+                        LST_Enqueue(&assoc->presentationContextList, (LST_NODE*)context);
+                        DCMNET_TRACE("Successfully parsed Presentation Context");
+                    }
+                }
+            }
+            else
+            {
+                cond = EC_MemoryExhausted;
+            }
             break;
         case DUL_TYPEUSERINFO:
             // parse user info, which can contain several sub-items like User
             // Identity Negotiation or SOP Class Extended Negotiation
             cond = parseUserInfo(&assoc->userInfo, buf, &itemLength, assoc->type, pduLength);
-            if (cond.bad())
-                return cond;
-            buf += itemLength;
-            if (!OFStandard::safeSubtract(pduLength, itemLength, pduLength))
-              return makeUnderflowError("User Information item", pduLength, itemLength);
-            DCMNET_TRACE("Successfully parsed User Information");
+            if (cond.good())
+            {
+                buf += itemLength;
+                if (!OFStandard::safeSubtract(pduLength, itemLength, pduLength))
+                {
+                    cond = makeUnderflowError("User Information item", pduLength, itemLength);
+                }
+                else
+                {
+                    DCMNET_TRACE("Successfully parsed User Information");
+                }
+            }
             break;
         default:
             cond = parseDummy(buf, &itemLength, pduLength);
-            if (cond.bad())
-                return cond;
-            buf += itemLength;
-            if (!OFStandard::safeSubtract(pduLength, itemLength, pduLength))
-              return makeUnderflowError("unknown item type", pduLength, itemLength);
+            if (cond.good())
+            {
+                buf += itemLength;
+                if (!OFStandard::safeSubtract(pduLength, itemLength, pduLength))
+                {
+                    cond = makeUnderflowError("unknown item type", pduLength, itemLength);
+                }
+            }
             break;
         }
     }
+    if (cond.bad())
+    {
+      destroyPresentationContextList(&assoc->presentationContextList);
+      destroyUserInformationLists(&assoc->userInfo);
+    }
     return cond;
 }
 
@@ -404,7 +440,11 @@ parsePresentationContext(unsigned char type,
                 subItem = (DUL_SUBITEM*)malloc(sizeof(DUL_SUBITEM));
                 if (subItem == NULL) return EC_MemoryExhausted;
                 cond = parseSubItem(subItem, buf, &length, presentationLength);
-                if (cond.bad()) return cond;
+                if (cond.bad())
+                {
+                    free(subItem);
+                    return cond;
+                }
                 LST_Enqueue(&context->transferSyntaxList, (LST_NODE*)subItem);
                 buf += length;
                 if (!OFStandard::safeSubtract(presentationLength, length, presentationLength))
@@ -525,7 +565,11 @@ parseUserInfo(DUL_USERINFO * userInfo,
             role = (PRV_SCUSCPROLE*)malloc(sizeof(PRV_SCUSCPROLE));
             if (role == NULL) return EC_MemoryExhausted;
             cond = parseSCUSCPRole(role, buf, &length, userLength);
-            if (cond.bad()) return cond;
+            if (cond.bad())
+            {
+                free(role);
+                return cond;
+            }
             LST_Enqueue(&userInfo->SCUSCPRoleList, (LST_NODE*)role);
             buf += length;
             if (!OFStandard::safeSubtract(userLength, OFstatic_cast(short unsigned int, length), userLength))