Closed
Bug 1207273
Opened 9 years ago
Closed 8 years ago
MarionetteTransport.send() does not check if all package data has been submitted
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
Tracking
(firefox46 fixed)
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: whimboo, Assigned: shivin.yadav, Mentored)
References
Details
(Keywords: dataloss, Whiteboard: [good first bug][lang=py])
Attachments
(2 files, 4 obsolete files)
1.22 KB,
patch
|
Details | Diff | Splinter Review | |
1.68 KB,
patch
|
jgriffin
:
review+
|
Details | Diff | Splinter Review |
So there is a bug in the transport code, which sends a message over the connected socket: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/transport/marionette_transport/transport.py?offset=200#99 > try: > self.sock.send(packet) > except IOError as e: Here we wrongly assume that always the whole package data has been sent. There will be situations when this is not the case and socket.send() will return a value which is smaller than len(packet). In such a case we will experience a dataloss. It might be better to use something like the following: > while totalsent < len(data): > sent = self.sock.send(data[totalsent:]) > if sent == 0: > raise RuntimeError("socket connection broken") > totalsent = totalsent + sent There would be no need to split the data into pieces first for submission, but the socket code itself determines the maximum package length.
Updated•9 years ago
|
Mentor: jgriffin
Whiteboard: [good first bug][lang=py]
Comment 2•9 years ago
|
||
(In reply to Abhilash Mhaisne from comment #1) > @jgriffin I would like to work on this bug. Please go ahead and add the checks suggested by whimboo. You can submit your patch to MozReview (https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview.html) or as a .patch file attached to this bug.
Comment 3•9 years ago
|
||
@canaar are you working on this bug? If not, can I take a crack at it?
Comment 6•8 years ago
|
||
(In reply to Shivin from comment #5) > Hi I would like to tackle this bug can you assign it to me Done!
Assignee: nobody → shivin.yadav
Status: NEW → ASSIGNED
Attachment #8703365 -
Attachment description: patch to remove the packet division and check data loss → bug-1207273.patch
I have created a patch can you please review it and tell me any changes needed
Attachment #8703365 -
Flags: review?(jgriffin)
Comment 9•8 years ago
|
||
Comment on attachment 8703365 [details] [diff] [review] bug-1207273.patch Review of attachment 8703365 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch, looks pretty good. Just a few nits. ::: testing/marionette/transport/marionette_transport/transport.py @@ +238,5 @@ > payload = "%s:%s" % (len(data), data) > > + totalsent = 0 > + while totalsent < len(data): > + try: nit: trailing whitespace @@ +241,5 @@ > + while totalsent < len(data): > + try: > + sent = self.sock.send(data[totalsent:]) > + if sent == 0: > + raise RuntimeError("socket connection broken") let's use IOError here, instead of RuntimeError @@ +242,5 @@ > + try: > + sent = self.sock.send(data[totalsent:]) > + if sent == 0: > + raise RuntimeError("socket connection broken") > + else: totalsent += sent nit: need newline after else:
Attachment #8703365 -
Flags: review?(jgriffin) → review-
Assignee | ||
Comment 10•8 years ago
|
||
I did the second and third change but the the whitespace in the first change is 12 spaces and I don't know why it is showing it as extra white space as it alligns with the except statement perfectly
Attachment #8703365 -
Attachment is obsolete: true
Attachment #8703493 -
Flags: review?(jgriffin)
Comment 11•8 years ago
|
||
Comment on attachment 8703493 [details] [diff] [review] bug-1207273(2).patch Review of attachment 8703493 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/marionette/transport/marionette_transport/transport.py @@ +238,5 @@ > payload = "%s:%s" % (len(data), data) > > + totalsent = 0 > + while totalsent < len(data): > + try: The extra whitespace here is after the colon, not at the beginning of the line. @@ +242,5 @@ > + try: > + sent = self.sock.send(data[totalsent:]) > + if sent == 0: > + raise IOError("socket connection broken") > + else: totalsent += sent Please put the code after the colon on the next line. E.g., if foo: # do something else: # do something else
Attachment #8703493 -
Flags: review?(jgriffin) → review-
Assignee | ||
Comment 12•8 years ago
|
||
Implemented the requested changes please tell me if any more details need to be changed
Attachment #8703493 -
Attachment is obsolete: true
Attachment #8704049 -
Flags: review?(jgriffin)
Attachment #8704049 -
Flags: feedback?(jgriffin)
Comment 13•8 years ago
|
||
Comment on attachment 8704049 [details] [diff] [review] bug-1207273.patch Review of attachment 8704049 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. Can I ask for one more change? In the IOError we throw, it would help with debugging if it printed something like "socket error after sending %d bytes out of %d"
Attachment #8704049 -
Flags: review?(jgriffin)
Attachment #8704049 -
Flags: review+
Attachment #8704049 -
Flags: feedback?(jgriffin)
Assignee | ||
Comment 14•8 years ago
|
||
(In reply to Jonathan Griffin (:jgriffin) from comment #13) > Comment on attachment 8704049 [details] [diff] [review] > bug-1207273.patch > > Review of attachment 8704049 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks good. Can I ask for one more change? > > In the IOError we throw, it would help with debugging if it printed > something like "socket error after sending %d bytes out of %d" If I do that then I would have to keep a count of the packet size.Which would mean creating individual packets should I do that because that would mean reverting back to the old method of creating individual packets and then checking the size of packets matching the size of packet sent to packet created . Thanks
Comment 15•8 years ago
|
||
Responded on irc, but ftr I just said I think we can dump totalsent and len(data) here.
Flags: needinfo?(jgriffin)
Assignee | ||
Comment 16•8 years ago
|
||
Made the change requested
Attachment #8704049 -
Attachment is obsolete: true
Attachment #8704271 -
Flags: review?(jgriffin)
Attachment #8704271 -
Flags: feedback?(jgriffin)
Updated•8 years ago
|
Attachment #8704271 -
Flags: review?(jgriffin)
Attachment #8704271 -
Flags: review+
Attachment #8704271 -
Flags: feedback?(jgriffin)
Comment 17•8 years ago
|
||
try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2559708dbd3b
Comment 19•8 years ago
|
||
This is failing on try, see https://treeherder.mozilla.org/logviewer.html#?job_id=15086992&repo=try Can you try to reproduce and fix this?
Flags: needinfo?(jgriffin)
Assignee | ||
Comment 20•8 years ago
|
||
(In reply to Jonathan Griffin (:jgriffin) from comment #19) > This is failing on try, see > https://treeherder.mozilla.org/logviewer.html#?job_id=15086992&repo=try > > Can you try to reproduce and fix this? can you give me an idea on how I may be able to reproduce this bug or accessing the test it failed on I am new to mozilla so I lack experience in these things thanks
Flags: needinfo?(jgriffin)
Comment 21•8 years ago
|
||
(In reply to Shivin from comment #20) > can you give me an idea on how I may be able to reproduce this bug or > accessing the test it failed on I am new to mozilla so I lack experience in > these things thanks You should be able to reproduce this with `./mach marionette-test --gecko-log -`.
Flags: needinfo?(jgriffin)
Assignee | ||
Comment 22•8 years ago
|
||
changed len(data) to len(payload) as payload is the actual information being sent sorry for the late response was busy for the last few days.Inform me if any other updates are needed
Attachment #8704271 -
Attachment is obsolete: true
Attachment #8705990 -
Flags: review?(jgriffin)
Comment 23•8 years ago
|
||
sanity try run, if this passes I'll do a more comprehensive one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b456d223b1e
Comment 24•8 years ago
|
||
(In reply to Jonathan Griffin (:jgriffin) from comment #23) > sanity try run, if this passes I'll do a more comprehensive one: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b456d223b1e That looks good, here's a fuller run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc07199aa882
Assignee | ||
Comment 25•8 years ago
|
||
(In reply to Jonathan Griffin (:jgriffin) from comment #24) > (In reply to Jonathan Griffin (:jgriffin) from comment #23) > > sanity try run, if this passes I'll do a more comprehensive one: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b456d223b1e > > That looks good, here's a fuller run: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc07199aa882 That's cool tell me if anything needs to be fixed .
Comment 26•8 years ago
|
||
Comment on attachment 8705990 [details] [diff] [review] bug-1207273.patch Review of attachment 8705990 [details] [diff] [review]: ----------------------------------------------------------------- Looks good and try run passes, thanks!
Attachment #8705990 -
Flags: review?(jgriffin) → review+
Updated•8 years ago
|
Flags: needinfo?(jgriffin)
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/84152b86b7b0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•