Skip to content

Commit 526ecd6

Browse files
nbougalisnbougalis
committed
Detect invalid inputs during STAmount conversion (RIPD-570):
* More robust validation of input * XRP may not be specified using fractions * Prevent creating native amounts larger than max possible value * Add unit tests to verify correct parsing
1 parent d373054 commit 526ecd6

File tree

3 files changed

+113
-114
lines changed

3 files changed

+113
-114
lines changed

‎src/ripple/data/protocol/STAmount.cpp

+39-91
Original file line numberDiff line numberDiff line change
@@ -371,21 +371,27 @@ STAmount operator- (STAmount const& v1, STAmount const& v2)
371371

372372
//------------------------------------------------------------------------------
373373

374-
std::uint64_t STAmount::uRateOne =
375-
getRate (STAmount (1), STAmount (1));
374+
std::uint64_t const STAmount::uRateOne = getRate (STAmount (1), STAmount (1));
376375

377376
// Note: mIsNative and mIssue.currency must be set already!
378377
bool
379-
STAmount::setValue (std::string const& sAmount)
378+
STAmount::setValue (std::string const& amount)
380379
{
381-
static boost::regex reNumber (
382-
"\\`([+-]?)(\\d*)(\\.(\\d*))?([eE]([+-]?)(\\d+))?\\'");
383-
boost::smatch smMatch;
380+
static boost::regex const reNumber (
381+
"^" // the beginning of the string
382+
"([-+]?)" // (optional) + or - character
383+
"(0|[1-9][0-9]*)" // a number (no leading zeroes, unless 0)
384+
"(\\.([0-9]+))?" // (optional) period followed by any number
385+
"([eE]([+-]?)([0-9]+))?" // (optional) E, optional + or -, any number
386+
"$",
387+
boost::regex_constants::optimize);
384388

385-
if (!boost::regex_match (sAmount, smMatch, reNumber))
389+
boost::smatch match;
390+
391+
if (!boost::regex_match (amount, match, reNumber))
386392
{
387-
WriteLog (lsWARNING, STAmount)
388-
<< "Number not valid: \"" << sAmount << "\"";
393+
WriteLog (lsWARNING, STAmount) <<
394+
"Number not valid: \"" << amount << "\"";
389395
return false;
390396
}
391397

@@ -401,113 +407,56 @@ STAmount::setValue (std::string const& sAmount)
401407

402408
try
403409
{
404-
if ((smMatch[2].length () + smMatch[4].length ()) > 32)
410+
// CHECKME: Why 32? Shouldn't this be 16?
411+
if ((match[2].length () + match[4].length ()) > 32)
405412
{
406-
WriteLog (lsWARNING, STAmount) << "Overlong number: " << sAmount;
413+
WriteLog (lsWARNING, STAmount) << "Overlong number: " << amount;
407414
return false;
408415
}
409416

410-
mIsNegative = (smMatch[1].matched && (smMatch[1] == "-"));
417+
mIsNegative = (match[1].matched && (match[1] == "-"));
418+
419+
// Can't specify XRP using fractional representation
420+
if (mIsNative && match[3].matched)
421+
return false;
411422

412-
if (!smMatch[4].matched) // integer only
423+
if (!match[4].matched) // integer only
413424
{
414-
mValue = beast::lexicalCast <std::uint64_t> (std::string (smMatch[2]));
425+
mValue = beast::lexicalCastThrow <std::uint64_t> (std::string (match[2]));
415426
mOffset = 0;
416427
}
417428
else
418429
{
419430
// integer and fraction
420-
mValue = beast::lexicalCast <std::uint64_t> (smMatch[2] + smMatch[4]);
421-
mOffset = - (smMatch[4].length ());
431+
mValue = beast::lexicalCastThrow <std::uint64_t> (match[2] + match[4]);
432+
mOffset = -(match[4].length ());
422433
}
423434

424-
if (smMatch[5].matched)
435+
if (match[5].matched)
425436
{
426437
// we have an exponent
427-
if (smMatch[6].matched && (smMatch[6] == "-"))
428-
mOffset -= beast::lexicalCast <int> (std::string (smMatch[7]));
438+
if (match[6].matched && (match[6] == "-"))
439+
mOffset -= beast::lexicalCastThrow <int> (std::string (match[7]));
429440
else
430-
mOffset += beast::lexicalCast <int> (std::string (smMatch[7]));
441+
mOffset += beast::lexicalCastThrow <int> (std::string (match[7]));
431442
}
432-
}
433-
catch (...)
434-
{
435-
WriteLog (lsWARNING, STAmount) << "Number not parsed: \"" << sAmount << "\"";
436-
return false;
437-
}
438-
439-
WriteLog (lsTRACE, STAmount) << "Float \"" << sAmount << "\" parsed to " << mValue << " : " << mOffset;
440-
441-
if (mIsNative)
442-
{
443-
if (smMatch[3].matched)
444-
mOffset -= SYSTEM_CURRENCY_PRECISION;
445443

446-
while (mOffset > 0)
447-
{
448-
mValue *= 10;
449-
--mOffset;
450-
}
451-
452-
while (mOffset < 0)
453-
{
454-
mValue /= 10;
455-
++mOffset;
456-
}
457-
}
458-
else
459444
canonicalize ();
460445

461-
return true;
462-
}
463-
464-
// Not meant to be the ultimate parser. For use by RPC which is supposed to be sane and trusted.
465-
// Native has special handling:
466-
// - Integer values are in base units.
467-
// - Float values are in float units.
468-
// - To avoid a mistake float value for native are specified with a "^" in place of a "."
469-
// <-- bValid: true = valid
470-
bool STAmount::setFullValue (std::string const& sAmount, std::string const& sCurrency, std::string const& sIssuer)
471-
{
472-
//
473-
// Figure out the currency.
474-
//
475-
if (!to_currency (mIssue.currency, sCurrency))
476-
{
477-
WriteLog (lsINFO, STAmount) << "Currency malformed: " << sCurrency;
478-
479-
return false;
480-
}
481-
482-
mIsNative = !mIssue.currency;
483-
484-
//
485-
// Figure out the issuer.
486-
//
487-
RippleAddress naIssuerID;
488-
489-
// Issuer must be "" or a valid account string.
490-
if (!naIssuerID.setAccountID (sIssuer))
491-
{
492-
WriteLog (lsINFO, STAmount) << "Issuer malformed: " << sIssuer;
493-
494-
return false;
446+
WriteLog (lsTRACE, STAmount) <<
447+
"Canonicalized \"" << amount << "\" to " << mValue << " : " << mOffset;
495448
}
496-
497-
mIssue.account = naIssuerID.getAccountID ();
498-
499-
// Stamps not must have an issuer.
500-
if (mIsNative && !isXRP (*this))
449+
catch (...)
501450
{
502-
WriteLog (lsINFO, STAmount) << "Issuer specified for XRP: " << sIssuer;
503-
504451
return false;
505452
}
506453

507-
return setValue (sAmount);
454+
return true;
508455
}
509456

