848843 - dmSUT should not call "cat"

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[William Lachance (:wlach)]

As mcote has pointed out, this method is inherently unsafe, because there is no way to accurately read the number of bytes in the file. The only way to determine whether we're finished reading is to detect the prompt, but we have no way to be sure that the prompt wasn't part of the file.

The "pull" command does everything "cat" does, except safely, since it also transmits the file size. I think we have two options here:

1. Change dmSUT's catFile to just be a wrapper around pullFile. This will preserve backwards compatibility.
2. Remove the catFile method altogether. This will require changes in any client code that uses it, but as far as I can tell nothing does anymore. Even sut_tools (https://hg.mozilla.org/build/tools/file/c290db3dd537/sut_tools/) seems to have migrated off of it.

I prefer (2)

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

I am fine with #2.

The one exception I know of is in the case of temperature monitoring which I am working on now via:
cat /sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input, pullFile doesn't work for me.  
I also need to adjust the timeouts and stuff, so I wouldn't be using the pullFile function anyway.

I wonder if we can pullFile watcher.ini successfully?

Comment 2

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #1)
> I am fine with #2.
> 
> The one exception I know of is in the case of temperature monitoring which I
> am working on now via:
> cat /sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input, pullFile
> doesn't work for me.  
> I also need to adjust the timeouts and stuff, so I wouldn't be using the
> pullFile function anyway.
>
> I wonder if we can pullFile watcher.ini successfully?

To be clear, calling "exec cat" via shellCheckOutput or whatever is still supported. :) It's calling the "cat" command directly which I want to discourage. I assume that's what you're using, and not catFile....

Actually, now that I think about it, since sut_tools does not seem to call the "cat" command, maybe we can outright take it out of the java agent.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

I am using:
dm._runCmds([{'cmd': 'cat /sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input'}], timeout=1)

This works just fine, but pullFile fails.  I am not using catFile because I needed to limit the timeout.  I would not support pulling cat out of the java agent.

Comment 4

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #3)
> I am using:
> dm._runCmds([{'cmd': 'cat
> /sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input'}], timeout=1)
>
> This works just fine, but pullFile fails.  I am not using catFile because I
> needed to limit the timeout.  I would not support pulling cat out of the
> java agent.

If 'cat' went away, you could just replace this with dm._runCmds([{'cmd': 'exec cat /sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input'}], timeout=1). 

Better yet, it is possible to do this without calling internal dmSUT-specific methods by using shellCheckOutput as follows: 

dm.shellCheckOutput(['cat', '/sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input']). 

This has the advantage of not using an internal method which is subject to change, as well as working with DeviceManagerADB. 

Regardless, whether "cat" stays in the agent is not the important thing. I'm more concerned with either kill or modifying the unsafe catFile method inside mozdevice.

Comment 5

[Mark Côté [:mcote]]

I would be very interested in knowing why pull doesn't work with that file.  I'll check it out on my panda.

Comment 6

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #4)
> (In reply to Joel Maher (:jmaher) from comment #3)

> dm.shellCheckOutput(['cat',
> '/sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input']). 

As discussed on irc, that should be dm.shellCheckOutput(['cat', '/sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input'], timeout=1) if you want the timeout...

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

I have verified this works and life is all good!

Comment 8

[William Lachance (:wlach)]

Attachment: Remove catFile method

Ok, since we're at least agreed that catFile isn't necessary anymore, let's remove it.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 722439 [details] [diff] [review]
Remove catFile method

Review of attachment 722439 [details] [diff] [review]:
-----------------------------------------------------------------

great stuff!

Comment 10

[William Lachance (:wlach)]

Pushed: https://github.com/mozilla/mozbase/commit/9edbd3d270ab9e2c0dfba22f101b5afe782de7b0

Let's punt on changing the agent for now. It doesn't seem particularly urgent.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

hmm, I have switched back to my _sendCmd, checkShellOutput is raising exceptions that are not getting caught when I fail to connect.  I will play with this some more when I have time, but I need to keep my machines and tests running.

Comment 12

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #11)
> hmm, I have switched back to my _sendCmd, checkShellOutput is raising
> exceptions that are not getting caught when I fail to connect.  I will play
> with this some more when I have time, but I need to keep my machines and
> tests running.

If you could attach a text file demonstrating the problem I'd be interested. shellCheckOutput is supposed to be usable for such things, if it isn't it's a problem. :)

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

I needed to add
try:
  dm.checkShellOutput(...)
except Exception:
  pass

The key was I needed except Exception.