Issue about checking the battery before starting

[miles....@gmail.com]

Hi,

Syzkaller uses adb command "adb shell dumpsys battery | grep level:" to check if the battery meets the minimum requirement.

I usually see the adb is already connected but the battery service is not available, which causes Syzkaller to reboot the device and start again.

Maybe adding a retry is better?

------------------------------------------------------------------------------------

[vm/adb/adb.go]

-       for ; numRetry >= 0 && err != nil; numRetry-- {

+       for ; numRetry >= 0 && ((err != nil) || (strings.Contains(string(out), "Can't find service"))); numRetry-- {

------------------------------------------------------------------------------------

https://github.com/google/syzkaller/blob/master/vm/adb/adb.go#L339

https://android.googlesource.com/platform/frameworks/native/+/master/cmds/dumpsys/dumpsys.cpp#288

Thanks,

Miles

[Dmitry Vyukov]

On Thu, Jun 22, 2017 at 4:45 AM, <miles....@gmail.com> wrote:
> Hi,
>
> Syzkaller uses adb command "adb shell dumpsys battery | grep level:" to
> check if the battery meets the minimum requirement.
>
> I usually see the adb is already connected but the battery service is not
> available, which causes Syzkaller to reboot the device and start again.
>
> Maybe adding a retry is better?

Hi,

There is already a retry for 150 seconds. When does the battery
service come up on your device?

> ------------------------------------------------------------------------------------
> [vm/adb/adb.go]
>
> - for ; numRetry >= 0 && err != nil; numRetry-- {
> + for ; numRetry >= 0 && ((err != nil) ||
> (strings.Contains(string(out), "Can't find service"))); numRetry-- {

We can extend the delay, but no infinite loops please.

> ------------------------------------------------------------------------------------
>
> https://github.com/google/syzkaller/blob/master/vm/adb/adb.go#L339
> https://android.googlesource.com/platform/frameworks/native/+/master/cmds/dumpsys/dumpsys.cpp#288
>
> Thanks,
>
> Miles
>

> --
> You received this message because you are subscribed to the Google Groups
> "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to syzkaller+...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[miles....@gmail.com]

Hi Dmitry,

I know there's already a delay (30 * 5 secs) in https://github.com/google/syzkaller/blob/master/vm/adb/adb.go#L311

But the problem is that the current delay does not include checking the "unavailable battery service (i.e. Can't find service: battery)".

No infinite delay needed.

After applying the below change, I can get the correct battery of my device after 6 times of retry at most.

------------------------------------------------------------------------------------ 
 [vm/adb/adb.go] 
 
 -       for ; numRetry >= 0 && err != nil; numRetry-- { 
 +      for ; numRetry >= 0 && ((err != nil) ||  (strings.Contains(string(out), "Can't find service"))); numRetry-- { 

------------------------------------------------------------------------------------ 

Syzkaller will do the "adb reboot" each time when getting the battery result "unavailable battery service".

So this change helps me a lot.

Thanks,

Miles

[Dmitry Vyukov]

On Thu, Jun 22, 2017 at 8:42 AM, <miles....@gmail.com> wrote:
> Hi Dmitry,
>
> I know there's already a delay (30 * 5 secs) in
> https://github.com/google/syzkaller/blob/master/vm/adb/adb.go#L311
> But the problem is that the current delay does not include checking the
> "unavailable battery service (i.e. Can't find service: battery)".
>
> No infinite delay needed.
>
> After applying the below change, I can get the correct battery of my device
> after 6 times of retry at most.
> ------------------------------------------------------------------------------------
> [vm/adb/adb.go]
>
> - for ; numRetry >= 0 && err != nil; numRetry-- {
> + for ; numRetry >= 0 && ((err != nil) ||
> (strings.Contains(string(out), "Can't find service"))); numRetry-- {
> ------------------------------------------------------------------------------------
>
> Syzkaller will do the "adb reboot" each time when getting the battery result
> "unavailable battery service".
> So this change helps me a lot.

You mean that err == nil when dumpsys returns "Can't find service"? If
yes, then that's the problem that we need to fix.
Does dumpsys itself fail in such case? If yes, then we need to remove
grep from the command and parse it manually.

What you propose is an infinite loop is we don't assume that the
device behaves in a sane way. First, we had a bunch of similar
problems during boot. Second, we proactively try to break it. We
absolutely must not assume that it will behave in a sane way. Consider
that you start fuzzing and go to a vacation. A month later you expect
to see lots of bugs, but instead you discover it was waiting for the
battery service to come up for the whole month.

[miles....@gmail.com]

Hi Dmitry,

You mean that err == nil when dumpsys returns "Can't find service"? If 

yes, then that's the problem that we need to fix.  => Yes

Does dumpsys itself fail in such case? If yes, then we need to remove 

grep from the command and parse it manually  => Yes

Please let me know once the patch is merged!

Thanks,

Miles

[Dmitry Vyukov]

On Thu, Jun 22, 2017 at 10:00 AM, <miles....@gmail.com> wrote:
> Hi Dmitry,
>

> You mean that err == nil when dumpsys returns "Can't find service"? If
> yes, then that's the problem that we need to fix. => Yes
> Does dumpsys itself fail in such case? If yes, then we need to remove
> grep from the command and parse it manually => Yes

But wait, shouldn't grep fail as well if there is no "level:" in output?
Here is what I see on my machine:

linux$ bash -c "echo 111 | grep 111" && echo OK
111
OK
linux$ bash -c "echo 222 | grep 111" && echo OK
linux$

Why is err==nil when dumpsys fails with "Can't find service"?

[miles....@gmail.com]

Hi Dmitry, 

I was testing on Android N1 phone.

This is strange, though.

-------------------------------------------------------------------------------

miles@ubuntu:~/syzkaller/src/github.com/google/syzkaller$ adb shell "dumpsys test123 | grep level:"

Can't find service: test123

miles@ubuntu:~/syzkaller/src/github.com/google/syzkaller$

miles@ubuntu:~/syzkaller/src/github.com/google/syzkaller$ adb shell "dumpsys test123" | grep level:

miles@ubuntu:~/syzkaller/src/github.com/google/syzkaller$

-------------------------------------------------------------------------------

Thanks,

Miles

[Dmitry Vyukov]

On Thu, Jun 22, 2017 at 11:13 AM, <miles....@gmail.com> wrote:
> Hi Dmitry,
>

> I was testing on Android N1 phone.
> This is strange, though.
>
> -------------------------------------------------------------------------------
> miles@ubuntu:~/syzkaller/src/github.com/google/syzkaller$ adb shell "dumpsys
> test123 | grep level:"
> Can't find service: test123
> miles@ubuntu:~/syzkaller/src/github.com/google/syzkaller$
>
> miles@ubuntu:~/syzkaller/src/github.com/google/syzkaller$ adb shell "dumpsys
> test123" | grep level:
> miles@ubuntu:~/syzkaller/src/github.com/google/syzkaller$
> -------------------------------------------------------------------------------

I think that's because adb shell merges stderr into stdout.

I still don't understand what exactly happens on your device and what
are exit statuses from adb, dumpsys and grep.

[miles....@gmail.com]

Hi Dmitry,

I've tried multiple Android phones and I can confirm it is Android's problem.

Like you said adb shell may merge stderr to stdout.

You can find any Android phone and try this command: adb shell "dumpsys test123 | grep level:".

That will prove that err == nil but the battery information still cannot be retrieved. 

Thanks,

Miles

[Dmitry Vyukov]

On Thu, Jun 22, 2017 at 12:51 PM, <miles....@gmail.com> wrote:
> Hi Dmitry,
>

> I've tried multiple Android phones and I can confirm it is Android's
> problem.
> Like you said adb shell may merge stderr to stdout.
>
> You can find any Android phone and try this command: adb shell "dumpsys
> test123 | grep level:".
> That will prove that err == nil but the battery information still cannot be
> retrieved.

Filed an issue for this:
https://github.com/google/syzkaller/issues/246

[$rik@nth]

Hi Dimitry,

In the same note. I am trying syzkaller on open embedded kernel on ARM
development board which will have adb support (But Android won't be
available). Device boots up to shell only and Ui won't be available.
In such case it will fail because it won't get battery service, Is it
possible to handle this use case as well? So that i can use syzkaller
on OE kernel as well.

Let me know if you need more information.

> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.



--

Thanks & Regards,
M.Srikanth Kumar.

[Dmitry Vyukov]

On Fri, Jun 23, 2017 at 12:28 PM, $rik@nth <srikan...@gmail.com> wrote:
> Hi Dimitry,
>
> In the same note. I am trying syzkaller on open embedded kernel on ARM
> development board which will have adb support (But Android won't be
> available). Device boots up to shell only and Ui won't be available.
> In such case it will fail because it won't get battery service, Is it
> possible to handle this use case as well? So that i can use syzkaller
> on OE kernel as well.

Does it have ssh? I think you need something like vm/odroid:
https://github.com/google/syzkaller/blob/master/vm/odroid/odroid.go
which we use for arm dev boards.

adb proved to be very unreliable with interface poorly suitable for
any automation. I would suggest to avoid using it if possible.

[$rik@nth]

adb protocol is ported to OE kernel and able to connect ARM board with adb.

SSH won't be available.

[$rik@nth]

AFAIK if we can skip the battery check, it should work as it is. This
is the only dependency we are having with Android services.

[Dmitry Vyukov]

Then I guess we can add a battery_check param to adb config to turn
this check off.

[$rik@nth]

On Fri, Jun 23, 2017 at 4:16 PM Dmitry Vyukov <dvy...@google.com> wrote:

Then I guess we can add a battery_check param to adb config to turn
this check off.

yes this will help