510-
void STAmount::setIssue (Issue const& issue) {
457+
void
458+
STAmount::setIssue (Issue const& issue)
459+
{
511460
mIssue = std::move(issue);
512461
mIsNative = isXRP (*this);
513462
}
@@ -836,7 +785,7 @@ void STAmount::canonicalize ()
836785
--mOffset;
837786
}
838787

839-
if (mValue > cMaxNative)
788+
if (mValue > cMaxNativeN)
840789
throw std::runtime_error ("Native currency amount out of range");
841790

842791
return;
@@ -870,7 +819,6 @@ void STAmount::canonicalize ()
870819
{
871820
mValue = 0;
872821
mOffset = 0;
873-
mIsNegative = false;
874822
}
875823

876824
if (mOffset > cMaxOffset)

‎src/ripple/data/protocol/STAmount.h

+1-4
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ class STAmount : public SerializedType
6565
static const std::uint64_t cNotNative = 0x8000000000000000ull;
6666
static const std::uint64_t cPosNative = 0x4000000000000000ull;
6767

68-
static std::uint64_t uRateOne;
68+
static std::uint64_t const uRateOne;
6969

7070
//--------------------------------------------------------------------------
7171

@@ -261,9 +261,6 @@ class STAmount : public SerializedType
261261
// Make this private
262262
bool setValue (std::string const& sAmount);
263263

264-
bool setFullValue (std::string const& sAmount,
265-
std::string const& sCurrency = "", std::string const& sIssuer = "");
266-
267264
//--------------------------------------------------------------------------
268265
//
269266
// SerializedType

‎src/ripple/data/protocol/STAmount.test.cpp

