Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update MaxMessageLength behavior #32

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update MaxMessageLength behavior #32

wants to merge 4 commits into from

Conversation

ltpquang
Copy link
Contributor

This PR intends to update goadb's behaviors regarding to message length. So that:

  • Accept all message lengths returned from adb server
  • Update MaxMessageLength and length-checking logics to reflect current adb's implementation

This fixed #31, and (hopefully) #26 #29.

Please take a look @zach-klippenstein. Thanks!

@ltpquang
Copy link
Contributor Author

ltpquang commented Dec 6, 2020

@zach-klippenstein hi there any progress on this?

zach-klippenstein
zach-klippenstein reviewed Dec 8, 2020
View reviewed changes
wire/conn.go Outdated
@@ -5,7 +5,7 @@ import "github.com/zach-klippenstein/goadb/internal/errors"
const (
// The official implementation of adb imposes an undocumented 255-byte limit
// on messages.
MaxMessageLength = 255
MaxMessageLength = 1024 * 1024
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the above comment, there should also be an explanation where this number comes from.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I've just updated the comment. Also, I renamed MaxMessageLength to MaxPayloadSize for better reflecting adb's implementation.

wire/sender.go Outdated
@@ -29,7 +29,7 @@ func SendMessageString(s Sender, msg string) error {
}

func (s *realSender) SendMessage(msg []byte) error {
if len(msg) > MaxMessageLength {
if len(msg) > MaxMessageLength - 4 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why -4?

Copy link
Contributor Author

@ltpquang ltpquang Dec 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A message's payload is combined from:

  1. 4-byte hex string denoting message's length
  2. The message itself

The ADB's implementation implies that the MaxPayloadSize is the size of the message AND the length-string:

MaxPayloadSize = MessageLenghtStringSize + MaxMessageLength

Therefore we need to minus 4 bytes (length-string) to get the message's size.

@ltpquang
Copy link
Contributor Author

Hi @zach-klippenstein any update on this PR?

@evrins
Copy link

evrins commented Feb 21, 2021

please merge this pr.
the limit of 255 bytes on response body size may make ListDevices function panic on parseKeyVal.
because response may trunk making parse fail.
eg:
full response:

24bc6ecc               device product:T782H_EEA model:T782H device:Boston transport_id:8
62bf3267               device product:T782H_EEA model:T782H device:Boston transport_id:7
99384216               device product:T782H_EEA model:T782H device:Boston transport_id:5
abef8bde               device product:T782H_EEA model:T782H device:Boston transport_id:6
f7241975               device product:T782H_EEA model:T782H device:Boston transport_id:4

trunked response:

24bc6ecc               device product:T782H_EEA model:T782H device:Boston transport_id:8
62bf3267               device product:T782H_EEA model:T782H device:Boston transport_id:7
99384216               device product:T782H_EEA model:T782H device:Boston tra

parseKeyVal panic on tra

@webninjadk
Copy link

still is not merged?

@aac3476
Copy link

aac3476 commented May 22, 2024

why not merged?
if we have too many device it will panic

@asjdf
Copy link

asjdf commented Aug 12, 2024

Looking forward to this PR being merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zach-klippenstein zach-klippenstein zach-klippenstein left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mismatch number of devices returned from goadb
6 participants
@ltpquang @evrins @webninjadk @aac3476 @asjdf @zach-klippenstein