Timemgr using sprintf with the output string also used in the input #284

oehmke · 2024-09-03T21:40:31Z

Atanas ran into an issue with a garbled time string and traced it down to a sprintf call in the time manager. It looks like there are several instances of sprintf() with the output string being also used as an input. It sounds like in general this shouldn't be done, but that it sometimes works (depending on the implementation of sprintf()). We should fix all of these, so we don't run into random issues. An example of one of these is in .../TimeMgr/src/ESMCI_Time.C line 1482.

billsacks · 2024-09-03T22:07:18Z

I'm happy to take this on.

I just did a regex search to try to identify the places that need to be fixed. This would miss anything where a sprintf call is split across multiple lines, but hopefully is at least a good start: The regex is sprintf *\(([^,]+) *,.*\1. There are a number of false positives, but it picks up some things that should be fixed:

esmf/src/Infrastructure/Mesh/src/Legacy/ESMCI_MeshMerge.C

Line 1183 in 82f8052

    
           sprintf(msg,"%selem Id: %d clip_elem Id: %d\n", msg, elem.get_id(), clip_elem.get_id());

esmf/src/Infrastructure/Mesh/src/Legacy/ESMCI_MeshMerge.C

Line 1186 in 82f8052

for(int npt=0; npt<num_p*2; npt++) sprintf(msg,"%s %g", msg, cd_sph[npt]);

esmf/src/Infrastructure/TimeMgr/src/ESMCI_Time.C

Line 1471 in 82f8052

sprintf(timeString, "%s%012.9f", timeString, (s + fractionalSeconds));

esmf/src/Infrastructure/TimeMgr/src/ESMCI_Time.C

Line 1473 in 82f8052

sprintf(timeString, "%s%02d", timeString, s);

esmf/src/Infrastructure/TimeMgr/src/ESMCI_Time.C

Line 1480 in 82f8052

sprintf(timeString, "%s%02d:%lld/%lld", timeString, s, sN, sD);

esmf/src/Infrastructure/TimeMgr/src/ESMCI_Time.C

Line 1482 in 82f8052

sprintf(timeString, "%s%02d", timeString, s);

esmf/src/Infrastructure/TimeMgr/src/ESMCI_TimeInterval.C

Line 3142 in 82f8052

sprintf(timeString, "%s%.9fS", timeString, (s + fractionalSeconds));

esmf/src/Infrastructure/TimeMgr/src/ESMCI_TimeInterval.C

Line 3144 in 82f8052

sprintf(timeString, "%s%dS", timeString, s);

esmf/src/Infrastructure/TimeMgr/src/ESMCI_TimeInterval.C

Line 3151 in 82f8052

sprintf(timeString, "%s%d:%lld/%lldS", timeString, s, sN, sD);

esmf/src/Infrastructure/TimeMgr/src/ESMCI_TimeInterval.C

Line 3153 in 82f8052

sprintf(timeString, "%s%dS", timeString, s);

oehmke · 2024-09-03T22:28:47Z

Thanks Bill! Nice regex. :-) If it’s not hard, while you’re in there, you could think about switching to snprintf(), since sprintf() is deprecated (it is vulnerable to buffer overflows).

…

billsacks · 2024-09-05T23:24:14Z

I've started looking at this. @oehmke I'd like some confirmation on what I'm thinking here:

I'm thinking that, ideally, we should add an argument to these two functions:

esmf/src/Infrastructure/TimeMgr/src/ESMCI_Time.C

