From 4c123c0c3c76b40e5a31341bf061cb27fe373d69 Mon Sep 17 00:00:00 2001 From: Michael Zillgith Date: Thu, 30 May 2019 15:08:02 +0200 Subject: [PATCH] - MMS server: fixed potential deadlock in multi-thread mode --- src/iec61850/server/mms_mapping/reporting.c | 12 ++++- src/mms/inc_private/iso_server.h | 8 +++- src/mms/inc_private/mms_server_connection.h | 2 +- src/mms/iso_mms/server/mms_file_service.c | 40 ++++++++++++---- .../iso_mms/server/mms_information_report.c | 39 +++++++++++++-- .../iso_mms/server/mms_server_connection.c | 4 +- src/mms/iso_mms/server/mms_write_service.c | 8 +++- src/mms/iso_server/iso_connection.c | 48 ++++++++++--------- src/mms/iso_server/iso_server.c | 1 + 9 files changed, 117 insertions(+), 45 deletions(-) diff --git a/src/iec61850/server/mms_mapping/reporting.c b/src/iec61850/server/mms_mapping/reporting.c index 74fedb6a..461074a6 100644 --- a/src/iec61850/server/mms_mapping/reporting.c +++ b/src/iec61850/server/mms_mapping/reporting.c @@ -349,6 +349,8 @@ sendReportSegment(ReportControl* self, bool isIntegrity, bool isGI) if (self->clientConnection == NULL) return false; + IsoConnection_lock(self->clientConnection->isoConnection); + int maxMmsPduSize = MmsServerConnection_getMaxMmsPduSize(self->clientConnection); int estimatedSegmentSize = 19; /* maximum size of header information (header can have 13-19 byte) */ estimatedSegmentSize += 8; /* reserve space for more-segments-follow (3 byte) and sub-seq-num (3-5 byte) */ @@ -687,10 +689,12 @@ sendReportSegment(ReportControl* self, bool isIntegrity, bool isGI) reportBuffer->size = bufPos; - MmsServerConnection_sendMessage(self->clientConnection, reportBuffer, false); + MmsServerConnection_sendMessage(self->clientConnection, reportBuffer); MmsServer_releaseTransmitBuffer(self->server->mmsServer); + IsoConnection_unlock(self->clientConnection->isoConnection); + if (moreFollows == false) { /* reset sub sequence number */ segmented = false; @@ -2874,6 +2878,8 @@ sendNextReportEntrySegment(ReportControl* self) ReportControl_unlockNotify(self); + IsoConnection_lock(self->clientConnection->isoConnection); + ByteBuffer* reportBuffer = MmsServer_reserveTransmitBuffer(self->server->mmsServer); uint8_t* buffer = reportBuffer->buffer; @@ -3062,10 +3068,12 @@ sendNextReportEntrySegment(ReportControl* self) reportBuffer->size = bufPos; - MmsServerConnection_sendMessage(self->clientConnection, reportBuffer, false); + MmsServerConnection_sendMessage(self->clientConnection, reportBuffer); MmsServer_releaseTransmitBuffer(self->server->mmsServer); + IsoConnection_unlock(self->clientConnection->isoConnection); + if (moreFollows == false) { /* reset sub sequence number */ segmented = false; diff --git a/src/mms/inc_private/iso_server.h b/src/mms/inc_private/iso_server.h index b6707a58..42f3dc0b 100644 --- a/src/mms/inc_private/iso_server.h +++ b/src/mms/inc_private/iso_server.h @@ -72,6 +72,12 @@ IsoConnection_getPeerAddress(IsoConnection self); LIB61850_INTERNAL void IsoConnection_close(IsoConnection self); +LIB61850_INTERNAL void +IsoConnection_lock(IsoConnection self); + +LIB61850_INTERNAL void +IsoConnection_unlock(IsoConnection self); + LIB61850_INTERNAL void IsoConnection_installListener(IsoConnection self, MessageReceivedHandler handler, void* parameter); @@ -86,7 +92,7 @@ IsoConnection_getSecurityToken(IsoConnection self); * (handlerMode) */ LIB61850_INTERNAL void -IsoConnection_sendMessage(IsoConnection self, ByteBuffer* message, bool handlerMode); +IsoConnection_sendMessage(IsoConnection self, ByteBuffer* message); LIB61850_INTERNAL IsoServer IsoServer_create(TLSConfiguration tlsConfiguration); diff --git a/src/mms/inc_private/mms_server_connection.h b/src/mms/inc_private/mms_server_connection.h index aa65e98b..15d4c1a9 100644 --- a/src/mms/inc_private/mms_server_connection.h +++ b/src/mms/inc_private/mms_server_connection.h @@ -52,7 +52,7 @@ LIB61850_INTERNAL int MmsServerConnection_getMaxMmsPduSize(MmsServerConnection self); LIB61850_INTERNAL void -MmsServerConnection_sendMessage(MmsServerConnection self, ByteBuffer* message, bool handlerMode); +MmsServerConnection_sendMessage(MmsServerConnection self, ByteBuffer* message); LIB61850_INTERNAL bool MmsServerConnection_addNamedVariableList(MmsServerConnection self, MmsNamedVariableList variableList); diff --git a/src/mms/iso_mms/server/mms_file_service.c b/src/mms/iso_mms/server/mms_file_service.c index 60ac9238..288512b7 100644 --- a/src/mms/iso_mms/server/mms_file_service.c +++ b/src/mms/iso_mms/server/mms_file_service.c @@ -434,16 +434,20 @@ mmsServer_fileUploadTask(MmsServer self, MmsObtainFileTask task) case MMS_FILE_UPLOAD_STATE_SEND_FILE_READ: { + IsoConnection_lock(task->connection->isoConnection); + ByteBuffer* request = MmsServer_reserveTransmitBuffer(self); task->lastRequestInvokeId = MmsServerConnection_getNextRequestInvokeId(task->connection); mmsClient_createFileReadRequest(task->lastRequestInvokeId, request, task->frmsId); - IsoConnection_sendMessage(task->connection->isoConnection, request, false); + IsoConnection_sendMessage(task->connection->isoConnection, request); MmsServer_releaseTransmitBuffer(self); + IsoConnection_unlock(task->connection->isoConnection); + task->nextTimeout = Hal_getTimeInMs() + 2000; /* timeout 2000 ms */ task->state = MMS_FILE_UPLOAD_STATE_FILE_READ_SENT; @@ -468,16 +472,20 @@ mmsServer_fileUploadTask(MmsServer self, MmsObtainFileTask task) case MMS_FILE_UPLOAD_STATE_SEND_FILE_CLOSE: { + IsoConnection_lock(task->connection->isoConnection); + ByteBuffer* request = MmsServer_reserveTransmitBuffer(self); task->lastRequestInvokeId = MmsServerConnection_getNextRequestInvokeId(task->connection); mmsClient_createFileCloseRequest(task->lastRequestInvokeId, request, task->frmsId); - IsoConnection_sendMessage(task->connection->isoConnection, request, false); + IsoConnection_sendMessage(task->connection->isoConnection, request); MmsServer_releaseTransmitBuffer(self); + IsoConnection_unlock(task->connection->isoConnection); + task->nextTimeout = Hal_getTimeInMs() + 2000; /* timeout 2000 ms */ task->state = MMS_FILE_UPLOAD_STATE_FILE_CLOSE_SENT; @@ -504,17 +512,21 @@ mmsServer_fileUploadTask(MmsServer self, MmsObtainFileTask task) { /* send ObtainFileError */ + IsoConnection_lock(task->connection->isoConnection); + ByteBuffer* response = MmsServer_reserveTransmitBuffer(self); createServiceErrorObtainFileError(task->obtainFileRequestInvokeId, response, MMS_ERROR_FILE_FILE_NON_EXISTENT, 0); - IsoConnection_sendMessage(task->connection->isoConnection, response, false); + IsoConnection_sendMessage(task->connection->isoConnection, response); + + MmsServer_releaseTransmitBuffer(self); + + IsoConnection_unlock(task->connection->isoConnection); FileSystem_closeFile(task->fileHandle); deleteFile(MmsServerConnection_getFilesystemBasepath(task->connection), task->destinationFilename); - MmsServer_releaseTransmitBuffer(self); - if (DEBUG_MMS_SERVER) printf("MMS_SERVER: ObtainFile service: failed to open file from client\n"); @@ -526,11 +538,17 @@ mmsServer_fileUploadTask(MmsServer self, MmsObtainFileTask task) { /* send ObtainFileError */ + IsoConnection_lock(task->connection->isoConnection); + ByteBuffer* response = MmsServer_reserveTransmitBuffer(self); createServiceErrorObtainFileError(task->obtainFileRequestInvokeId, response, MMS_ERROR_FILE_OTHER, 1); - IsoConnection_sendMessage(task->connection->isoConnection, response, false); + IsoConnection_sendMessage(task->connection->isoConnection, response); + + MmsServer_releaseTransmitBuffer(self); + + IsoConnection_unlock(task->connection->isoConnection); if (task->fileHandle) { FileSystem_closeFile(task->fileHandle); @@ -539,8 +557,6 @@ mmsServer_fileUploadTask(MmsServer self, MmsObtainFileTask task) deleteFile(MmsServerConnection_getFilesystemBasepath(task->connection), task->destinationFilename); } - MmsServer_releaseTransmitBuffer(self); - if (DEBUG_MMS_SERVER) printf("MMS_SERVER: ObtainFile service: failed to create local file\n"); @@ -551,14 +567,18 @@ mmsServer_fileUploadTask(MmsServer self, MmsObtainFileTask task) case MMS_FILE_UPLOAD_STATE_SEND_OBTAIN_FILE_RESPONSE: { + IsoConnection_lock(task->connection->isoConnection); + ByteBuffer* response = MmsServer_reserveTransmitBuffer(self); createObtainFileResponse(task->obtainFileRequestInvokeId, response); - IsoConnection_sendMessage(task->connection->isoConnection, response, false); + IsoConnection_sendMessage(task->connection->isoConnection, response); MmsServer_releaseTransmitBuffer(self); + IsoConnection_unlock(task->connection->isoConnection); + if (self->getFileCompleteHandler) self->getFileCompleteHandler(self->getFileCompleteHandlerParameter, task->connection, task->destinationFilename); @@ -678,7 +698,7 @@ mmsServer_handleObtainFileRequest( mmsClient_createFileOpenRequest(task->lastRequestInvokeId, request, sourceFilename, 0); - IsoConnection_sendMessage(task->connection->isoConnection, request, true); + IsoConnection_sendMessage(task->connection->isoConnection, request); MmsServer_releaseTransmitBuffer(connection->server); diff --git a/src/mms/iso_mms/server/mms_information_report.c b/src/mms/iso_mms/server/mms_information_report.c index 932cc473..e0411dc2 100644 --- a/src/mms/iso_mms/server/mms_information_report.c +++ b/src/mms/iso_mms/server/mms_information_report.c @@ -1,7 +1,7 @@ /* * mms_information_report.c * - * Copyright 2013, 2015 Michael Zillgith + * Copyright 2013-2019 Michael Zillgith * * This file is part of libIEC61850. * @@ -59,6 +59,11 @@ MmsServerConnection_sendInformationReportSingleVariableVMDSpecific(MmsServerConn if (DEBUG_MMS_SERVER) printf("MMS_SERVER: sendInfReportSingle variable: %s\n", itemId); +#if (CONFIG_MMS_THREADLESS_STACK != 1) + if (handlerMode == false) + IsoConnection_lock(self->isoConnection); +#endif + ByteBuffer* reportBuffer = MmsServer_reserveTransmitBuffer(self->server); uint8_t* buffer = reportBuffer->buffer; @@ -80,10 +85,15 @@ MmsServerConnection_sendInformationReportSingleVariableVMDSpecific(MmsServerConn reportBuffer->size = bufPos; - IsoConnection_sendMessage(self->isoConnection, reportBuffer, handlerMode); + IsoConnection_sendMessage(self->isoConnection, reportBuffer); MmsServer_releaseTransmitBuffer(self->server); +#if (CONFIG_MMS_THREADLESS_STACK != 1) + if (handlerMode == false) + IsoConnection_unlock(self->isoConnection); +#endif + exit_function: return; } @@ -152,6 +162,11 @@ MmsServerConnection_sendInformationReportListOfVariables( goto exit_function; } +#if (CONFIG_MMS_THREADLESS_STACK != 1) + if (handlerMode == false) + IsoConnection_lock(self->isoConnection); +#endif + /* encode message */ ByteBuffer* reportBuffer = MmsServer_reserveTransmitBuffer(self->server); @@ -212,10 +227,16 @@ MmsServerConnection_sendInformationReportListOfVariables( reportBuffer->size = bufPos; - IsoConnection_sendMessage(self->isoConnection, reportBuffer, handlerMode); + IsoConnection_sendMessage(self->isoConnection, reportBuffer); MmsServer_releaseTransmitBuffer(self->server); +#if (CONFIG_MMS_THREADLESS_STACK != 1) + if (handlerMode == false) { + IsoConnection_unlock(self->isoConnection); + } +#endif + exit_function: return; } @@ -265,7 +286,10 @@ MmsServerConnection_sendInformationReportVMDSpecific(MmsServerConnection self, c goto exit_function; } - if (DEBUG_MMS_SERVER) printf("MMS_SERVER: sendInfReport\n"); +#if (CONFIG_MMS_THREADLESS_STACK != 1) + if (handlerMode == false) + IsoConnection_lock(self->isoConnection); +#endif ByteBuffer* reportBuffer = MmsServer_reserveTransmitBuffer(self->server); @@ -295,10 +319,15 @@ MmsServerConnection_sendInformationReportVMDSpecific(MmsServerConnection self, c reportBuffer->size = bufPos; - IsoConnection_sendMessage(self->isoConnection, reportBuffer, false); + IsoConnection_sendMessage(self->isoConnection, reportBuffer); MmsServer_releaseTransmitBuffer(self->server); +#if (CONFIG_MMS_THREADLESS_STACK != 1) + if (handlerMode == false) + IsoConnection_unlock(self->isoConnection); +#endif + exit_function: return; } diff --git a/src/mms/iso_mms/server/mms_server_connection.c b/src/mms/iso_mms/server/mms_server_connection.c index 448d9a98..d00ce842 100644 --- a/src/mms/iso_mms/server/mms_server_connection.c +++ b/src/mms/iso_mms/server/mms_server_connection.c @@ -752,9 +752,9 @@ MmsServerConnection_getMaxMmsPduSize(MmsServerConnection self) } void -MmsServerConnection_sendMessage(MmsServerConnection self, ByteBuffer* message, bool handlerMode) +MmsServerConnection_sendMessage(MmsServerConnection self, ByteBuffer* message) { - IsoConnection_sendMessage(self->isoConnection, message, false); + IsoConnection_sendMessage(self->isoConnection, message); } #if (MMS_DYNAMIC_DATA_SETS == 1) diff --git a/src/mms/iso_mms/server/mms_write_service.c b/src/mms/iso_mms/server/mms_write_service.c index ca4d8f95..08afbc1c 100644 --- a/src/mms/iso_mms/server/mms_write_service.c +++ b/src/mms/iso_mms/server/mms_write_service.c @@ -83,15 +83,21 @@ mmsServer_createMmsWriteResponse(MmsServerConnection connection, void MmsServerConnection_sendWriteResponse(MmsServerConnection self, uint32_t invokeId, MmsDataAccessError indication, bool handlerMode) { + if (handlerMode == false) + IsoConnection_lock(self->isoConnection); + ByteBuffer* response = MmsServer_reserveTransmitBuffer(self->server); ByteBuffer_setSize(response, 0); mmsServer_createMmsWriteResponse(self, invokeId, response, 1, &indication); - IsoConnection_sendMessage(self->isoConnection, response, handlerMode); + IsoConnection_sendMessage(self->isoConnection, response); MmsServer_releaseTransmitBuffer(self->server); + + if (handlerMode == false) + IsoConnection_unlock(self->isoConnection); } #if 0 diff --git a/src/mms/iso_server/iso_connection.c b/src/mms/iso_server/iso_connection.c index beb8330d..e412710b 100644 --- a/src/mms/iso_server/iso_connection.c +++ b/src/mms/iso_server/iso_connection.c @@ -163,13 +163,13 @@ IsoConnection_handleTcpConnection(IsoConnection self) printf("ISO_SERVER: COTP connection indication\n"); #if (CONFIG_MMS_THREADLESS_STACK != 1) - Semaphore_wait(self->conMutex); + IsoConnection_lock(self); #endif CotpConnection_sendConnectionResponseMessage(self->cotpConnection); #if (CONFIG_MMS_THREADLESS_STACK != 1) - Semaphore_post(self->conMutex); + IsoConnection_unlock(self); #endif break; @@ -200,7 +200,7 @@ IsoConnection_handleTcpConnection(IsoConnection self) if (aIndication == ACSE_ASSOCIATE) { #if (CONFIG_MMS_THREADLESS_STACK != 1) - Semaphore_wait(self->conMutex); + IsoConnection_lock(self); #endif if (DEBUG_ISO_SERVER) @@ -263,7 +263,7 @@ IsoConnection_handleTcpConnection(IsoConnection self) } #if (CONFIG_MMS_THREADLESS_STACK != 1) - Semaphore_post(self->conMutex); + IsoConnection_unlock(self); #endif } else { @@ -295,7 +295,7 @@ IsoConnection_handleTcpConnection(IsoConnection self) #if (CONFIG_MMS_THREADLESS_STACK != 1) IsoServer_userLock(self->isoServer); - Semaphore_wait(self->conMutex); + IsoConnection_lock(self); #endif ByteBuffer_wrap(&mmsResponseBuffer, self->sendBuffer, 0, SEND_BUF_SIZE); @@ -334,7 +334,7 @@ IsoConnection_handleTcpConnection(IsoConnection self) } #if (CONFIG_MMS_THREADLESS_STACK != 1) - Semaphore_post(self->conMutex); + IsoConnection_unlock(self); IsoServer_userUnlock(self->isoServer); #endif } @@ -355,7 +355,7 @@ IsoConnection_handleTcpConnection(IsoConnection self) #if (CONFIG_MMS_THREADLESS_STACK != 1) IsoServer_userLock(self->isoServer); - Semaphore_wait(self->conMutex); + IsoConnection_lock(self); #endif struct sBufferChain acseBufferPartStruct; @@ -382,7 +382,7 @@ IsoConnection_handleTcpConnection(IsoConnection self) CotpConnection_sendDataMessage(self->cotpConnection, sessionBufferPart); #if (CONFIG_MMS_THREADLESS_STACK != 1) - Semaphore_post(self->conMutex); + IsoConnection_unlock(self); IsoServer_userUnlock(self->isoServer); #endif } @@ -547,7 +547,23 @@ IsoConnection_getPeerAddress(IsoConnection self) } void -IsoConnection_sendMessage(IsoConnection self, ByteBuffer* message, bool handlerMode) +IsoConnection_lock(IsoConnection self) +{ +#if (CONFIG_MMS_THREADLESS_STACK != 1) + Semaphore_wait(self->conMutex); +#endif +} + +void +IsoConnection_unlock(IsoConnection self) +{ +#if (CONFIG_MMS_THREADLESS_STACK != 1) + Semaphore_post(self->conMutex); +#endif +} + +void +IsoConnection_sendMessage(IsoConnection self, ByteBuffer* message) { if (self->state == ISO_CON_STATE_STOPPED) { if (DEBUG_ISO_SERVER) @@ -555,15 +571,6 @@ IsoConnection_sendMessage(IsoConnection self, ByteBuffer* message, bool handlerM goto exit_error; } - bool locked = false; - -#if (CONFIG_MMS_THREADLESS_STACK != 1) - if (handlerMode == false) { - Semaphore_wait(self->conMutex); - locked = true; - } -#endif - struct sBufferChain payloadBufferStruct; BufferChain payloadBuffer = &payloadBufferStruct; payloadBuffer->length = message->size; @@ -597,11 +604,6 @@ IsoConnection_sendMessage(IsoConnection self, ByteBuffer* message, bool handlerM printf("ISO_SERVER: IsoConnection_sendMessage success!\n"); } -#if (CONFIG_MMS_THREADLESS_STACK != 1) - if (locked) - Semaphore_post(self->conMutex); -#endif - exit_error: return; } diff --git a/src/mms/iso_server/iso_server.c b/src/mms/iso_server/iso_server.c index 413ac7e2..194ad542 100644 --- a/src/mms/iso_server/iso_server.c +++ b/src/mms/iso_server/iso_server.c @@ -83,6 +83,7 @@ struct sIsoServer { #endif /* (CONFIG_MAXIMUM_TCP_CLIENT_CONNECTIONS == -1) */ #if (CONFIG_MMS_THREADLESS_STACK != 1) + /* used to control access to server data model */ Semaphore userLock; #endif