From 2c802a8eaab37025db2ccc0366cd4072b417f18f Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Thu, 18 Jul 2024 17:34:09 +0200 Subject: [PATCH 1/3] tostring() expression function: validate format string, and make sure buffer is large enough --- src/mapparser.c | 43 +++++++++++++++++--------------- src/mapparser.y | 8 ++++-- src/mapserver.h | 1 + src/mapstring.cpp | 63 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 22 deletions(-) diff --git a/src/mapparser.c b/src/mapparser.c index 62cffd466a..f5175e3f02 100644 --- a/src/mapparser.c +++ b/src/mapparser.c @@ -557,8 +557,8 @@ static const yytype_uint16 yyrline[] = 672, 689, 705, 723, 724, 725, 726, 727, 728, 729, 736, 737, 738, 739, 750, 751, 754, 755, 756, 770, 784, 798, 812, 826, 840, 854, 868, 882, 896, 910, - 924, 939, 961, 962, 963, 969, 974, 978, 982, 986, - 990, 996, 997 + 924, 939, 961, 962, 963, 969, 978, 982, 986, 990, + 994, 1000, 1001 }; #endif @@ -2892,66 +2892,70 @@ YYSTYPE yylval YY_INITIAL_VALUE (= yyval_default); case 95: #line 969 "mapparser.y" /* yacc.c:1646 */ { - (yyval.strval) = (char *) malloc(strlen((yyvsp[-1].strval)) + 64); /* Plenty big? Should use snprintf below... */ - sprintf((yyval.strval), (yyvsp[-1].strval), (yyvsp[-3].dblval)); + char* ret = msToString((yyvsp[-1].strval), (yyvsp[-3].dblval)); msReplaceFreeableStr(&((yyvsp[-1].strval)), NULL); + if(!ret) { + yyerror(p, "tostring() failed."); + return(-1); + } + (yyval.strval) = ret; } -#line 2900 "/vagrant/mapparser.c" /* yacc.c:1646 */ +#line 2904 "/vagrant/mapparser.c" /* yacc.c:1646 */ break; case 96: -#line 974 "mapparser.y" /* yacc.c:1646 */ +#line 978 "mapparser.y" /* yacc.c:1646 */ { (yyvsp[-1].strval) = msCommifyString((yyvsp[-1].strval)); (yyval.strval) = (yyvsp[-1].strval); } -#line 2909 "/vagrant/mapparser.c" /* yacc.c:1646 */ +#line 2913 "/vagrant/mapparser.c" /* yacc.c:1646 */ break; case 97: -#line 978 "mapparser.y" /* yacc.c:1646 */ +#line 982 "mapparser.y" /* yacc.c:1646 */ { msStringToUpper((yyvsp[-1].strval)); (yyval.strval) = (yyvsp[-1].strval); } -#line 2918 "/vagrant/mapparser.c" /* yacc.c:1646 */ +#line 2922 "/vagrant/mapparser.c" /* yacc.c:1646 */ break; case 98: -#line 982 "mapparser.y" /* yacc.c:1646 */ +#line 986 "mapparser.y" /* yacc.c:1646 */ { msStringToLower((yyvsp[-1].strval)); (yyval.strval) = (yyvsp[-1].strval); } -#line 2927 "/vagrant/mapparser.c" /* yacc.c:1646 */ +#line 2931 "/vagrant/mapparser.c" /* yacc.c:1646 */ break; case 99: -#line 986 "mapparser.y" /* yacc.c:1646 */ +#line 990 "mapparser.y" /* yacc.c:1646 */ { msStringInitCap((yyvsp[-1].strval)); (yyval.strval) = (yyvsp[-1].strval); } -#line 2936 "/vagrant/mapparser.c" /* yacc.c:1646 */ +#line 2940 "/vagrant/mapparser.c" /* yacc.c:1646 */ break; case 100: -#line 990 "mapparser.y" /* yacc.c:1646 */ +#line 994 "mapparser.y" /* yacc.c:1646 */ { msStringFirstCap((yyvsp[-1].strval)); (yyval.strval) = (yyvsp[-1].strval); } -#line 2945 "/vagrant/mapparser.c" /* yacc.c:1646 */ +#line 2949 "/vagrant/mapparser.c" /* yacc.c:1646 */ break; case 102: -#line 997 "mapparser.y" /* yacc.c:1646 */ +#line 1001 "mapparser.y" /* yacc.c:1646 */ { (yyval.tmval) = (yyvsp[-1].tmval); } -#line 2951 "/vagrant/mapparser.c" /* yacc.c:1646 */ +#line 2955 "/vagrant/mapparser.c" /* yacc.c:1646 */ break; -#line 2955 "/vagrant/mapparser.c" /* yacc.c:1646 */ +#line 2959 "/vagrant/mapparser.c" /* yacc.c:1646 */ default: break; } /* User semantic actions sometimes alter yychar, and that requires @@ -3000,7 +3004,6 @@ YYSTYPE yylval YY_INITIAL_VALUE (= yyval_default); if (!yyerrstatus) { ++yynerrs; - if(0) printf("%d", yynerrs); /* avoid warning about unused yynerrs */ #if ! YYERROR_VERBOSE yyerror (p, YY_("syntax error")); #else @@ -3180,7 +3183,7 @@ YYSTYPE yylval YY_INITIAL_VALUE (= yyval_default); #endif return yyresult; } -#line 1000 "mapparser.y" /* yacc.c:1906 */ +#line 1004 "mapparser.y" /* yacc.c:1906 */ /* diff --git a/src/mapparser.y b/src/mapparser.y index a5190eed9d..62f8f9fb8f 100644 --- a/src/mapparser.y +++ b/src/mapparser.y @@ -967,9 +967,13 @@ string_exp: STRING msReplaceFreeableStr(&($3), NULL); } | TOSTRING '(' math_exp ',' string_exp ')' { - $$ = (char *) malloc(strlen($5) + 64); /* Plenty big? Should use snprintf below... */ - sprintf($$, $5, $3); + char* ret = msToString($5, $3); msReplaceFreeableStr(&($5), NULL); + if(!ret) { + yyerror(p, "tostring() failed."); + return(-1); + } + $$ = ret; } | COMMIFY '(' string_exp ')' { $3 = msCommifyString($3); diff --git a/src/mapserver.h b/src/mapserver.h index d99f50faf2..aab530821d 100644 --- a/src/mapserver.h +++ b/src/mapserver.h @@ -2879,6 +2879,7 @@ MS_DLL_EXPORT char *msJoinStrings(char **array, int arrayLength, const char *delimiter); MS_DLL_EXPORT char *msHashString(const char *pszStr); MS_DLL_EXPORT char *msCommifyString(char *str); +MS_DLL_EXPORT char *msToString(const char *format, double value); MS_DLL_EXPORT int msHexToInt(char *hex); MS_DLL_EXPORT char *msGetEncodedString(const char *string, const char *encoding); diff --git a/src/mapstring.cpp b/src/mapstring.cpp index 398cb35432..9f076148f6 100644 --- a/src/mapstring.cpp +++ b/src/mapstring.cpp @@ -37,6 +37,7 @@ #include "cpl_vsi.h" #include +#include #include #include @@ -1548,6 +1549,68 @@ char *msCommifyString(char *str) { return str; } +/************************************************************************/ +/* msToString() */ +/************************************************************************/ + +char *msToString(const char *format, double value) { + bool pctAlreadyFound = false; + // Validate that the formatting string is OK for a single input double value + int extra_size = 0; + for (const char *ptr = format; *ptr; ++ptr) { + if (*ptr == '%' && ptr[1] == '%') { + ++ptr; + } else if (*ptr == '%') { + if (pctAlreadyFound) { + msSetError(MS_MISCERR, "More than one conversion specifier", + "msToString()"); + return nullptr; + } + pctAlreadyFound = true; + ++ptr; + // Skip flag characters + while (*ptr == '+' || *ptr == '-' || *ptr == ' ' || *ptr == '\'' || + *ptr == '0') { + ++ptr; + } + // Skip width + if (*ptr >= '1' && *ptr <= '9') { + extra_size = atoi(ptr); + do { + ++ptr; + } while (*ptr >= '0' && *ptr <= '9'); + if (extra_size > 1024) { + // To avoid arbitrary memory allocatin + msSetError(MS_MISCERR, "Too large width", "msToString()"); + return nullptr; + } + } + // maximum double value is of the order of ~1e308 + if (extra_size < DBL_MAX_10_EXP) + extra_size = DBL_MAX_10_EXP; + extra_size += 32; // extra margin + + // Skip precision + if (*ptr == '.') { + ++ptr; + while (*ptr >= '0' && *ptr <= '9') + ++ptr; + } + // Check conversion specifier + if (!(*ptr == 'e' || *ptr == 'E' || *ptr == 'f' || *ptr == 'F' || + *ptr == 'g' || *ptr == 'G')) { + msSetError(MS_MISCERR, "Invalid conversion specifier", "msToString()"); + return nullptr; + } + } + } + // extra_size / 3 if thousands' grouping characters is used + const size_t nBufferSize = strlen(format) + extra_size + (extra_size / 3) + 1; + char *ret = static_cast(msSmallMalloc(nBufferSize)); + snprintf(ret, nBufferSize, format, value); + return ret; +} + /* ------------------------------------------------------------------------------- */ /* Replace all occurrences of old with new in str. */ From 6a1a7efdd0c73770b1ed6714643b48d950843b13 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Thu, 18 Jul 2024 18:04:04 +0200 Subject: [PATCH 2/3] Add unit tests for msToString() --- tests/unit/test.cpp | 75 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/tests/unit/test.cpp b/tests/unit/test.cpp index 4644b975d7..24721cf318 100644 --- a/tests/unit/test.cpp +++ b/tests/unit/test.cpp @@ -19,6 +19,17 @@ static void EXPECT_STREQ_func(const char *got, const char *expected, #define EXPECT_STREQ(got, expected) \ EXPECT_STREQ_func(got, expected, __func__, __LINE__) +static void EXPECT_TRUE_FUNC(bool cond, const char *condstr, + const char *function, int line) { + if (!cond) { + fprintf(stderr, "EXPECT_TRUE(\"%s\") failed at %s:%d\n", condstr, function, + line); + gTestRetCode = 1; + } +} + +#define EXPECT_TRUE(cond) EXPECT_TRUE_FUNC(cond, #cond, __func__, __LINE__) + /* ----------------------------------------------------------------------- */ static void testRedactCredentials() { @@ -68,7 +79,71 @@ static void testRedactCredentials() { /* ----------------------------------------------------------------------- */ +static void testToString() { + { + char *ret = msToString("x%f %%y", 1.5); + EXPECT_STREQ(ret, "x1.500000 %y"); + msFree(ret); + } + { + char *ret = msToString("%+f", 1.5); + EXPECT_STREQ(ret, "+1.500000"); + msFree(ret); + } + { + char *ret = msToString("%5.2f", 1.5); + EXPECT_STREQ(ret, " 1.50"); + msFree(ret); + } + { + char *ret = msToString("%f", 1e308); + EXPECT_STREQ( + ret, "10000000000000000109790636294404554174049230967731184633681068290" + "31575854049114915371633289784946888990612496697211725156115902837" + "43140088328307009198146046031271664502933027185697489699588559043" + "33838446616500117842689762621294517762809119578670745812278397017" + "1784415105291802893207873272974885715430223118336.000000"); + msFree(ret); + } + { + char *ret = msToString("%1f", 1e308); + EXPECT_STREQ( + ret, "10000000000000000109790636294404554174049230967731184633681068290" + "31575854049114915371633289784946888990612496697211725156115902837" + "43140088328307009198146046031271664502933027185697489699588559043" + "33838446616500117842689762621294517762809119578670745812278397017" + "1784415105291802893207873272974885715430223118336.000000"); + msFree(ret); + } + { + char *ret = msToString("%320f", 1e308); + EXPECT_STREQ( + ret, " " + "10000000000000000109790636294404554174049230967731184633681068290" + "31575854049114915371633289784946888990612496697211725156115902837" + "43140088328307009198146046031271664502933027185697489699588559043" + "33838446616500117842689762621294517762809119578670745812278397017" + "1784415105291802893207873272974885715430223118336.000000"); + msFree(ret); + } + { + char *ret = msToString("%f%f", 1); + EXPECT_TRUE(ret == nullptr); + } + { + char *ret = msToString("%s", 1); + EXPECT_TRUE(ret == nullptr); + } + { + char *ret = msToString("%100000f", 1); + EXPECT_TRUE(ret == nullptr); + } +} + +/* ----------------------------------------------------------------------- */ + int main() { testRedactCredentials(); + testToString(); return gTestRetCode; } From eb64cc1f0b597ec0e61bb5aacb901c194fe39b75 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Fri, 19 Jul 2024 18:19:35 +0200 Subject: [PATCH 3/3] CMakeLists.txt: fix build of unit_test --- CMakeLists.txt | 1 - tests/unit/test.cpp | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index da3e95a67b..7cb5a5a664 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1163,7 +1163,6 @@ option(BUILD_TESTING "Build unit test" ON) if(BUILD_DYNAMIC AND BUILD_TESTING) enable_testing() add_executable(unit_test tests/unit/test.cpp) - target_include_directories(unit_test PRIVATE src) target_link_libraries(unit_test PRIVATE mapserver) add_test(NAME unit_test COMMAND unit_test) endif() diff --git a/tests/unit/test.cpp b/tests/unit/test.cpp index 24721cf318..46ebf1ac7c 100644 --- a/tests/unit/test.cpp +++ b/tests/unit/test.cpp @@ -1,5 +1,5 @@ -#include "mapserver.h" -#include "maperror.h" +#include "../../src/mapserver.h" +#include "../../src/maperror.h" /* ----------------------------------------------------------------------- */