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)

defect
Not set
critical

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)

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.
Mentor: jgriffin
Whiteboard: [good first bug][lang=py]
@jgriffin I would like to work on this bug.
(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.
@canaar are you working on this bug? 
If not, can I take a crack at it?
Attached patch naj.patchSplinter Review
Hi I would like to tackle this bug can you assign it to me
(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
Attached patch bug-1207273.patch (obsolete) — Splinter Review
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 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-
Attached patch bug-1207273(2).patch (obsolete) — Splinter Review
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 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-
Attached patch bug-1207273.patch (obsolete) — Splinter Review
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 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)
(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
Flags: needinfo?(jgriffin)
Responded on irc, but ftr I just said I think we can dump totalsent and len(data) here.
Flags: needinfo?(jgriffin)
Attached patch bug-1207273.patch (obsolete) — Splinter Review
Made the change requested
Attachment #8704049 - Attachment is obsolete: true
Attachment #8704271 - Flags: review?(jgriffin)
Attachment #8704271 - Flags: feedback?(jgriffin)
Attachment #8704271 - Flags: review?(jgriffin)
Attachment #8704271 - Flags: review+
Attachment #8704271 - Flags: feedback?(jgriffin)
flagging myself to keep track of the try run
Flags: needinfo?(jgriffin)
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)
(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)
(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)
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)
sanity try run, if this passes I'll do a more comprehensive one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b456d223b1e
(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
(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 .
Flags: needinfo?(jgriffin)
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+
Flags: needinfo?(jgriffin)
https://hg.mozilla.org/mozilla-central/rev/84152b86b7b0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Blocks: 1238996
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.