From 9bc54ab2f3bc9756925b4d7abc8a26477cbf041d Mon Sep 17 00:00:00 2001 From: Bill Sacks Date: Tue, 17 Sep 2024 16:47:25 -0600 Subject: [PATCH] Fix sprintf issues Fixes uses of sprintf where the output string is also used in the input, which can result in garbled time strings. Also changes these sprintf calls to snprintf to avoid possible buffer overflows, and checks these calls to ensure that the output variable is large enough to hold the relevant string, returning an error code if not. Resolves esmf-org/esmf#284 --- .../Mesh/src/Legacy/ESMCI_MeshMerge.C | 10 ++--- .../TimeMgr/include/ESMCI_Time.h | 2 +- .../TimeMgr/include/ESMCI_TimeInterval.h | 2 +- src/Infrastructure/TimeMgr/src/ESMCI_Time.C | 40 +++++++++++++----- .../TimeMgr/src/ESMCI_TimeInterval.C | 41 ++++++++++++++----- .../TimeMgr/tests/ESMF_TimeIntervalUTest.F90 | 27 ++++++++---- .../TimeMgr/tests/ESMF_TimeUTest.F90 | 34 ++++++++++----- 7 files changed, 110 insertions(+), 46 deletions(-) diff --git a/src/Infrastructure/Mesh/src/Legacy/ESMCI_MeshMerge.C b/src/Infrastructure/Mesh/src/Legacy/ESMCI_MeshMerge.C index 4285002832..199dd9fa24 100644 --- a/src/Infrastructure/Mesh/src/Legacy/ESMCI_MeshMerge.C +++ b/src/Infrastructure/Mesh/src/Legacy/ESMCI_MeshMerge.C @@ -1178,12 +1178,12 @@ void concat_meshes(const Mesh & srcmesh, const Mesh & dstmesh, Mesh & mergemesh, for(int npt=0; npt // return in string format (TMG 2.4.7) - int getString(char *timeString, const char *options=0) const; + int getString(char *timeString, int timeStringLen, const char *options=0) const; int getDayOfWeek(int *dayOfWeek) const; // (TMG 2.5.3) int getMidMonth(Time *midMonth) const; diff --git a/src/Infrastructure/TimeMgr/include/ESMCI_TimeInterval.h b/src/Infrastructure/TimeMgr/include/ESMCI_TimeInterval.h index 633c6d4575..ab2186830c 100644 --- a/src/Infrastructure/TimeMgr/include/ESMCI_TimeInterval.h +++ b/src/Infrastructure/TimeMgr/include/ESMCI_TimeInterval.h @@ -245,7 +245,7 @@ class TimeInterval : public BaseTime { // private: // return in string format (TMG 1.5.9) - int getString(char *timeString, const char *options=0) const; + int getString(char *timeString, int timeStringLen, const char *options=0) const; // common method for overloaded comparison operators bool compare(const TimeInterval &, ESMC_ComparisonType) const; diff --git a/src/Infrastructure/TimeMgr/src/ESMCI_Time.C b/src/Infrastructure/TimeMgr/src/ESMCI_Time.C index 78c21bce04..eb3febdd48 100644 --- a/src/Infrastructure/TimeMgr/src/ESMCI_Time.C +++ b/src/Infrastructure/TimeMgr/src/ESMCI_Time.C @@ -545,7 +545,7 @@ namespace ESMCI{ *timeZone = this->timeZone; } if (tempTimeString != ESMC_NULL_POINTER && timeStringLen > 0) { - rc = Time::getString(tempTimeString); + rc = Time::getString(tempTimeString, timeStringLen); if (ESMC_LogDefault.MsgFoundError(rc, ESMCI_ERR_PASSTHRU, ESMC_CONTEXT, &rc)) return(rc); @@ -554,7 +554,7 @@ namespace ESMCI{ } if (tempTimeStringISOFrac != ESMC_NULL_POINTER && timeStringLenISOFrac > 0) { - rc = Time::getString(tempTimeStringISOFrac, "isofrac"); + rc = Time::getString(tempTimeStringISOFrac, timeStringLenISOFrac, "isofrac"); if (ESMC_LogDefault.MsgFoundError(rc, ESMCI_ERR_PASSTHRU, ESMC_CONTEXT, &rc)) return(rc); @@ -1260,7 +1260,7 @@ namespace ESMCI{ if (options != ESMC_NULL_POINTER) { if (strncmp(options, "string", 6) == 0) { char timeString[ESMF_MAXSTR]; - Time::getString(timeString, &options[6]); + Time::getString(timeString, sizeof(timeString)-1, &options[6]); printf("%s\n", timeString); // see also method Time::get() } @@ -1393,9 +1393,9 @@ namespace ESMCI{ // int error return code // // !ARGUMENTS: - char *timeString, const char *options) const { // out - time value in - // string format - // in - format options + char *timeString, // out - time value in string format + int timeStringLen, // in - max number of characters that can be stored in timeString, not including the null terminator + const char *options) const { // in - format options // // !DESCRIPTION: // Gets a {\tt time}'s value in ISO 8601 string format @@ -1451,14 +1451,25 @@ namespace ESMCI{ &rc)) return(rc); + int requiredLen; + // format everything except seconds - sprintf(timeString, "%04lld-%02d-%02dT%02d:%02d:", yy_i8, mm, dd, h, m); + requiredLen = snprintf(timeString, timeStringLen+1, "%04lld-%02d-%02dT%02d:%02d:", yy_i8, mm, dd, h, m); + if (requiredLen > timeStringLen) { + std::stringstream msg; + msg << "timeString too small for result: " << timeStringLen << " < " << requiredLen; + ESMC_LogDefault.MsgFoundError(ESMC_RC_ARG_SIZE, msg, ESMC_CONTEXT, &rc); + return(rc); + } // format seconds according to specified options bool isofrac = false; if (options != ESMC_NULL_POINTER) { if (strstr(options, "isofrac") != ESMC_NULL_POINTER) isofrac = true; } + + char timeStringTemp[timeStringLen+1]; + strncpy(timeStringTemp, timeString, sizeof(timeStringTemp)); if (isofrac) { // strict ISO 8601 format YYYY-MM-DDThh:mm:ss[.f] @@ -1468,20 +1479,27 @@ namespace ESMCI{ // if fractionalSeconds non-zero (>= 0.5 ns) append full fractional value if (fabs(fractionalSeconds) >= 5e-10) { - sprintf(timeString, "%s%012.9f", timeString, (s + fractionalSeconds)); + requiredLen = snprintf(timeString, timeStringLen+1, "%s%012.9f", timeStringTemp, (s + fractionalSeconds)); } else { // no fractional seconds, just append integer seconds - sprintf(timeString, "%s%02d", timeString, s); + requiredLen = snprintf(timeString, timeStringLen+1, "%s%02d", timeStringTemp, s); } } else { // not strict ISO fractional seconds format // hybrid ISO 8601 format YYYY-MM-DDThh:mm:ss[:n/d] // if fractionalSeconds non-zero (sN!=0) append full fractional value if (sN != 0) { - sprintf(timeString, "%s%02d:%lld/%lld", timeString, s, sN, sD); + requiredLen = snprintf(timeString, timeStringLen+1, "%s%02d:%lld/%lld", timeStringTemp, s, sN, sD); } else { // no fractional seconds, just append integer seconds - sprintf(timeString, "%s%02d", timeString, s); + requiredLen = snprintf(timeString, timeStringLen+1, "%s%02d", timeStringTemp, s); } } + if (requiredLen > timeStringLen) + { + std::stringstream msg; + msg << "timeString too small for result: " << timeStringLen << " < " << requiredLen; + ESMC_LogDefault.MsgFoundError(ESMC_RC_ARG_SIZE, msg, ESMC_CONTEXT, &rc); + return (rc); + } return(rc); diff --git a/src/Infrastructure/TimeMgr/src/ESMCI_TimeInterval.C b/src/Infrastructure/TimeMgr/src/ESMCI_TimeInterval.C index 142d2fae09..7d9ea05b0f 100644 --- a/src/Infrastructure/TimeMgr/src/ESMCI_TimeInterval.C +++ b/src/Infrastructure/TimeMgr/src/ESMCI_TimeInterval.C @@ -822,7 +822,7 @@ namespace ESMCI{ // if requested, return time interval in string format if (tempTimeString != ESMC_NULL_POINTER && timeStringLen > 0) { - rc = TimeInterval::getString(tempTimeString); + rc = TimeInterval::getString(tempTimeString, timeStringLen); if (ESMC_LogDefault.MsgFoundError(rc, ESMCI_ERR_PASSTHRU, ESMC_CONTEXT, &rc)) return(rc); @@ -831,7 +831,7 @@ namespace ESMCI{ } if (tempTimeStringISOFrac != ESMC_NULL_POINTER && timeStringLenISOFrac > 0) { - rc = TimeInterval::getString(tempTimeStringISOFrac, "isofrac"); + rc = TimeInterval::getString(tempTimeStringISOFrac, timeStringLenISOFrac, "isofrac"); if (ESMC_LogDefault.MsgFoundError(rc, ESMCI_ERR_PASSTHRU, ESMC_CONTEXT, &rc)) return(rc); @@ -2885,7 +2885,7 @@ namespace ESMCI{ if (options != ESMC_NULL_POINTER) { if (strncmp(options, "string", 6) == 0) { char timeString[160]; - TimeInterval::getString(timeString, &options[6]); + TimeInterval::getString(timeString, sizeof(timeString)-1, &options[6]); printf("%s\n", timeString); // see also method TimeInterval::get() } @@ -3075,9 +3075,9 @@ namespace ESMCI{ // int error return code // // !ARGUMENTS: - char *timeString, const char *options) const { // out - time interval - // value in - // string format + char *timeString, // out - time interval value in string format + int timeStringLen, // in - max number of characters that can be stored in timeString, not including the null terminator + const char *options) const { // in - format options // // !DESCRIPTION: // Gets a {\tt ESMC\_TimeInterval}'s value in ISO 8601 string format @@ -3122,14 +3122,26 @@ namespace ESMCI{ if (ESMC_LogDefault.MsgFoundError(rc, ESMCI_ERR_PASSTHRU, ESMC_CONTEXT, &rc)) return(rc); + int requiredLen; + // format everything except seconds - sprintf(timeString, "P%lldY%lldM%lldDT%dH%dM", yy_i8, mm_i8, d_i8, h, m); + requiredLen = snprintf(timeString, timeStringLen+1, "P%lldY%lldM%lldDT%dH%dM", yy_i8, mm_i8, d_i8, h, m); + if (requiredLen > timeStringLen) + { + std::stringstream msg; + msg << "timeString too small for result: " << timeStringLen << " < " << requiredLen; + ESMC_LogDefault.MsgFoundError(ESMC_RC_ARG_SIZE, msg, ESMC_CONTEXT, &rc); + return (rc); + } // format seconds according to specified options bool isofrac = false; if (options != ESMC_NULL_POINTER) { if (strstr(options, "isofrac") != ESMC_NULL_POINTER) isofrac = true; } + + char timeStringTemp[timeStringLen+1]; + strncpy(timeStringTemp, timeString, sizeof(timeStringTemp)); if (isofrac) { // strict ISO 8601 format PyYmMdDThHmMs[.f]S @@ -3139,20 +3151,27 @@ namespace ESMCI{ // if fractionalSeconds non-zero (>= 0.5 ns) append full fractional value if (fabs(fractionalSeconds) >= 5e-10) { - sprintf(timeString, "%s%.9fS", timeString, (s + fractionalSeconds)); + requiredLen = snprintf(timeString, timeStringLen+1, "%s%.9fS", timeStringTemp, (s + fractionalSeconds)); } else { // no fractional seconds, just append integer seconds - sprintf(timeString, "%s%dS", timeString, s); + requiredLen = snprintf(timeString, timeStringLen+1, "%s%dS", timeStringTemp, s); } } else { // not strict ISO fractional seconds format // hybrid ISO 8601 format PyYmMdDThHmMs[:n/d]S // if fractionalSeconds non-zero (sN!=0) append full fractional value if (sN != 0) { - sprintf(timeString, "%s%d:%lld/%lldS", timeString, s, sN, sD); + requiredLen = snprintf(timeString, timeStringLen+1, "%s%d:%lld/%lldS", timeStringTemp, s, sN, sD); } else { // no fractional seconds, just append integer seconds - sprintf(timeString, "%s%dS", timeString, s); + requiredLen = snprintf(timeString, timeStringLen+1, "%s%dS", timeStringTemp, s); } } + if (requiredLen > timeStringLen) + { + std::stringstream msg; + msg << "timeString too small for result: " << timeStringLen << " < " << requiredLen; + ESMC_LogDefault.MsgFoundError(ESMC_RC_ARG_SIZE, msg, ESMC_CONTEXT, &rc); + return (rc); + } return(rc); diff --git a/src/Infrastructure/TimeMgr/tests/ESMF_TimeIntervalUTest.F90 b/src/Infrastructure/TimeMgr/tests/ESMF_TimeIntervalUTest.F90 index c8ee211fde..ad2669cd28 100644 --- a/src/Infrastructure/TimeMgr/tests/ESMF_TimeIntervalUTest.F90 +++ b/src/Infrastructure/TimeMgr/tests/ESMF_TimeIntervalUTest.F90 @@ -68,7 +68,13 @@ program ESMF_TimeIntervalUTest logical :: bool ! to retrieve time in string format - character(ESMF_MAXSTR) :: timeString + ! + ! note that this string is just barely long enough to hold the result string (to + ! ensure we don't have off-by-one errors in the string building and going back and + ! forth between Fortran and C strings) + character(15) :: timeString15 + ! and this one is just barely too short: + character(14) :: timeString14 ! instantiate timestep, start and stop times type(ESMF_Time) :: time1, time2 @@ -503,12 +509,19 @@ program ESMF_TimeIntervalUTest calendar=gregorianCalendar, rc=rc) call ESMF_TimeIntervalSet(timeStep, d=60, rc=rc) call ESMF_TimeIntervalGet(timeStep, mm=months, startTimeIn=startTime, & - timeString=timeString, rc=rc) - call ESMF_Test((months==2 .and. timeString=="P0Y0M60DT0H0M0S" .and. & + timeString=timeString15, rc=rc) + call ESMF_Test((months==2 .and. timeString15=="P0Y0M60DT0H0M0S" .and. & rc==ESMF_SUCCESS), name, failMsg, result, ESMF_SRCLINE) !print *, "months = ", months - !print *, "timeStep = ", timeString + !print *, "timeStep = ", timeString15 + + ! ---------------------------------------------------------------------------- + !EX_UTest + write(name, *) "Get TimeInterval Test - too short string" + write(failMsg, *) " Did not return ESMC_RC_ARG_SIZE" + call ESMF_TimeIntervalGet(timeStep, startTimeIn=startTime, timeString=timeString14, rc=rc) + call ESMF_Test((rc==ESMC_RC_ARG_SIZE), name, failMsg, result, ESMF_SRCLINE) ! ---------------------------------------------------------------------------- !EX_UTest @@ -1766,12 +1779,12 @@ program ESMF_TimeIntervalUTest calendar=julianCalendar, rc=rc) call ESMF_TimeIntervalSet(timeStep, d=60, rc=rc) call ESMF_TimeIntervalGet(timeStep, mm=months, startTimeIn=startTime, & - timeString=timeString, rc=rc) - call ESMF_Test((months==2 .and. timeString=="P0Y0M60DT0H0M0S" .and. & + timeString=timeString15, rc=rc) + call ESMF_Test((months==2 .and. timeString15=="P0Y0M60DT0H0M0S" .and. & rc==ESMF_SUCCESS), name, failMsg, result, ESMF_SRCLINE) !print *, "months = ", months - !print *, "timeStep = ", timeString + !print *, "timeStep = ", timeString15 ! ---------------------------------------------------------------------------- !EX_UTest diff --git a/src/Infrastructure/TimeMgr/tests/ESMF_TimeUTest.F90 b/src/Infrastructure/TimeMgr/tests/ESMF_TimeUTest.F90 index d1a7cb0ca3..1ac112ae6e 100644 --- a/src/Infrastructure/TimeMgr/tests/ESMF_TimeUTest.F90 +++ b/src/Infrastructure/TimeMgr/tests/ESMF_TimeUTest.F90 @@ -53,7 +53,14 @@ program ESMF_TimeUTest character(ESMF_MAXSTR) :: failMsg ! to retrieve time in string format - character(ESMF_MAXSTR) :: timeString + ! + ! note that these strings are just barely long enough to hold the result string (to + ! ensure we don't have off-by-one errors in the string building and going back and + ! forth between Fortran and C strings) + character(19) :: timeString19 + character(20) :: timeString20 + ! and this one is just barely too short: + character(18) :: timeString18 ! instantiate start time type(ESMF_Time) :: startTime @@ -131,14 +138,21 @@ program ESMF_TimeUTest write(name, *) "Get Time Test 1" write(failMsg, *) " Did not return 2004-01-29T12:17:58 or ESMF_SUCCESS" call ESMF_TimeGet(startTime, yy=YY, mm=MM, dd=DD, h=H, m=M, s=S, & - timeString=timeString, rc=rc) + timeString=timeString19, rc=rc) call ESMF_Test((YY==2004 .and. MM==1 .and. DD==29 .and. & H==12 .and. M==17 .and. S==58 .and. & - timeString=="2004-01-29T12:17:58" .and. & + timeString19=="2004-01-29T12:17:58" .and. & rc==ESMF_SUCCESS), name, failMsg, result, ESMF_SRCLINE) - !print *, "startTime = ", timeString + !print *, "startTime = ", timeString19 + ! ---------------------------------------------------------------------------- + !NEX_UTest + + write(name, *) "Get Time Test - too short string" + write(failMsg, *) " Did not return ESMC_RC_ARG_SIZE" + call ESMF_TimeGet(startTime, timeString=timeString18, rc=rc) + call ESMF_Test((rc==ESMC_RC_ARG_SIZE), name, failMsg, result, ESMF_SRCLINE) #ifdef ESMF_TESTEXHAUSTIVE @@ -227,12 +241,12 @@ program ESMF_TimeUTest call ESMF_TimeSet(time1, yy=9, mm=2, dd=7, & calendar=gregorianCalendar, rc=rc) call ESMF_TimeGet(time1, yy=YY, mm=MM, dd=DD, & - timeString=timeString, rc=rc) + timeString=timeString19, rc=rc) call ESMF_Test((YY==9 .and. MM==2 .and. DD==7 .and. & - timeString=="0009-02-07T00:00:00" .and. & + timeString19=="0009-02-07T00:00:00" .and. & rc==ESMF_SUCCESS), name, failMsg, result, ESMF_SRCLINE) - !print *, "time1 = ", timeString + !print *, "time1 = ", timeString19 ! ---------------------------------------------------------------------------- !EX_UTest @@ -243,12 +257,12 @@ program ESMF_TimeUTest call ESMF_TimeSet(time1, yy=10000, mm=2, dd=7, & calendar=gregorianCalendar, rc=rc) call ESMF_TimeGet(time1, yy=YY, mm=MM, dd=DD, & - timeString=timeString, rc=rc) + timeString=timeString20, rc=rc) call ESMF_Test((YY==10000 .and. MM==2 .and. DD==7 .and. & - timeString=="10000-02-07T00:00:00" .and. & + timeString20=="10000-02-07T00:00:00" .and. & rc==ESMF_SUCCESS), name, failMsg, result, ESMF_SRCLINE) - !print *, "time1 = ", timeString + !print *, "time1 = ", timeString20 ! ---------------------------------------------------------------------------- !EX_UTest