Skip to content

Commit 4517882

Browse files
grpc/service_config: Handle deserialization edge cases (#2899)
## Motivation There are a couple of cases where the expected behavior doesn't match the actual behavior: ### GrpcDuration (The "timeout" field) | Input | Expected per Protobuf JSON spec | Actual (current) result | | :--- | :--- | :--- | | `"+1s"` | `Err` (leading `+` is not valid) | `Ok(1s)` | | `"1.+5s"` | `Err` (`+` is not valid in fractional seconds) | `Ok(1.05s)` | | `"315576000001s"` | `Err` (exceeds Protobuf max of `315_576_000_000s`) | `Ok(315576000001s)` | ### SerdeU32 (e.g. "maxRequestMessageBytes" field) | Input | Expected per Protobuf JSON spec | Actual (current) result | | :--- | :--- | :--- | | `"+5"` | `Err` (leading `+` is not valid in a `uint32` string) | `Ok(5)` | | `1024.0` | `Ok(1024)` (integer-valued floats for int fields allowed) | `Err("invalid type: floating point \`1024.0\`...")` | | `1e3` | `Ok(1000)` (exp. notation for int fields allowed) | `Err("invalid type: floating point \`1000.0\`...")` | <!-- Explain the context and why you're making that change. What is the problem you're trying to solve? If a new feature is being added, describe the intended use case that feature fulfills. --> ## Solution Small fixes to the parsing, update the test cases to cover this.
1 parent f91ee92 commit 4517882

2 files changed

Lines changed: 44 additions & 2 deletions

File tree

‎grpc/src/client/service_config/duration.rs‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,11 +65,13 @@ impl<'de> Deserialize<'de> for GrpcDuration {
6565
}
6666
}
6767

68+
const MAX_DURATION_SECONDS: u64 = 315_576_000_000;
69+
6870
// Parsing logic, isolated for testing and to reduce serde monomorphization
6971
// footprint.
7072
fn parse_duration(s: &str) -> Result<Duration, String> {
71-
if !s.ends_with('s') {
72-
return Err("duration string must end with 's'".to_string());
73+
if !s.ends_with('s') || s.contains('+') {
74+
return Err("invalid duration string".to_string());
7375
}
7476
let s = &s[..s.len() - 1]; // strip 's'.
7577
let mut parts = s.splitn(2, '.');
@@ -79,6 +81,9 @@ fn parse_duration(s: &str) -> Result<Duration, String> {
7981
let secs: u64 = secs_str
8082
.parse()
8183
.map_err(|e| format!("failed to parse seconds: {e}"))?;
84+
if secs > MAX_DURATION_SECONDS {
85+
return Err(format!("duration exceeds max of {MAX_DURATION_SECONDS}s"));
86+
}
8287

8388
let nanos = if let Some(fraction_str) = parts.next() {
8489
if fraction_str.is_empty() {
@@ -109,6 +114,10 @@ mod test {
109114
fn test_parse_duration() {
110115
assert_eq!(parse_duration("1s").unwrap(), Duration::from_secs(1));
111116
assert_eq!(parse_duration("0s").unwrap(), Duration::from_secs(0));
117+
assert_eq!(
118+
parse_duration("315576000000s").unwrap(),
119+
Duration::from_secs(315_576_000_000)
120+
);
112121
assert_eq!(
113122
parse_duration("1.5s").unwrap(),
114123
Duration::new(1, 500_000_000)
@@ -126,6 +135,9 @@ mod test {
126135
assert!(parse_duration("1.0000000001s").is_err());
127136
assert!(parse_duration("as").is_err());
128137
assert!(parse_duration(".5s").is_err());
138+
assert!(parse_duration("+1s").is_err());
139+
assert!(parse_duration("1.+5s").is_err());
140+
assert!(parse_duration("315576000001s").is_err());
129141
}
130142

131143
#[test]

‎grpc/src/client/service_config/serde_bindings.rs‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,10 +205,25 @@ impl<'de> Deserialize<'de> for SerdeU32 {
205205
v.try_into().map(SerdeU32).map_err(serde::de::Error::custom)
206206
}
207207

208+
#[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)]
209+
fn visit_f64<E>(self, v: f64) -> Result<Self::Value, E>
210+
where
211+
E: serde::de::Error,
212+
{
213+
if v.is_sign_positive() && v.fract() == 0.0 && v <= f64::from(u32::MAX) {
214+
Ok(SerdeU32(v as u32))
215+
} else {
216+
Err(E::custom(format!("invalid u32 value: {v}")))
217+
}
218+
}
219+
208220
fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
209221
where
210222
E: serde::de::Error,
211223
{
224+
if v.starts_with('+') {
225+
return Err(E::custom("leading '+' is not allowed"));
226+
}
212227
v.parse().map(SerdeU32).map_err(serde::de::Error::custom)
213228
}
214229
}
@@ -315,11 +330,26 @@ mod test {
315330
let val: TestStruct = serde_json::from_value(json!({})).unwrap();
316331
assert_eq!(val.val, None);
317332

333+
let val: TestStruct = serde_json::from_value(json!({ "val": 1024.0 })).unwrap();
334+
assert_eq!(val.val, Some(SerdeU32(1024)));
335+
336+
let val: TestStruct = serde_json::from_str(r#"{"val": 1e3}"#).unwrap();
337+
assert_eq!(val.val, Some(SerdeU32(1000)));
338+
318339
let res: Result<TestStruct, _> = serde_json::from_value(json!({ "val": "invalid" }));
319340
assert!(res.is_err());
320341

342+
let res: Result<TestStruct, _> = serde_json::from_value(json!({ "val": "+5" }));
343+
assert!(res.is_err());
344+
345+
let res: Result<TestStruct, _> = serde_json::from_value(json!({ "val": 1.5 }));
346+
assert!(res.is_err());
347+
321348
let res: Result<TestStruct, _> = serde_json::from_value(json!({ "val": -1 }));
322349
assert!(res.is_err());
350+
351+
let res: Result<TestStruct, _> = serde_json::from_str(r#"{"val": -0}"#);
352+
assert!(res.is_err());
323353
}
324354

325355
#[test]

0 commit comments

Comments
 (0)