Closed Bug 1246301 Opened 4 years ago Closed 4 years ago
Incorrectly parsed value for update interval in shavar response
MozReview Request: Bug 1246301 - Incorrectly parsed value for update interval in shavar response. r=francois
58 bytes, text/x-review-board-request
With the (malformed) shavar response: n:9!=Ao[JDw+aN'|e}[ ]2 Results: Firefox indicates an update success, with an update interval of 9 seconds (later corrected to 1800 seconds, since this is too short). The n field should be an integer, but the string is parsed and interpreted as an integer only until it reaches non-numeric characters Expected: Reject invalid parameter for n value. This is not a big issue by itself, just odd behavior.
We should treat this as a failed update.
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Hi François, You mentioned we should treat incorrect update interval as an update failure, but currently in ProtocolParser.cpp when n cannot be parsed successfully, we just set |mUpdateWait| to 0 . I guess the behavior for incorrect value of n(too small or too large) and parse failure should sync, do you think we should return error or just set |mUpdateWait| to 0?  https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/ProtocolParser.cpp?from=ProtocolParser.cpp#133
Comment on attachment 8732695 [details] [diff] [review] check update interval value Review of attachment 8732695 [details] [diff] [review]: ----------------------------------------------------------------- If it works, my preference here would be to not touch the mUpdateWait unless the PR_sscanf() worked and then return an error code immediately (i.e. something else than NS_OK), aborting the parsing. In terms of enforcing a minimum/maximum size, isn't this code sufficient? https://hg.mozilla.org/mozilla-central/rev/4d76f652669d
It turns out that the PR_sscanf() doesn't fail when it gets "9!=Ao[JDw+aN'|e}[ ]2", it simply returns "9" which we fix up later. I think that's OK. The only case that would be worth fixing IMHO is "n:foobar" which will cause PR_sscanf() to return an error. In that case, we should return an error immediately and abort the parsing.
Review commit: https://reviewboard.mozilla.org/r/42387/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42387/
Attachment #8734599 - Flags: review?(francois)
Attachment #8732695 - Attachment is obsolete: true
Comment on attachment 8734599 [details] MozReview Request: Bug 1246301 - Incorrectly parsed value for update interval in shavar response. r=francois https://reviewboard.mozilla.org/r/42387/#review39223
Attachment #8734599 - Flags: review?(francois) → review+
You need to log in before you can comment on or make changes to this bug.