Lines 1390 to 1396 in edb848d

    
                 int Time::getString( 
        
           // 
        
           // !RETURN VALUE: 
        
           //    int error return code 
        
           // 
        
           // !ARGUMENTS: 
        
                 char *timeString, const char *options) const {    // out - time value in

esmf/src/Infrastructure/TimeMgr/src/ESMCI_TimeInterval.C

Lines 3072 to 3078 in edb848d

    
                 int TimeInterval::getString( 
        
           // 
        
           // !RETURN VALUE: 
        
           //    int error return code 
        
           // 
        
           // !ARGUMENTS: 
        
                 char *timeString, const char *options) const {    // out - time interval

giving the string length. It looks to me like we have that length available in all calls; for example:

esmf/src/Infrastructure/TimeMgr/src/ESMCI_Time.C

Line 433 in edb848d

int timeStringLen, // in - F90 time string size

which is referenced here:

esmf/src/Infrastructure/TimeMgr/src/ESMCI_Time.C

Lines 547 to 548 in edb848d

    
           if (tempTimeString != ESMC_NULL_POINTER && timeStringLen > 0) { 
        
             rc = Time::getString(tempTimeString);

But I'm not familiar enough with the Fortran-C connections in ESMF to be confident about this: can we rely on this timeStringLen variable being the size of the C character allocation for tempTimeString? Or maybe off-by-one because of the need to hold the terminating null character on the C side?? In case it's helpful, this is the relevant Fortran-C call:

esmf/src/Infrastructure/TimeMgr/interface/ESMCI_Time_F.C

Lines 91 to 161 in edb848d

    
           void FTN_X(c_esmc_timeget)(Time *ptr, 
        
                                  ESMC_I4 *yy, ESMC_I8 *yy_i8, 
        
                                  int *mm, int *dd, 
        
                                  ESMC_I4 *d,  ESMC_I8 *d_i8, 
        
                                  ESMC_I4 *h,  ESMC_I4 *m, 
        
                                  ESMC_I4 *s,  ESMC_I8 *s_i8, 
        
                                  ESMC_I4 *ms, ESMC_I4 *us, 
        
                                  ESMC_I4 *ns, 
        
                                  ESMC_R8 *d_r8,  ESMC_R8 *h_r8, 
        
                                  ESMC_R8 *m_r8,  ESMC_R8 *s_r8, 
        
                                  ESMC_R8 *ms_r8, ESMC_R8 *us_r8, 
        
                                  ESMC_R8 *ns_r8, 
        
                                  ESMC_I4 *sN, ESMC_I8 *sN_i8, 
        
                                  ESMC_I4 *sD, ESMC_I8 *sD_i8, 
        
                                  Calendar **calendar,  
        
                                  ESMC_CalKind_Flag *calkindflag,  
        
                                  int *timeZone, 
        
                                  int *timeStringLen, int *tempTimeStringLen,  
        
                                  char *tempTimeString, 
        
                                  int *timeStringLenISOFrac,  
        
                                  int *tempTimeStringLenISOFrac,  
        
                                  char *tempTimeStringISOFrac, 
        
                                  int *dayOfWeek, 
        
                                  Time *midMonth, 
        
                                  ESMC_I4 *dayOfYear, 
        
                                  ESMC_R8 *dayOfYear_r8, 
        
                                  TimeInterval *dayOfYear_intvl, 
        
                                  int *status, 
        
                                  ESMCI_FortranStrLenArg tempTime_l, 
        
                                  ESMCI_FortranStrLenArg tempTimeISOFrac_l) { 
        
              int rc = (ptr)->Time::get( 
        
                           ESMC_NOT_PRESENT_FILTER(yy), 
        
                           ESMC_NOT_PRESENT_FILTER(yy_i8), 
        
                           ESMC_NOT_PRESENT_FILTER(mm), 
        
                           ESMC_NOT_PRESENT_FILTER(dd), 
        
                           ESMC_NOT_PRESENT_FILTER(d), 
        
                           ESMC_NOT_PRESENT_FILTER(d_i8), 
        
                           ESMC_NOT_PRESENT_FILTER(h), 
        
                           ESMC_NOT_PRESENT_FILTER(m), 
        
                           ESMC_NOT_PRESENT_FILTER(s), 
        
                           ESMC_NOT_PRESENT_FILTER(s_i8), 
        
                           ESMC_NOT_PRESENT_FILTER(ms), 
        
                           ESMC_NOT_PRESENT_FILTER(us), 
        
                           ESMC_NOT_PRESENT_FILTER(ns), 
        
                           ESMC_NOT_PRESENT_FILTER(d_r8), 
        
                           ESMC_NOT_PRESENT_FILTER(h_r8), 
        
                           ESMC_NOT_PRESENT_FILTER(m_r8), 
        
                           ESMC_NOT_PRESENT_FILTER(s_r8), 
        
                           ESMC_NOT_PRESENT_FILTER(ms_r8), 
        
                           ESMC_NOT_PRESENT_FILTER(us_r8), 
        
                           ESMC_NOT_PRESENT_FILTER(ns_r8), 
        
                           ESMC_NOT_PRESENT_FILTER(sN), 
        
                           ESMC_NOT_PRESENT_FILTER(sN_i8), 
        
                           ESMC_NOT_PRESENT_FILTER(sD), 
        
                           ESMC_NOT_PRESENT_FILTER(sD_i8), 
        
                           ESMC_NOT_PRESENT_FILTER(calendar), 
        
                           ESMC_NOT_PRESENT_FILTER(calkindflag), 
        
                           ESMC_NOT_PRESENT_FILTER(timeZone), 
        
                                              // always present internal arguments 
        
                                                  *timeStringLen, 
        
                                                   tempTimeStringLen,  
        
                                                   tempTimeString, 
        
                                                  *timeStringLenISOFrac, 
        
                                                   tempTimeStringLenISOFrac,  
        
                                                   tempTimeStringISOFrac, 
        
                           ESMC_NOT_PRESENT_FILTER(dayOfWeek), 
        
                           ESMC_NOT_PRESENT_FILTER(midMonth), 
        
                           ESMC_NOT_PRESENT_FILTER(dayOfYear), 
        
                           ESMC_NOT_PRESENT_FILTER(dayOfYear_r8), 
        
                           ESMC_NOT_PRESENT_FILTER(dayOfYear_intvl) ); 
        
              if (ESMC_PRESENT(status)) *status = rc;

My sense is that these getString methods are only accessible internally, so it's okay to change the interface... Time::getString is a public method and appears like this:

esmf/src/Infrastructure/TimeMgr/src/ESMCI_Time.C

Lines 1381 to 1390 in edb848d

    
           //------------------------------------------------------------------------- 
        
           //  Public methods to support Time::get() API 
        
           //------------------------------------------------------------------------- 
        
           //------------------------------------------------------------------------- 
        
           //BOP 
        
           // !IROUTINE:  Time::getString - Get a Time value 
        
           // 
        
           // !INTERFACE: 
        
                 int Time::getString(

but my impression is that this C++ code isn't actually part of the ESMF public API; is that true?

Let me know if you'd find it easier to meet to talk about this rather than trying to reply in writing. I have other things I can work on while waiting for a reply, so no rush on this. (I thought maybe if I got this done quickly I could sneak this bug fix into 8.7, but that may be unrealistic at this point.)

mathomp4 · 2024-09-06T12:25:07Z

Just a note. We had another user on Linux 6.8+ (@gmao-ckung) hit this, so it might become more prevalent!

Thanks for looking at this!

oehmke · 2024-09-06T22:07:01Z

Hi Bill, Yes, that makes sense to add the argument with the string length. We trust that Fortran length other places in the code and we don’t seem to have issues with it, so I think it’s ok to use. I think that it’s just the length without the null terminator, but I’ll have to look a bit to be sure. We have some interface functions to help with the F to C strings, but I’ll have to remind myself about how it works (I don’t do it super often), but I can find some examples for you to use. Are you in T or Th next week? How about we meet to talk about it and look at some code? That’ll give me time to swap it back into main memory. ;-) - Bob

…

On Sep 5, 2024, at 5:24 PM, Bill Sacks ***@***.***> wrote: I've started looking at this. @oehmke <https://github.com/oehmke> I'd like some confirmation on what I'm thinking here: I'm thinking that, ideally, we should add an argument to these two functions: https://github.com/esmf-org/esmf/blob/edb848d90bf5d70a2db158771f4d07be605fdd32/src/Infrastructure/TimeMgr/src/ESMCI_Time.C#L1390-L1396 https://github.com/esmf-org/esmf/blob/edb848d90bf5d70a2db158771f4d07be605fdd32/src/Infrastructure/TimeMgr/src/ESMCI_TimeInterval.C#L3072-L3078 giving the string length. It looks to me like we have that length available in all calls; for example: https://github.com/esmf-org/esmf/blob/edb848d90bf5d70a2db158771f4d07be605fdd32/src/Infrastructure/TimeMgr/src/ESMCI_Time.C#L433 which is referenced here: https://github.com/esmf-org/esmf/blob/edb848d90bf5d70a2db158771f4d07be605fdd32/src/Infrastructure/TimeMgr/src/ESMCI_Time.C#L547-L548 But I'm not familiar enough with the Fortran-C connections in ESMF to be confident about this: can we rely on this timeStringLen variable being the size of the C character allocation for tempTimeString? Or maybe off-by-one because of the need to hold the terminating null character on the C side?? In case it's helpful, this is the relevant Fortran-C call: https://github.com/esmf-org/esmf/blob/edb848d90bf5d70a2db158771f4d07be605fdd32/src/Infrastructure/TimeMgr/interface/ESMCI_Time_F.C#L91-L161 My sense is that these getString methods are only accessible internally, so it's okay to change the interface... Time::getString is a public method and appears like this: https://github.com/esmf-org/esmf/blob/edb848d90bf5d70a2db158771f4d07be605fdd32/src/Infrastructure/TimeMgr/src/ESMCI_Time.C#L1381-L1390 but my impression is that this C++ code isn't actually part of the ESMF public API; is that true? Let me know if you'd find it easier to meet to talk about this rather than trying to reply in writing. I have other things I can work on while waiting for a reply, so no rush on this. (I thought maybe if I got this done quickly I could sneak this bug fix into 8.7, but that may be unrealistic at this point.) — Reply to this email directly, view it on GitHub <#284 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE6A7U5P4CACHDXOWMDLL7LZVDR3HAVCNFSM6AAAAABNS75RHWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZSHA3TGMRXGE>. You are receiving this because you were mentioned.

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 #284

oehmke assigned billsacks Sep 3, 2024

billsacks mentioned this issue Sep 17, 2024

Fix sprintf issues #298

Merged

billsacks closed this as completed in #298 Sep 19, 2024

billsacks closed this as completed in 9bc54ab Sep 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timemgr using sprintf with the output string also used in the input #284

Timemgr using sprintf with the output string also used in the input #284

oehmke commented Sep 3, 2024

billsacks commented Sep 3, 2024

oehmke commented Sep 3, 2024 via email

billsacks commented Sep 5, 2024

mathomp4 commented Sep 6, 2024

oehmke commented Sep 6, 2024 via email

Timemgr using sprintf with the output string also used in the input #284

Timemgr using sprintf with the output string also used in the input #284

Comments

oehmke commented Sep 3, 2024

billsacks commented Sep 3, 2024

oehmke commented Sep 3, 2024 via email

billsacks commented Sep 5, 2024

mathomp4 commented Sep 6, 2024

oehmke commented Sep 6, 2024 via email