-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add limits when deserializing BigDecimal
and BigInteger
#2510
Merged
eamonnmcmanus
merged 5 commits into
google:main
from
Marcono1234:marcono1234/number-limits
Oct 23, 2023
Merged
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
c98efd8
Add limits when deserializing `BigDecimal` and `BigInteger`
Marcono1234 a1cedb5
Merge remote-tracking branch 'remotes/origin/main' into marcono1234/n…
Marcono1234 cf297ec
Use assertThrows
Marcono1234 d1004f9
Don't check number limits in JsonReader
Marcono1234 5822681
Merge branch 'main' into marcono1234/number-limits
Marcono1234 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
37 changes: 37 additions & 0 deletions
37
gson/src/main/java/com/google/gson/internal/NumberLimits.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
package com.google.gson.internal; | ||
|
||
import java.math.BigDecimal; | ||
import java.math.BigInteger; | ||
|
||
/** | ||
* This class enforces limits on numbers parsed from JSON to avoid potential performance | ||
* problems when extremely large numbers are used. | ||
*/ | ||
public class NumberLimits { | ||
private NumberLimits() { | ||
} | ||
|
||
public static final int MAX_NUMBER_STRING_LENGTH = 10_000; | ||
|
||
private static void checkNumberStringLength(String s) { | ||
if (s.length() > MAX_NUMBER_STRING_LENGTH) { | ||
throw new NumberFormatException("Number string too large: " + s.substring(0, 30) + "..."); | ||
} | ||
} | ||
|
||
public static BigDecimal parseBigDecimal(String s) throws NumberFormatException { | ||
checkNumberStringLength(s); | ||
BigDecimal decimal = new BigDecimal(s); | ||
|
||
// Cast to long to avoid issues with abs when value is Integer.MIN_VALUE | ||
if (Math.abs((long) decimal.scale()) >= 10_000) { | ||
throw new NumberFormatException("Number has unsupported scale: " + s); | ||
} | ||
return decimal; | ||
} | ||
|
||
public static BigInteger parseBigInteger(String s) throws NumberFormatException { | ||
checkNumberStringLength(s); | ||
return new BigInteger(s); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
169 changes: 169 additions & 0 deletions
169
gson/src/test/java/com/google/gson/functional/NumberLimitsTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,169 @@ | ||
package com.google.gson.functional; | ||
|
||
import static com.google.common.truth.Truth.assertThat; | ||
import static org.junit.Assert.assertThrows; | ||
|
||
import com.google.gson.Gson; | ||
import com.google.gson.JsonParseException; | ||
import com.google.gson.JsonPrimitive; | ||
import com.google.gson.JsonSyntaxException; | ||
import com.google.gson.ToNumberPolicy; | ||
import com.google.gson.ToNumberStrategy; | ||
import com.google.gson.TypeAdapter; | ||
import com.google.gson.internal.LazilyParsedNumber; | ||
import com.google.gson.stream.JsonReader; | ||
import com.google.gson.stream.JsonToken; | ||
import com.google.gson.stream.MalformedJsonException; | ||
import java.io.IOException; | ||
import java.io.ObjectOutputStream; | ||
import java.io.OutputStream; | ||
import java.io.StringReader; | ||
import java.math.BigDecimal; | ||
import java.math.BigInteger; | ||
import org.junit.Test; | ||
|
||
public class NumberLimitsTest { | ||
private static final int MAX_LENGTH = 10_000; | ||
|
||
private static JsonReader jsonReader(String json) { | ||
return new JsonReader(new StringReader(json)); | ||
} | ||
|
||
@Test | ||
public void testJsonReader() throws IOException { | ||
JsonReader reader = jsonReader("1".repeat(1000)); | ||
assertThat(reader.peek()).isEqualTo(JsonToken.NUMBER); | ||
assertThat(reader.nextString()).isEqualTo("1".repeat(1000)); | ||
|
||
JsonReader reader2 = jsonReader("1".repeat(MAX_LENGTH + 1)); | ||
// Currently JsonReader does not recognize large JSON numbers as numbers but treats them | ||
// as unquoted string | ||
MalformedJsonException e = assertThrows(MalformedJsonException.class, () -> reader2.peek()); | ||
assertThat(e).hasMessageThat().startsWith("Use JsonReader.setStrictness(Strictness.LENIENT) to accept malformed JSON"); | ||
|
||
reader = jsonReader("1e9999"); | ||
assertThat(reader.peek()).isEqualTo(JsonToken.NUMBER); | ||
assertThat(reader.nextString()).isEqualTo("1e9999"); | ||
|
||
reader = jsonReader("1e+9999"); | ||
assertThat(reader.peek()).isEqualTo(JsonToken.NUMBER); | ||
assertThat(reader.nextString()).isEqualTo("1e+9999"); | ||
|
||
JsonReader reader3 = jsonReader("1e10000"); | ||
e = assertThrows(MalformedJsonException.class, () -> reader3.peek()); | ||
assertThat(e).hasMessageThat().isEqualTo("Too many number exponent digits at line 1 column 1 path $"); | ||
|
||
JsonReader reader4 = jsonReader("1e00001"); | ||
// Currently JsonReader does not ignore leading 0s in exponent | ||
e = assertThrows(MalformedJsonException.class, () -> reader4.peek()); | ||
assertThat(e).hasMessageThat().isEqualTo("Too many number exponent digits at line 1 column 1 path $"); | ||
} | ||
|
||
@Test | ||
public void testJsonPrimitive() { | ||
assertThat(new JsonPrimitive("1".repeat(MAX_LENGTH)).getAsBigDecimal()) | ||
.isEqualTo(new BigDecimal("1".repeat(MAX_LENGTH))); | ||
assertThat(new JsonPrimitive("1e9999").getAsBigDecimal()) | ||
.isEqualTo(new BigDecimal("1e9999")); | ||
assertThat(new JsonPrimitive("1e-9999").getAsBigDecimal()) | ||
.isEqualTo(new BigDecimal("1e-9999")); | ||
|
||
NumberFormatException e = assertThrows(NumberFormatException.class, | ||
() -> new JsonPrimitive("1".repeat(MAX_LENGTH + 1)).getAsBigDecimal()); | ||
assertThat(e).hasMessageThat().isEqualTo("Number string too large: 111111111111111111111111111111..."); | ||
|
||
e = assertThrows(NumberFormatException.class, | ||
() -> new JsonPrimitive("1e10000").getAsBigDecimal()); | ||
assertThat(e).hasMessageThat().isEqualTo("Number has unsupported scale: 1e10000"); | ||
|
||
e = assertThrows(NumberFormatException.class, | ||
() -> new JsonPrimitive("1e-10000").getAsBigDecimal()); | ||
assertThat(e).hasMessageThat().isEqualTo("Number has unsupported scale: 1e-10000"); | ||
|
||
|
||
assertThat(new JsonPrimitive("1".repeat(MAX_LENGTH)).getAsBigInteger()) | ||
.isEqualTo(new BigInteger("1".repeat(MAX_LENGTH))); | ||
|
||
e = assertThrows(NumberFormatException.class, | ||
() -> new JsonPrimitive("1".repeat(MAX_LENGTH + 1)).getAsBigInteger()); | ||
assertThat(e).hasMessageThat().isEqualTo("Number string too large: 111111111111111111111111111111..."); | ||
} | ||
|
||
@Test | ||
public void testToNumberPolicy() throws IOException { | ||
ToNumberStrategy strategy = ToNumberPolicy.BIG_DECIMAL; | ||
|
||
assertThat(strategy.readNumber(jsonReader("\"" + "1".repeat(MAX_LENGTH) + "\""))) | ||
.isEqualTo(new BigDecimal("1".repeat(MAX_LENGTH))); | ||
assertThat(strategy.readNumber(jsonReader("1e9999"))) | ||
.isEqualTo(new BigDecimal("1e9999")); | ||
|
||
|
||
JsonParseException e = assertThrows(JsonParseException.class, | ||
() -> strategy.readNumber(jsonReader("\"" + "1".repeat(MAX_LENGTH + 1) + "\""))); | ||
assertThat(e).hasMessageThat().isEqualTo("Cannot parse " + "1".repeat(MAX_LENGTH + 1) + "; at path $"); | ||
assertThat(e).hasCauseThat().hasMessageThat().isEqualTo("Number string too large: 111111111111111111111111111111..."); | ||
|
||
e = assertThrows(JsonParseException.class, () -> strategy.readNumber(jsonReader("\"1e10000\""))); | ||
assertThat(e).hasMessageThat().isEqualTo("Cannot parse 1e10000; at path $"); | ||
assertThat(e).hasCauseThat().hasMessageThat().isEqualTo("Number has unsupported scale: 1e10000"); | ||
} | ||
|
||
@Test | ||
public void testLazilyParsedNumber() throws IOException { | ||
assertThat(new LazilyParsedNumber("1".repeat(MAX_LENGTH)).intValue()) | ||
.isEqualTo(new BigDecimal("1".repeat(MAX_LENGTH)).intValue()); | ||
assertThat(new LazilyParsedNumber("1e9999").intValue()) | ||
.isEqualTo(new BigDecimal("1e9999").intValue()); | ||
|
||
NumberFormatException e = assertThrows(NumberFormatException.class, | ||
() -> new LazilyParsedNumber("1".repeat(MAX_LENGTH + 1)).intValue()); | ||
assertThat(e).hasMessageThat().isEqualTo("Number string too large: 111111111111111111111111111111..."); | ||
|
||
e = assertThrows(NumberFormatException.class, | ||
() -> new LazilyParsedNumber("1e10000").intValue()); | ||
assertThat(e).hasMessageThat().isEqualTo("Number has unsupported scale: 1e10000"); | ||
|
||
e = assertThrows(NumberFormatException.class, | ||
() -> new LazilyParsedNumber("1e10000").longValue()); | ||
assertThat(e).hasMessageThat().isEqualTo("Number has unsupported scale: 1e10000"); | ||
|
||
ObjectOutputStream objOut = new ObjectOutputStream(OutputStream.nullOutputStream()); | ||
// Number is serialized as BigDecimal; should also enforce limits during this conversion | ||
e = assertThrows(NumberFormatException.class, () -> objOut.writeObject(new LazilyParsedNumber("1e10000"))); | ||
assertThat(e).hasMessageThat().isEqualTo("Number has unsupported scale: 1e10000"); | ||
} | ||
|
||
@Test | ||
public void testBigDecimalAdapter() throws IOException { | ||
TypeAdapter<BigDecimal> adapter = new Gson().getAdapter(BigDecimal.class); | ||
|
||
assertThat(adapter.fromJson("\"" + "1".repeat(MAX_LENGTH) + "\"")) | ||
.isEqualTo(new BigDecimal("1".repeat(MAX_LENGTH))); | ||
assertThat(adapter.fromJson("\"1e9999\"")) | ||
.isEqualTo(new BigDecimal("1e9999")); | ||
|
||
JsonSyntaxException e = assertThrows(JsonSyntaxException.class, | ||
() -> adapter.fromJson("\"" + "1".repeat(MAX_LENGTH + 1) + "\"")); | ||
assertThat(e).hasMessageThat().isEqualTo("Failed parsing '" + "1".repeat(MAX_LENGTH + 1) + "' as BigDecimal; at path $"); | ||
assertThat(e).hasCauseThat().hasMessageThat().isEqualTo("Number string too large: 111111111111111111111111111111..."); | ||
|
||
e = assertThrows(JsonSyntaxException.class, | ||
() -> adapter.fromJson("\"1e10000\"")); | ||
assertThat(e).hasMessageThat().isEqualTo("Failed parsing '1e10000' as BigDecimal; at path $"); | ||
assertThat(e).hasCauseThat().hasMessageThat().isEqualTo("Number has unsupported scale: 1e10000"); | ||
} | ||
|
||
@Test | ||
public void testBigIntegerAdapter() throws IOException { | ||
TypeAdapter<BigInteger> adapter = new Gson().getAdapter(BigInteger.class); | ||
|
||
assertThat(adapter.fromJson("\"" + "1".repeat(MAX_LENGTH) + "\"")) | ||
.isEqualTo(new BigInteger("1".repeat(MAX_LENGTH))); | ||
|
||
JsonSyntaxException e = assertThrows(JsonSyntaxException.class, | ||
() -> adapter.fromJson("\"" + "1".repeat(MAX_LENGTH + 1) + "\"")); | ||
assertThat(e).hasMessageThat().isEqualTo("Failed parsing '" + "1".repeat(MAX_LENGTH + 1) + "' as BigInteger; at path $"); | ||
assertThat(e).hasCauseThat().hasMessageThat().isEqualTo("Number string too large: 111111111111111111111111111111..."); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you think of instead recording the index of the first exponent digit? Then you could still easily reject a number if it has too many exponent digits. Or you could parse the digits and compare against a limit, which would fix the leading-zero problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that would be possible, but it might then rely on the buffer not being refilled (and indices not changing) during parsing, which works because this is currently the case. Though from that perspective tracking the count seems a bit more reliable to me.
Is your concern performance (I assume not)? Or whether tracking the index might lead to cleaner or easier to understand code?
I mainly omitted the leading 0s check for simplicity (but nonetheless added a comment and test to document the behavior). Fixing it might also be possible by slightly adjusting this method to explicitly check for
0
. For example:I just wasn't sure if that many leading 0s is really a common use case and worth supporting.
Should I solve this though with the diff shown above (and some comments)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parsing logic is fairly complicated, but it seems to me that the way the
i
variable works is quite simple, starting from 0 and increasing. Even if the buffer is refilled andpos
changes,i
is still a valid offset. So I think saving its value and using it at the end is reasonably robust. And I feel the resulting code would be slightly simpler. It also seems very slightly better to check for a specific maximum exponent rather than a number of exponent digits.I still have the question of whether we need to change
JsonReader
at all, if we're also checking for large exponents inTypeAdapters
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will give it a try.
Changing
JsonReader
is mainly for the cases where users directly obtain the number from theJsonReader
usingnextString()
and then parse the number themselves. Do you think this should not be done? In that case is it ok if I change the documentation ofnextString()
to tell the users to validate the number string, if necessary?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obsolete, I have reverted the changes to
JsonReader
, see #2510 (comment)