From f13d510cf267c27f4c123494de67670ec201cedc Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Wed, 20 Jan 2016 11:34:01 -0800 Subject: Bugs in CJS_PublicMethods::ParseNumber(). Fix the bugs by removing ParseNumber() entirely. For PDFium's JavaScript bindings, we want to get out of the numeric conversion business and inflict that on V8 as possible, avoiding platform-specific issue in strtod(). For other uses, there is a FX_atof() which is similarly buggy, but we can consolidate the use. Add an overloaded FX_atof() to handle wide strings more simply. BUG=pdfium:361 R=jochen@chromium.org Review URL: https://codereview.chromium.org/1586203006 . --- core/include/fxcrt/fx_string.h | 11 +- fpdfsdk/src/javascript/Field.cpp | 50 +---- fpdfsdk/src/javascript/JS_Value.cpp | 30 ++- fpdfsdk/src/javascript/JS_Value.h | 5 + fpdfsdk/src/javascript/PublicMethods.cpp | 240 ++++------------------ fpdfsdk/src/javascript/PublicMethods.h | 9 - testing/resources/javascript/bug_361_expected.txt | 24 +-- 7 files changed, 97 insertions(+), 272 deletions(-) diff --git a/core/include/fxcrt/fx_string.h b/core/include/fxcrt/fx_string.h index 1c1e237076..6abcf52c76 100644 --- a/core/include/fxcrt/fx_string.h +++ b/core/include/fxcrt/fx_string.h @@ -794,9 +794,7 @@ inline bool operator!=(const wchar_t* lhs, const CFX_WideString& rhs) { inline bool operator!=(const CFX_WideStringC& lhs, const CFX_WideString& rhs) { return rhs != lhs; } -FX_FLOAT FX_atof(const CFX_ByteStringC& str); -void FX_atonum(const CFX_ByteStringC& str, FX_BOOL& bInteger, void* pData); -FX_STRSIZE FX_ftoa(FX_FLOAT f, FX_CHAR* buf); + CFX_ByteString FX_UTF8Encode(const FX_WCHAR* pwsStr, FX_STRSIZE len); inline CFX_ByteString FX_UTF8Encode(const CFX_WideStringC& wsStr) { return FX_UTF8Encode(wsStr.GetPtr(), wsStr.GetLength()); @@ -805,4 +803,11 @@ inline CFX_ByteString FX_UTF8Encode(const CFX_WideString& wsStr) { return FX_UTF8Encode(wsStr.c_str(), wsStr.GetLength()); } +FX_FLOAT FX_atof(const CFX_ByteStringC& str); +inline FX_FLOAT FX_atof(const CFX_WideStringC& wsStr) { + return FX_atof(FX_UTF8Encode(wsStr.GetPtr(), wsStr.GetLength())); +} +void FX_atonum(const CFX_ByteStringC& str, FX_BOOL& bInteger, void* pData); +FX_STRSIZE FX_ftoa(FX_FLOAT f, FX_CHAR* buf); + #endif // CORE_INCLUDE_FXCRT_FX_STRING_H_ diff --git a/fpdfsdk/src/javascript/Field.cpp b/fpdfsdk/src/javascript/Field.cpp index 959e8dcadd..d97001aae2 100644 --- a/fpdfsdk/src/javascript/Field.cpp +++ b/fpdfsdk/src/javascript/Field.cpp @@ -2738,19 +2738,7 @@ FX_BOOL Field::value(IJS_Context* cc, return FALSE; case FIELDTYPE_COMBOBOX: case FIELDTYPE_TEXTFIELD: { - CFX_WideString swValue = pFormField->GetValue(); - - double dRet; - FX_BOOL bDot; - if (CJS_PublicMethods::ConvertStringToNumber(swValue.c_str(), dRet, - bDot)) { - if (bDot) - vp << dRet; - else - vp << dRet; - } else { - vp << swValue; - } + vp << pFormField->GetValue(); } break; case FIELDTYPE_LISTBOX: { if (pFormField->CountSelectedItems() > 1) { @@ -2766,40 +2754,18 @@ FX_BOOL Field::value(IJS_Context* cc, } vp << ValueArray; } else { - CFX_WideString swValue = pFormField->GetValue(); - - double dRet; - FX_BOOL bDot; - if (CJS_PublicMethods::ConvertStringToNumber(swValue.c_str(), dRet, - bDot)) { - if (bDot) - vp << dRet; - else - vp << dRet; - } else { - vp << swValue; - } + vp << pFormField->GetValue(); } } break; case FIELDTYPE_CHECKBOX: case FIELDTYPE_RADIOBUTTON: { - FX_BOOL bFind = FALSE; + bool bFind = false; for (int i = 0, sz = pFormField->CountControls(); i < sz; i++) { - if (!pFormField->GetControl(i)->IsChecked()) - continue; - - CFX_WideString swValue = pFormField->GetControl(i)->GetExportValue(); - double dRet; - FX_BOOL bDotDummy; - if (CJS_PublicMethods::ConvertStringToNumber(swValue.c_str(), dRet, - bDotDummy)) { - vp << dRet; - } else { - vp << swValue; + if (pFormField->GetControl(i)->IsChecked()) { + vp << pFormField->GetControl(i)->GetExportValue(); + bFind = true; + break; } - - bFind = TRUE; - break; } if (!bFind) vp << L"Off"; @@ -2809,7 +2775,7 @@ FX_BOOL Field::value(IJS_Context* cc, break; } } - + vp.MaybeCoerceToNumber(); return TRUE; } diff --git a/fpdfsdk/src/javascript/JS_Value.cpp b/fpdfsdk/src/javascript/JS_Value.cpp index bd00adcf49..cfa565e4d4 100644 --- a/fpdfsdk/src/javascript/JS_Value.cpp +++ b/fpdfsdk/src/javascript/JS_Value.cpp @@ -19,8 +19,6 @@ static double GetNan() { return *(double*)g_nan; } -/* ---------------------------- CJS_Value ---------------------------- */ - CJS_Value::CJS_Value(CJS_Runtime* pRuntime) : m_eType(VT_unknown), m_pJSRuntime(pRuntime) { } @@ -98,9 +96,6 @@ void CJS_Value::Detach() { m_eType = VT_unknown; } -/* ---------------------------------------------------------------------------------------- - */ - int CJS_Value::ToInt() const { return FXJS_ToInt32(m_pJSRuntime->GetIsolate(), m_pValue); } @@ -146,8 +141,26 @@ v8::Local CJS_Value::ToV8Array() const { return v8::Local(); } -/* ---------------------------------------------------------------------------------------- - */ +void CJS_Value::MaybeCoerceToNumber() { + bool bAllowNaN = false; + if (m_eType == VT_string) { + CFX_ByteString bstr = ToCFXByteString(); + if (bstr.GetLength() == 0) + return; + if (bstr == "NaN") + bAllowNaN = true; + } + v8::TryCatch(m_pJSRuntime->GetIsolate()); + v8::MaybeLocal maybeNum = + m_pValue->ToNumber(m_pJSRuntime->GetIsolate()->GetCurrentContext()); + if (maybeNum.IsEmpty()) + return; + v8::Local num = maybeNum.ToLocalChecked(); + if (std::isnan(num->Value()) && !bAllowNaN) + return; + m_pValue = num; + m_eType = VT_number; +} void CJS_Value::operator=(int iValue) { m_pValue = FXJS_NewNumber(m_pJSRuntime->GetIsolate(), iValue); @@ -217,9 +230,6 @@ void CJS_Value::operator=(CJS_Value value) { m_pJSRuntime = value.m_pJSRuntime; } -/* ---------------------------------------------------------------------------------------- - */ - CJS_Value::Type CJS_Value::GetType() const { if (m_pValue.IsEmpty()) return VT_unknown; diff --git a/fpdfsdk/src/javascript/JS_Value.h b/fpdfsdk/src/javascript/JS_Value.h index 8517b76dac..20a6e38b46 100644 --- a/fpdfsdk/src/javascript/JS_Value.h +++ b/fpdfsdk/src/javascript/JS_Value.h @@ -62,6 +62,11 @@ class CJS_Value { v8::Local ToV8Array() const; v8::Local ToV8Value() const; + // Replace the current |m_pValue| with a v8::Number if possible + // to make one from the current |m_pValue|, updating |m_eType| + // as appropriate to indicate the result. + void MaybeCoerceToNumber(); + void operator=(int iValue); void operator=(bool bValue); void operator=(double); diff --git a/fpdfsdk/src/javascript/PublicMethods.cpp b/fpdfsdk/src/javascript/PublicMethods.cpp index d2f7fb09ac..3c6d36fdd8 100644 --- a/fpdfsdk/src/javascript/PublicMethods.cpp +++ b/fpdfsdk/src/javascript/PublicMethods.cpp @@ -194,149 +194,6 @@ CFX_ByteString CJS_PublicMethods::StrTrim(const FX_CHAR* pStr) { return StrRTrim(StrLTrim(pStr)); } -double CJS_PublicMethods::ParseNumber(const FX_WCHAR* swSource, - FX_BOOL& bAllDigits, - FX_BOOL& bDot, - FX_BOOL& bSign, - FX_BOOL& bKXJS) { - bDot = FALSE; - bSign = FALSE; - bKXJS = FALSE; - - FX_BOOL bDigitExist = FALSE; - - const FX_WCHAR* p = swSource; - wchar_t c; - - const FX_WCHAR* pStart = NULL; - const FX_WCHAR* pEnd = NULL; - - while ((c = *p)) { - if (!pStart && c != L' ') { - pStart = p; - } - - pEnd = p; - p++; - } - - if (!pStart) { - bAllDigits = FALSE; - return 0; - } - - while (pEnd != pStart) { - if (*pEnd == L' ') - pEnd--; - else - break; - } - - double dRet = 0; - p = pStart; - bAllDigits = TRUE; - CFX_WideString swDigits; - - while (p <= pEnd) { - c = *p; - - if (FXSYS_iswdigit(c)) { - swDigits += c; - bDigitExist = TRUE; - } else { - switch (c) { - case L' ': - bAllDigits = FALSE; - break; - case L'.': - case L',': - if (!bDot) { - if (bDigitExist) { - swDigits += L'.'; - } else { - swDigits += L'0'; - swDigits += L'.'; - bDigitExist = TRUE; - } - - bDot = TRUE; - break; - } - case 'e': - case 'E': - if (!bKXJS) { - p++; - c = *p; - if (c == '+' || c == '-') { - bKXJS = TRUE; - swDigits += 'e'; - swDigits += c; - } - break; - } - case L'-': - if (!bDigitExist && !bSign) { - swDigits += c; - bSign = TRUE; - break; - } - default: - bAllDigits = FALSE; - - if (p != pStart && !bDot && bDigitExist) { - swDigits += L'.'; - bDot = TRUE; - } else { - bDot = FALSE; - bDigitExist = FALSE; - swDigits = L""; - } - break; - } - } - - p++; - } - - if (swDigits.GetLength() > 0 && swDigits.GetLength() < 17) { - CFX_ByteString sDigits = swDigits.UTF8Encode(); - - if (bKXJS) { - dRet = atof(sDigits); - } else { - if (bDot) { - char* pStopString; - dRet = ::strtod(sDigits, &pStopString); - } else { - dRet = atol(sDigits); - } - } - } - - return dRet; -} - -double CJS_PublicMethods::ParseStringToNumber(const FX_WCHAR* swSource) { - FX_BOOL bAllDigits = FALSE; - FX_BOOL bDot = FALSE; - FX_BOOL bSign = FALSE; - FX_BOOL bKXJS = FALSE; - - return ParseNumber(swSource, bAllDigits, bDot, bSign, bKXJS); -} - -FX_BOOL CJS_PublicMethods::ConvertStringToNumber(const FX_WCHAR* swSource, - double& dRet, - FX_BOOL& bDot) { - FX_BOOL bAllDigits = FALSE; - FX_BOOL bSign = FALSE; - FX_BOOL bKXJS = FALSE; - - dRet = ParseNumber(swSource, bAllDigits, bDot, bSign, bKXJS); - - return bAllDigits; -} - CJS_Array CJS_PublicMethods::AF_MakeArrayFromList(CJS_Runtime* pRuntime, CJS_Value val) { CJS_Array StrArray(pRuntime); @@ -1344,71 +1201,57 @@ FX_BOOL CJS_PublicMethods::AFDate_FormatEx(IJS_Context* cc, } double CJS_PublicMethods::MakeInterDate(CFX_WideString strValue) { - int nHour; - int nMin; - int nSec; - int nYear; - int nMonth; - int nDay; - CFX_WideStringArray wsArray; - CFX_WideString sMonth = L""; CFX_WideString sTemp = L""; - int nSize = strValue.GetLength(); - - for (int i = 0; i < nSize; i++) { + for (int i = 0; i < strValue.GetLength(); ++i) { FX_WCHAR c = strValue.GetAt(i); if (c == L' ' || c == L':') { wsArray.Add(sTemp); sTemp = L""; continue; } - sTemp += c; } - wsArray.Add(sTemp); if (wsArray.GetSize() != 8) return 0; + int nMonth = 1; sTemp = wsArray[1]; if (sTemp.Compare(L"Jan") == 0) nMonth = 1; - if (sTemp.Compare(L"Feb") == 0) + else if (sTemp.Compare(L"Feb") == 0) nMonth = 2; - if (sTemp.Compare(L"Mar") == 0) + else if (sTemp.Compare(L"Mar") == 0) nMonth = 3; - if (sTemp.Compare(L"Apr") == 0) + else if (sTemp.Compare(L"Apr") == 0) nMonth = 4; - if (sTemp.Compare(L"May") == 0) + else if (sTemp.Compare(L"May") == 0) nMonth = 5; - if (sTemp.Compare(L"Jun") == 0) + else if (sTemp.Compare(L"Jun") == 0) nMonth = 6; - if (sTemp.Compare(L"Jul") == 0) + else if (sTemp.Compare(L"Jul") == 0) nMonth = 7; - if (sTemp.Compare(L"Aug") == 0) + else if (sTemp.Compare(L"Aug") == 0) nMonth = 8; - if (sTemp.Compare(L"Sep") == 0) + else if (sTemp.Compare(L"Sep") == 0) nMonth = 9; - if (sTemp.Compare(L"Oct") == 0) + else if (sTemp.Compare(L"Oct") == 0) nMonth = 10; - if (sTemp.Compare(L"Nov") == 0) + else if (sTemp.Compare(L"Nov") == 0) nMonth = 11; - if (sTemp.Compare(L"Dec") == 0) + else if (sTemp.Compare(L"Dec") == 0) nMonth = 12; - nDay = (int)ParseStringToNumber(wsArray[2].c_str()); - nHour = (int)ParseStringToNumber(wsArray[3].c_str()); - nMin = (int)ParseStringToNumber(wsArray[4].c_str()); - nSec = (int)ParseStringToNumber(wsArray[5].c_str()); - nYear = (int)ParseStringToNumber(wsArray[7].c_str()); - + int nDay = FX_atof(wsArray[2]); + int nHour = FX_atof(wsArray[3]); + int nMin = FX_atof(wsArray[4]); + int nSec = FX_atof(wsArray[5]); + int nYear = FX_atof(wsArray[7]); double dRet = JS_MakeDate(JS_MakeDay(nYear, nMonth - 1, nDay), JS_MakeTime(nHour, nMin, nSec, 0)); - - if (JS_PortIsNan(dRet)) { + if (JS_PortIsNan(dRet)) dRet = JS_DateParse(strValue.c_str()); - } return dRet; } @@ -1866,14 +1709,17 @@ FX_BOOL CJS_PublicMethods::AFMakeNumber(IJS_Context* cc, const std::vector& params, CJS_Value& vRet, CFX_WideString& sError) { + CJS_Context* pContext = (CJS_Context*)cc; if (params.size() != 1) { - CJS_Context* pContext = (CJS_Context*)cc; - ASSERT(pContext); - sError = JSGetStringFromID(pContext, IDS_STRING_JSPARAMERROR); return FALSE; } - vRet = ParseStringToNumber(params[0].ToCFXWideString().c_str()); + CFX_WideString ws = params[0].ToCFXWideString(); + ws.Replace(L",", L"."); + vRet = ws; + vRet.MaybeCoerceToNumber(); + if (vRet.GetType() != CJS_Value::VT_number) + vRet = 0; return TRUE; } @@ -1913,38 +1759,40 @@ FX_BOOL CJS_PublicMethods::AFSimple_Calculate( for (int j = 0, jsz = pInterForm->CountFields(wsFieldName); j < jsz; j++) { if (CPDF_FormField* pFormField = pInterForm->GetField(j, wsFieldName)) { double dTemp = 0.0; - switch (pFormField->GetFieldType()) { case FIELDTYPE_TEXTFIELD: case FIELDTYPE_COMBOBOX: { - dTemp = ParseStringToNumber(pFormField->GetValue().c_str()); - break; - } + CFX_WideString trimmed = pFormField->GetValue(); + trimmed.TrimRight(); + trimmed.TrimLeft(); + dTemp = FX_atof(trimmed); + } break; case FIELDTYPE_PUSHBUTTON: { dTemp = 0.0; - break; - } + } break; case FIELDTYPE_CHECKBOX: case FIELDTYPE_RADIOBUTTON: { dTemp = 0.0; for (int c = 0, csz = pFormField->CountControls(); c < csz; c++) { if (CPDF_FormControl* pFormCtrl = pFormField->GetControl(c)) { if (pFormCtrl->IsChecked()) { - dTemp += - ParseStringToNumber(pFormCtrl->GetExportValue().c_str()); + CFX_WideString trimmed = pFormCtrl->GetExportValue(); + trimmed.TrimRight(); + trimmed.TrimLeft(); + dTemp = FX_atof(trimmed); break; } } } - break; - } + } break; case FIELDTYPE_LISTBOX: { - if (pFormField->CountSelectedItems() > 1) - break; - - dTemp = ParseStringToNumber(pFormField->GetValue().c_str()); - break; - } + if (pFormField->CountSelectedItems() <= 1) { + CFX_WideString trimmed = pFormField->GetValue(); + trimmed.TrimRight(); + trimmed.TrimLeft(); + dTemp = FX_atof(trimmed); + } + } break; default: break; } diff --git a/fpdfsdk/src/javascript/PublicMethods.h b/fpdfsdk/src/javascript/PublicMethods.h index 013c4ce1a4..26640bc40b 100644 --- a/fpdfsdk/src/javascript/PublicMethods.h +++ b/fpdfsdk/src/javascript/PublicMethods.h @@ -144,18 +144,9 @@ class CJS_PublicMethods : public CJS_Object { bool* bWrongFormat); static CFX_WideString MakeFormatDate(double dDate, const CFX_WideString& format); - static FX_BOOL ConvertStringToNumber(const FX_WCHAR* swSource, - double& dRet, - FX_BOOL& bDot); - static double ParseStringToNumber(const FX_WCHAR* swSource); static double ParseNormalDate(const CFX_WideString& value, bool* bWrongFormat); static double MakeInterDate(CFX_WideString strValue); - static double ParseNumber(const FX_WCHAR* swSource, - FX_BOOL& bAllDigits, - FX_BOOL& bDot, - FX_BOOL& bSign, - FX_BOOL& bKXJS); public: static CFX_WideString StrLTrim(const FX_WCHAR* pStr); diff --git a/testing/resources/javascript/bug_361_expected.txt b/testing/resources/javascript/bug_361_expected.txt index f7f7a562bb..04d72bce20 100644 --- a/testing/resources/javascript/bug_361_expected.txt +++ b/testing/resources/javascript/bug_361_expected.txt @@ -12,31 +12,31 @@ Alert: Answer for " 4 Alert: Answer for "4 3 2 1" is: string 4 3 2 1 Alert: Answer for "-4" is: number -4 Alert: Answer for "23.00000001" is: number 23.00000001 -Alert: Answer for "23.00000000000000001" is: number 0 +Alert: Answer for "23.00000000000000001" is: number 23 Alert: Answer for "4e+25" is: number 4e+25 -Alert: Answer for "40000000000000000000000000" is: number 0 -Alert: Answer for "25,5" is: number 25.5 +Alert: Answer for "40000000000000000000000000" is: number 4e+25 +Alert: Answer for "25,5" is: string 25,5 Alert: Answer for "1e+5" is: number 100000 -Alert: Answer for "1e5" is: number 1 +Alert: Answer for "1e5" is: number 100000 Alert: Answer for "1e-5" is: number 0.00001 Alert: Answer for "-1e-5" is: number -0.00001 -Alert: Answer for "1.2e5" is: number 1.2 -Alert: Answer for "Infinity" is: string Infinity -Alert: Answer for "Infinity" is: string Infinity +Alert: Answer for "1.2e5" is: number 120000 +Alert: Answer for "Infinity" is: number Infinity +Alert: Answer for "Infinity" is: number Infinity Alert: Answer for "INFINITY" is: string INFINITY Alert: Answer for "INF" is: string INF -Alert: Answer for "NaN" is: string NaN -Alert: Answer for "NaN" is: string NaN +Alert: Answer for "NaN" is: number NaN +Alert: Answer for "NaN" is: number NaN Alert: Answer for "NAN" is: string NAN -Alert: Answer for "0x100" is: string 0x100 +Alert: Answer for "0x100" is: number 256 Alert: Answer for "0x100.1" is: string 0x100.1 Alert: Answer for "0x100,1" is: string 0x100,1 Alert: Answer for "0x100x1" is: string 0x100x1 Alert: Answer for "123x6" is: string 123x6 Alert: Answer for "123xy6" is: string 123xy6 Alert: Answer for "123.y6" is: string 123.y6 -Alert: Answer for "1,000,000" is: number 1 -Alert: Answer for "1.2.3" is: number 1.2 +Alert: Answer for "1,000,000" is: string 1,000,000 +Alert: Answer for "1.2.3" is: string 1.2.3 Alert: Answer for "1-3" is: string 1-3 Alert: Answer for "1+3" is: string 1+3 Alert: Answer for "1.-3" is: string 1.-3 -- cgit v1.2.3