Skip to content

Commit

Permalink
Merge pull request esmf-org#298 from esmf-org/fix_sprintf
Browse files Browse the repository at this point in the history 
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#284
  • Loading branch information
@billsacks
billsacks authored Sep 19, 2024
2 parents cef85de + 9bc54ab commit 46dfc47
Show file tree
Hide file tree
Showing 7 changed files with 110 additions and 46 deletions.
10 changes: 5 additions & 5 deletions src/Infrastructure/Mesh/src/Legacy/ESMCI_MeshMerge.C
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1178,12 +1178,12 @@ void concat_meshes(const Mesh & srcmesh, const Mesh & dstmesh, Mesh & mergemesh,
for(int npt=0; npt<clip_num_nodes; npt++) std::cout << clip_cd_sph[npt*2] << " " << clip_cd_sph[npt*2+1] << std::endl;
delete[] clip_cd_sph;
}
char msg[1024];
sprintf(msg," - there was a problem (e.g. repeated points, clockwise poly, etc.) with the triangulation of the element:\n");
sprintf(msg,"%selem Id: %d clip_elem Id: %d\n", msg, elem.get_id(), clip_elem.get_id());
std::stringstream msg;
msg << " - there was a problem (e.g. repeated points, clockwise poly, etc.) with the triangulation of the element:\n";
msg << "elem Id: " << elem.get_id() << " clip_elem Id: " << clip_elem.get_id() << "\n";
{
cd_sph = new double[num_p*2]; cart2sph(num_p, pts, cd_sph);
for(int npt=0; npt<num_p*2; npt++) sprintf(msg,"%s %g", msg, cd_sph[npt]);
cd_sph = new double[num_p*2]; cart2sph(num_p, pts, cd_sph);
for(int npt=0; npt<num_p*2; npt++) msg << " " << cd_sph[npt];
delete[] cd_sph;
}
if (ESMC_LogDefault.MsgFoundError(ESMC_RC_ARG_INCOMP, msg,
Expand Down
2 changes: 1 addition & 1 deletion src/Infrastructure/TimeMgr/include/ESMCI_Time.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@
// < declare the rest of the public interface methods here >

// 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;
Expand Down
2 changes: 1 addition & 1 deletion src/Infrastructure/TimeMgr/include/ESMCI_TimeInterval.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
40 changes: 29 additions & 11 deletions src/Infrastructure/TimeMgr/src/ESMCI_Time.C
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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]

Expand All @@ -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);

Expand Down
41 changes: 30 additions & 11 deletions src/Infrastructure/TimeMgr/src/ESMCI_TimeInterval.C
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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);

Expand Down
27 changes: 20 additions & 7 deletions src/Infrastructure/TimeMgr/tests/ESMF_TimeIntervalUTest.F90
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
34 changes: 24 additions & 10 deletions src/Infrastructure/TimeMgr/tests/ESMF_TimeUTest.F90
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 46dfc47

Please sign in to comment.