Hi Vu, Please find my comments taged with [AVM]
On 5/5/2016 12:48 PM, Vu Minh Nguyen wrote: > Hi Mahesh, > > I don't think there is a connection b/w SA_LOG_MAX_RECORD_SIZE vs > MDS_DIRECT_BUF_MAXSIZE. > And why the limit was set to 65535? Due to the limitation of its holder, I > guess so. > > In log service, in few places, holding `fixedLogRecordSize` is SaUint16T ( > MAX_UINT16 = 65535) > We need to change them all to SaUint32T in future. [AVM] In most of the places `fixedLogRecordSize` is already defined as SaUint32T , so we can just correct the missing places now it self . =========================================================================== osaf/services/saf/logsv/lgs/lgs_stream.h: SaUint32T fixedLogRecordSize; osaf/services/saf/logsv/lgs/lgs_mbcsv_v2.h: SaUint32T fixedLogRecordSize osaf/services/saf/logsv/lgs/lgs_mbcsv.cc: SaUint32T fixedLogRecordSize; osaf/services/saf/logsv/lgs/lgs_mbcsv_v1.h: SaUint32T fixedLogRecordSize; osaf/services/saf/logsv/lgs/lgs_filehdl.h: SaUint32T fixedLogRecordSize; osaf/services/saf/logsv/lgs/lgs_filehdl.h: SaUint32T fixedLogRecordSize; =========================================================================== > > For your comments, please see my responses inline, with [Vu]. > > Regards, Vu. > > >> -----Original Message----- >> From: A V Mahesh [mailto:[email protected]] >> Sent: Thursday, May 05, 2016 11:38 AM >> To: [email protected]; Mathivanan Naickan Palanivelu >> Subject: Re: [devel] [PATCH 1 of 1] log: verify logBufSize to avoid node >> malfunctioned [#1789] >> >> Hi Vu Minh Nguyen, >> >> 1) I hope this is mapping to size of a single MDS ( transport ) >> message size with out >> fragmentation , if so please replace >> >> `65535` with `MDS_DIRECT_BUF_MAXSIZE` >> >> ================================================================== >> -#define SA_LOG_MAX_RECORD_SIZE 65535 >> +#define SA_LOG_MAX_RECORD_SIZE MDS_DIRECT_BUF_MAXSIZE >> ================================================================== > [Vu] There is no connection b/w these constants as I mentioned above. > >> 2) In lga_api.c function `validate_open_params()` is validating >> SA_LOG_MAX_RECORD_SIZE `(logFileCreateAttributes->maxLogRecordSize > >> SA_LOG_MAX_RECORD_SIZE)))` >> and this patch introduced a new validation of >> SA_LOG_MAX_RECORD_SIZE in handle_log_record() , is their any >> possibility of consolidating thees two in to one >> common location where lga API can go through form common >> validation function ? >> > [Vu] Thanks. I will consider to clean it in one of next enhancement tickets. > e.g: Long DN & RDN support. [AVM] OK , just update the Long DN & RDN support enhancement tickets , to consider this while fixing. >> 3) By the way what is issue that you are seeing in MDS point of view by >> sending message more then >> SA_LOG_MAX_RECORD_SIZE with MDS ( transport ) message size with >> fragmentation ? >> > [Vu] Actually I have not yet considered impacts in case of sending an > message > over the MDS size limitation. So, I have no idea about that. [AVM] For your information , I would like to confirm you that as for as MDS messaging , their is no limitation on message size. -AVM > >> -AVM >> >> On 5/4/2016 12:52 PM, Vu Minh Nguyen wrote: >>> osaf/libs/agents/saf/lga/lga_api.c | 18 +++++++++++++++++ >>> tests/logsv/tet_saLogStreamOpen_2.c | 4 +++ >>> tests/logsv/tet_saLogWriteLogAsync.c | 38 >> ++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 60 insertions(+), 0 deletions(-) >>> >>> >>> When accidentally passing an invalid value of logBufSize to >> saLogWriteLogAsync() >>> such as a very large number which is caused by not using strlen() on > logBuf, >>> it will cause a lot of troubles. >>> >>> Add code to verify if logBufSize is calculated based on logBuf or not >>> and also prevent too big data sent to server side. >>> >>> diff --git a/osaf/libs/agents/saf/lga/lga_api.c >> b/osaf/libs/agents/saf/lga/lga_api.c >>> --- a/osaf/libs/agents/saf/lga/lga_api.c >>> +++ b/osaf/libs/agents/saf/lga/lga_api.c >>> @@ -996,6 +996,24 @@ static SaAisErrorT handle_log_record(con >>> ais_rc = SA_AIS_ERR_INVALID_PARAM; >>> goto done; >>> } >>> + >>> + if (logRecord->logBuffer->logBuf != NULL) { >>> + SaSizeT size = logRecord->logBuffer->logBufSize; >>> + bool sizeOver = size > strlen((char *)logRecord- >>> logBuffer->logBuf) + 1; >>> + /* Prevent log client accidently assign too big > number >> to logBufSize */ >>> + if (sizeOver == true) { >>> + TRACE("logBufSize > strlen(logBuf) + 1"); >>> + ais_rc = SA_AIS_ERR_INVALID_PARAM; >>> + goto done; >>> + } >>> + >>> + /* Prevent sending too big data to server side */ >>> + if (size > SA_LOG_MAX_RECORD_SIZE) { >>> + TRACE("logBuf data is too big (max: %d)", >> SA_LOG_MAX_RECORD_SIZE); >>> + ais_rc = SA_AIS_ERR_INVALID_PARAM; >>> + goto done; >>> + } >>> + } >>> } >>> >>> /* Set timeStamp data if not provided by application user */ >>> diff --git a/tests/logsv/tet_saLogStreamOpen_2.c >> b/tests/logsv/tet_saLogStreamOpen_2.c >>> --- a/tests/logsv/tet_saLogStreamOpen_2.c >>> +++ b/tests/logsv/tet_saLogStreamOpen_2.c >>> @@ -724,6 +724,8 @@ extern void saLogWriteLogAsync_14(void); >>> extern void saLogWriteLogAsync_15(void); >>> extern void saLogWriteLogAsync_16(void); >>> extern void saLogWriteLogAsync_17(void); >>> +extern void saLogWriteLogAsync_18(void); >>> +extern void saLogWriteLogAsync_19(void); >>> extern void saLogWriteLogCallbackT_01(void); >>> extern void saLogWriteLogCallbackT_02(void); >>> extern void saLogWriteLogCallbackT_03(void); >>> @@ -774,6 +776,8 @@ extern void saLogStreamClose_01(void); >>> test_case_add(2, saLogWriteLogAsync_15, "saLogWriteAsyncLog() NTF >> notificationObject length == 256"); >>> test_case_add(2, saLogWriteLogAsync_16, "saLogWriteAsyncLog() NTF >> notifyingObject length == 256"); >>> test_case_add(2, saLogWriteLogAsync_17, "saLogWriteLogAsync() > Generic >> logSvcUsrName length == 256"); >>> + test_case_add(2, saLogWriteLogAsync_18, "saLogWriteLogAsync() >> logBufSize > strlen(logBuf) + 1"); >>> + test_case_add(2, saLogWriteLogAsync_19, "saLogWriteLogAsync() >> logBufSize > SA_LOG_MAX_RECORD_SIZE"); >>> test_case_add(2, saLogWriteLogCallbackT_01, > "saLogWriteLogCallbackT() >> SA_DISPATCH_ONE"); >>> test_case_add(2, saLogWriteLogCallbackT_02, > "saLogWriteLogCallbackT() >> SA_DISPATCH_ALL"); >>> test_case_add(2, saLogFilterSetCallbackT_01, > "saLogFilterSetCallbackT >> OK"); >>> diff --git a/tests/logsv/tet_saLogWriteLogAsync.c >> b/tests/logsv/tet_saLogWriteLogAsync.c >>> --- a/tests/logsv/tet_saLogWriteLogAsync.c >>> +++ b/tests/logsv/tet_saLogWriteLogAsync.c >>> @@ -514,3 +514,41 @@ void saLogWriteLogAsync_17(void) >>> test_validate(rc1, SA_AIS_ERR_INVALID_PARAM); >>> } >>> } >>> + >>> +/** >>> + * saLogWriteAsyncLog() - logBufSize > strlen(logBuf) + 1 >>> + */ >>> +void saLogWriteLogAsync_18(void) >>> +{ >>> + SaInvocationT invocation = 0; >>> + >>> + strcpy((char*)genLogRecord.logBuffer->logBuf, __FUNCTION__); >>> + genLogRecord.logBuffer->logBufSize = strlen(__FUNCTION__) + 2; >>> + safassert(saLogInitialize(&logHandle, &logCallbacks, &logVersion), >> SA_AIS_OK); >>> + safassert(saLogStreamOpen_2(logHandle, &systemStreamName, NULL, >> 0, >>> + SA_TIME_ONE_SECOND, &logStreamHandle), >> SA_AIS_OK); >>> + rc = saLogWriteLogAsync(logStreamHandle, invocation, 0, >> &genLogRecord); >>> + safassert(saLogFinalize(logHandle), SA_AIS_OK); >>> + test_validate(rc, SA_AIS_ERR_INVALID_PARAM); >>> +} >>> + >>> +/** >>> + * saLogWriteAsyncLog() - big logBufSize > SA_LOG_MAX_RECORD_SIZE >>> + */ >>> +void saLogWriteLogAsync_19(void) >>> +{ >>> + SaInvocationT invocation = 0; >>> + char logBuf[SA_LOG_MAX_RECORD_SIZE + 10]; >>> + >>> + memset(logBuf, 'A', sizeof(logBuf)); >>> + logBuf[sizeof(logBuf) - 1] = '\0'; >>> + >>> + genLogRecord.logBuffer->logBuf = (SaUint8T *)&logBuf; >>> + genLogRecord.logBuffer->logBufSize = SA_LOG_MAX_RECORD_SIZE + 10; >>> + safassert(saLogInitialize(&logHandle, &logCallbacks, &logVersion), >> SA_AIS_OK); >>> + safassert(saLogStreamOpen_2(logHandle, &systemStreamName, NULL, >> 0, >>> + SA_TIME_ONE_SECOND, &logStreamHandle), >> SA_AIS_OK); >>> + rc = saLogWriteLogAsync(logStreamHandle, invocation, 0, >> &genLogRecord); >>> + safassert(saLogFinalize(logHandle), SA_AIS_OK); >>> + test_validate(rc, SA_AIS_ERR_INVALID_PARAM); >>> +} >>> >>> > ---------------------------------------------------------------------------- > -- >>> Find and fix application performance issues faster with Applications > Manager >>> Applications Manager provides deep performance insights into multiple > tiers >> of >>> your business applications. It resolves application problems quickly and >>> reduces your MTTR. Get your free trial! >>> https://ad.doubleclick.net/ddm/clk/302982198;130105516;z >>> _______________________________________________ >>> Opensaf-devel mailing list >>> [email protected] >>> https://lists.sourceforge.net/lists/listinfo/opensaf-devel >> >> --------------------------------------------------------------------------- > --- >> Find and fix application performance issues faster with Applications > Manager >> Applications Manager provides deep performance insights into multiple tiers >> of >> your business applications. It resolves application problems quickly and >> reduces your MTTR. Get your free trial! >> https://ad.doubleclick.net/ddm/clk/302982198;130105516;z >> _______________________________________________ >> Opensaf-devel mailing list >> [email protected] >> https://lists.sourceforge.net/lists/listinfo/opensaf-devel ------------------------------------------------------------------------------ Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