+73-19
Original file line numberDiff line numberDiff line change
@@ -111,27 +111,81 @@ class STAmount_test : public beast::unit_test::suite
111111

112112
//--------------------------------------------------------------------------
113113

114+
void testSetValue (
115+
std::string const& value, Issue const& issue, bool success = true)
116+
{
117+
STAmount amount (issue);
118+
119+
bool const result = amount.setValue (value);
120+
121+
expect (result == success, std::string ("parse ") + value);
122+
123+
if (success)
124+
expect (amount.getText () == value, std::string ("format ") + value);
125+
}
126+
114127
void testSetValue ()
115128
{
116-
testcase ("set value");
117-
118-
STAmount saTmp;
119-
120-
#if 0
121-
// Check native floats
122-
saTmp.setFullValue ("1^0");
123-
BOOST_CHECK_MESSAGE (SYSTEM_CURRENCY_PARTS == saTmp.getNValue (), "float integer failed");
124-
saTmp.setFullValue ("0^1");
125-
BOOST_CHECK_MESSAGE (SYSTEM_CURRENCY_PARTS / 10 == saTmp.getNValue (), "float fraction failed");
126-
saTmp.setFullValue ("0^12");
127-
BOOST_CHECK_MESSAGE (12 * SYSTEM_CURRENCY_PARTS / 100 == saTmp.getNValue (), "float fraction failed");
128-
saTmp.setFullValue ("1^2");
129-
BOOST_CHECK_MESSAGE (SYSTEM_CURRENCY_PARTS + (2 * SYSTEM_CURRENCY_PARTS / 10) == saTmp.getNValue (), "float combined failed");
130-
#endif
131-
132-
// Check native integer
133-
saTmp.setFullValue ("1");
134-
expect (1 == saTmp.getNValue (), "should be equal");
129+
{
130+
testcase ("set value (native)");
131+
132+
Issue const xrp (xrpIssue ());
133+
134+
// fractional XRP (i.e. drops)
135+
testSetValue ("1", xrp);
136+
testSetValue ("22", xrp);
137+
testSetValue ("333", xrp);
138+
testSetValue ("4444", xrp);
139+
testSetValue ("55555", xrp);
140+
testSetValue ("666666", xrp);
141+
142+
// 1 XRP up to 100 billion, in powers of 10 (in drops)
143+
testSetValue ("1000000", xrp);
144+
testSetValue ("10000000", xrp);
145+
testSetValue ("100000000", xrp);
146+
testSetValue ("1000000000", xrp);
147+
testSetValue ("10000000000", xrp);
148+
testSetValue ("100000000000", xrp);
149+
testSetValue ("1000000000000", xrp);
150+
testSetValue ("10000000000000", xrp);
151+
testSetValue ("100000000000000", xrp);
152+
testSetValue ("1000000000000000", xrp);
153+
testSetValue ("10000000000000000", xrp);
154+
testSetValue ("100000000000000000", xrp);
155+
156+
// Invalid native values:
157+
testSetValue ("1.1", xrp, false);
158+
testSetValue ("100000000000000001", xrp, false);
159+
testSetValue ("1000000000000000000", xrp, false);
160+
}
161+
162+
{
163+
testcase ("set value (iou)");
164+
165+
Issue const usd (Currency (0x5553440000000000), Account (0x4985601));
166+
167+
testSetValue ("1", usd);
168+
testSetValue ("10", usd);
169+
testSetValue ("100", usd);
170+
testSetValue ("1000", usd);
171+
testSetValue ("10000", usd);
172+
testSetValue ("100000", usd);
173+
testSetValue ("1000000", usd);
174+
testSetValue ("10000000", usd);
175+
testSetValue ("100000000", usd);
176+
testSetValue ("1000000000", usd);
177+
testSetValue ("10000000000", usd);
178+
179+
testSetValue ("1234567.1", usd);
180+
testSetValue ("1234567.12", usd);
181+
testSetValue ("1234567.123", usd);
182+
testSetValue ("1234567.1234", usd);
183+
testSetValue ("1234567.12345", usd);
184+
testSetValue ("1234567.123456", usd);
185+
testSetValue ("1234567.1234567", usd);
186+
testSetValue ("1234567.12345678", usd);
187+
testSetValue ("1234567.123456789", usd);
188+
}
135189
}
136190

137191
//--------------------------------------------------------------------------

0 commit comments

Comments
 (0)