MarionetteTransport.send() does not check if all package data has been submitted

RESOLVED FIXED in Firefox 46

Status

--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: whimboo, Assigned: shivin.yadav, Mentored)

Tracking

({dataloss})

unspecified
mozilla46
dataloss
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [good first bug][lang=py])

Attachments

(2 attachments, 4 obsolete attachments)

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]

Comment 1

3 years ago
@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?

Comment 4

3 years ago
Posted patch naj.patchSplinter Review
(Assignee)

Comment 5

3 years ago
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
(Assignee)

Comment 7

3 years ago
Posted patch bug-1207273.patch (obsolete) — Splinter Review
(Assignee)

Updated

3 years ago
Attachment #8703365 - Attachment description: patch to remove the packet division and check data loss → bug-1207273.patch
(Assignee)

Comment 8

3 years ago
I have created a patch can you please review it and tell me any changes needed
(Assignee)

Updated

3 years ago
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-
(Assignee)

Comment 10

3 years ago
Posted 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-
(Assignee)

Comment 12

3 years ago
Posted 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)
(Assignee)

Comment 14

3 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
(Assignee)

Updated

3 years ago
Flags: needinfo?(jgriffin)
Responded on irc, but ftr I just said I think we can dump totalsent and len(data) here.
Flags: needinfo?(jgriffin)
(Assignee)

Comment 16

3 years ago
Posted 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)
(Assignee)

Comment 20

3 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)
(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

3 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)
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
(Assignee)

Comment 25

3 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 .
(Assignee)

Updated

3 years ago
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)

Comment 28

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/84152b86b7b0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Blocks: 1238996
You need to log in before you can comment on or make changes to this bug.