Closed
Bug 1246301
Opened 8 years ago
Closed 8 years ago
Incorrectly parsed value for update interval in shavar response
Categories
(Toolkit :: Safe Browsing, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: mwobensmith, Assigned: dimi)
References
Details
(Whiteboard: tpe-seceng)
Attachments
(1 file, 1 obsolete file)
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.
Updated•8 years ago
|
Assignee: francois → nobody
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
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 [1]. 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? [1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/ProtocolParser.cpp?from=ProtocolParser.cpp#133
Attachment #8732695 -
Flags: feedback?(francois)
Comment 3•8 years ago
|
||
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
Attachment #8732695 -
Flags: feedback?(francois)
Comment 4•8 years ago
|
||
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.
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8732695 -
Attachment is obsolete: true
Comment 6•8 years ago
|
||
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+
Assignee | ||
Comment 7•8 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b8027bd45e9&selectedJob=18743915
Keywords: checkin-needed
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f3160a1e4fb6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•