From d38df0732d2713a7520a65aee6c78769026f5932 Mon Sep 17 00:00:00 2001 From: Stuart Baker Date: Sat, 4 Nov 2023 12:52:50 -0500 Subject: [PATCH 1/4] Improve the error handling and/or recovery of BroadcastTimeDefs::string_to_date(). --- src/openlcb/BroadcastTimeDefs.cxx | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/openlcb/BroadcastTimeDefs.cxx b/src/openlcb/BroadcastTimeDefs.cxx index b40a665c2..c73d5b7ca 100644 --- a/src/openlcb/BroadcastTimeDefs.cxx +++ b/src/openlcb/BroadcastTimeDefs.cxx @@ -189,11 +189,33 @@ int16_t BroadcastTimeDefs::string_to_rate_quarters(const std::string &srate) bool BroadcastTimeDefs::string_to_date( const std::string &sdate, int *year, int *month, int *day) { - struct tm tm = {}; + struct tm tm; + memset(&tm, 0, sizeof(tm)); if (strptime(sdate.c_str(), "%b %e, %Y", &tm) == nullptr) { return false; } + + // newlib does not have the proper boundary checking for strptime(). + // Therefore we use mktime() to determine if the time we have is really + // valid or not. In newlib, mktime() can actually correct some invalid + // struct tm values by making some educated guesses. + // + // While glibc does have proper boundary checking for strptime(), it + // can still use mktime() to correct some invalid struct tm values by + // making some educated guesses. + // + time_t t = mktime(&tm); + + // newlib doe not correctly set the errno value when mktime() + // encounters an error. Instead it "only" returns -1, which is technically + // a valid time. We are counting on the fact that we zeroed out the struct + // tm above, and subsequently -1 cannot be an expected result. + if (t == (time_t)-1) + { + return false; + } + if (tm.tm_year < (0 - 1900) || tm.tm_year > (4095 - 1900)) { // Out of range for openlcb. From 4f44dfd83118c59ca08bf2a8ff0dd1ffc57db504 Mon Sep 17 00:00:00 2001 From: Stuart Baker Date: Sat, 4 Nov 2023 12:53:20 -0500 Subject: [PATCH 2/4] Fix tests. --- src/openlcb/BroadcastTimeDefs.cxxtest | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/openlcb/BroadcastTimeDefs.cxxtest b/src/openlcb/BroadcastTimeDefs.cxxtest index 60cb03636..bdf727e3e 100644 --- a/src/openlcb/BroadcastTimeDefs.cxxtest +++ b/src/openlcb/BroadcastTimeDefs.cxxtest @@ -165,9 +165,10 @@ TEST_F(BroadcastTimeDefsTest, string_to_date) parse_date("Feb 29, 2024", 2024, 2, 29); // Not leap year - parse_date("Feb 29, 2023", 2023, 2, 29); + parse_date("Feb 29, 2023", 2023, 3, 1); + // No canonicalization in glibc - parse_date("Feb 31, 2023", 2023, 2, 31); + parse_date("Feb 31, 2023", 2023, 3, 3); } TEST_F(BroadcastTimeDefsTest, string_to_date_fail) @@ -202,9 +203,9 @@ TEST_F(BroadcastTimeDefsTest, date_canonicalize) EXPECT_FALSE(canon_date("Aug 12, 2023")); EXPECT_TRUE(canon_date("Aug 12, \n\t 2023", "Aug 12, 2023")); - EXPECT_FALSE(canon_date("Feb 31, 2023")); - EXPECT_FALSE(canon_date("Feb 31, 23")); - EXPECT_FALSE(canon_date("Feb 31, 3023")); + EXPECT_TRUE(canon_date("Feb 31, 2023", "Mar 3, 2023")); + EXPECT_TRUE(canon_date("Feb 31, 23", "Mar 3, 23")); + EXPECT_TRUE(canon_date("Feb 31, 3023", "Mar 3, 3023")); EXPECT_TRUE(canon_date("Aug 9, 2023", "Aug 9, 2023")); From d0e3ae9c9c2b3ba311f48ce9178de15416b7ee96 Mon Sep 17 00:00:00 2001 From: Stuart Baker Date: Sat, 4 Nov 2023 12:55:10 -0500 Subject: [PATCH 3/4] Temove trivial white space. --- src/openlcb/BroadcastTimeDefs.cxx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/openlcb/BroadcastTimeDefs.cxx b/src/openlcb/BroadcastTimeDefs.cxx index c73d5b7ca..a4c6eb446 100644 --- a/src/openlcb/BroadcastTimeDefs.cxx +++ b/src/openlcb/BroadcastTimeDefs.cxx @@ -204,7 +204,6 @@ bool BroadcastTimeDefs::string_to_date( // While glibc does have proper boundary checking for strptime(), it // can still use mktime() to correct some invalid struct tm values by // making some educated guesses. - // time_t t = mktime(&tm); // newlib doe not correctly set the errno value when mktime() From dd24694505b1f3fdabe7409a1a94691d2cc4fa21 Mon Sep 17 00:00:00 2001 From: Stuart Baker Date: Sat, 11 Nov 2023 16:44:45 -0600 Subject: [PATCH 4/4] Fix review comments. --- src/openlcb/BroadcastTimeDefs.cxx | 2 +- src/openlcb/BroadcastTimeDefs.cxxtest | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openlcb/BroadcastTimeDefs.cxx b/src/openlcb/BroadcastTimeDefs.cxx index a4c6eb446..0f7f21178 100644 --- a/src/openlcb/BroadcastTimeDefs.cxx +++ b/src/openlcb/BroadcastTimeDefs.cxx @@ -206,7 +206,7 @@ bool BroadcastTimeDefs::string_to_date( // making some educated guesses. time_t t = mktime(&tm); - // newlib doe not correctly set the errno value when mktime() + // newlib does not correctly set the errno value when mktime() // encounters an error. Instead it "only" returns -1, which is technically // a valid time. We are counting on the fact that we zeroed out the struct // tm above, and subsequently -1 cannot be an expected result. diff --git a/src/openlcb/BroadcastTimeDefs.cxxtest b/src/openlcb/BroadcastTimeDefs.cxxtest index bdf727e3e..796a2c15c 100644 --- a/src/openlcb/BroadcastTimeDefs.cxxtest +++ b/src/openlcb/BroadcastTimeDefs.cxxtest @@ -167,7 +167,7 @@ TEST_F(BroadcastTimeDefsTest, string_to_date) // Not leap year parse_date("Feb 29, 2023", 2023, 3, 1); - // No canonicalization in glibc + // canonize parse_date("Feb 31, 2023", 2023, 3, 3); }