Use shell cat command as a workaround if ADB stat cannot lookup a file

[ovyalov]

Summary

adbd may fail to access a file due security constraints - in such case sync:stat command returns file's mode as 0. If it's the case - use shell cat a workaround then.

[labath]

Looks good.

Please also run clang-format over the patch (also when I don't request it, i tend to forget about that).

source/Plugins/Platform/Android/AdbClient.cpp

449

We should close the file and make check for the error flags on the stream to make sure the write was successful.

[ldrumm]

source/Plugins/Platform/Android/PlatformAndroid.cpp

244

I have some issues with this approach.

If the filename contains the literal ' character, this command fail with unpredictable results. The solution would seem to be to pass a list of strings a'la POSIX exec i.e.:

- AdbClient::internalShell (const char* command, uint32_t timeout_ms, std::vector<char> &output_buf)
+ AdbClient::internalShell (std::vector<const char*> command, uint32_t timeout_ms, std::vector<char> &output_buf)

but on a little investigation, it does seem that the adb protocol is limited to escaping for the shell command, and does not propagate return status. If I run the following python command:

import subprocess
print(subprocess.check_output(['adb', 'shell', 'cat', "/data/'"]))

The TCP conversation looks like this:

0017host:transport:6D8E7152
OKAY
0012shell:cat /data/\'
OKAY

When I try and cat a file that doesn't exist:

0017host:transport:6D8E7152OKAY0016
shell:cat /nonexistent
OKAY/system/bin/sh: cat: /nonexistent: No such file or directory

So there's seemingly no way to know whether the file exists using adb shell cat $FILE, or whether you're getting the error message as contents.

However, if I use the adb pull command on the same file, the return status is propogated to the calling shell because the stat fails:

0017host:transport:6D8E7152
OKAY
0005sync:
OKAY
STAT..../nonexistent
STAT............

So it seems like the protocol itself does not handle escaping consistently but requires clients to do this on a per adb command basis and there is no way to know the difference between success and failure for the cat file approach.

Unfortunately, I don't actually have a better solution to offer here.

[labath]

You raised good points here, Luke.

I've thought about the quoting issue, but as you have already noticed there is no way to pass arguments containing quotes to adb correctly. Thinking about it more, what we could do is detect this situation (look for ' in the file name) and abort the whole process even before issuing the "adb shell" command.

As for the return value, this should not be a big problem, as further down the line we will discover that this is not a real ELF file and ignore it, but perhaps we could help here by searching for the file name in the first 1K bytes of the result, and ignoring the file if we find it?

Oleksiy, what do you think?

[ovyalov]

Addressed review comments:

• Check whether shell command output starts with /system/bin/sh: in order to find out whether command failed
• Fail-fast download if filename contains single-quotes
• clang-format

[ovyalov]

In D22081#476766, @labath wrote:

You raised good points here, Luke.

I've thought about the quoting issue, but as you have already noticed there is no way to pass arguments containing quotes to adb correctly. Thinking about it more, what we could do is detect this situation (look for ' in the file name) and abort the whole process even before issuing the "adb shell" command.

As for the return value, this should not be a big problem, as further down the line we will discover that this is not a real ELF file and ignore it, but perhaps we could help here by searching for the file name in the first 1K bytes of the result, and ignoring the file if we find it?

Oleksiy, what do you think?

Initially I was thinking that we can pass any shell output to the caller so either module cache or ELF ObjectFile will verify its correctness - but since Platform::GetFile can be used to download an arbitrary file we should try to handle execution errors as much as we can. I added for "/system/bin/sh:" output prefix check - presumably it should cover majority of failures. As an alternative we can run a command like "$user_cmd || echo $rand_id" and then look whether output ends with $rand_id - but it might be an overkill.

[labath]

Thanks for adding the additional checks. Looks good apart from the clang-format changes. This looks like the llvm style. I am not sure how you've run this but it does not seem to have picked up LLDB's .clang-format file.

Are you using git? You need to set clangFormat.binary setting to a sufficient new version (ideally the one you've just built), so it knows how to handle our AlwaysBreakAfterReturnType: All format. Then git clang-format HEAD^ should just work (assuming you have git-clang-format in your path).

[ovyalov]

Reran with proper clang-format.

[ovyalov]

In D22081#477746, @labath wrote:

Thanks for adding the additional checks. Looks good apart from the clang-format changes. This looks like the llvm style. I am not sure how you've run this but it does not seem to have picked up LLDB's .clang-format file.

Are you using git? You need to set clangFormat.binary setting to a sufficient new version (ideally the one you've just built), so it knows how to handle our AlwaysBreakAfterReturnType: All format. Then git clang-format HEAD^ should just work (assuming you have git-clang-format in your path).

Yep, apparently I used llvm style formatting - by running clang/tools/clang-format/clang-format-diff.py. Thank you for pointers - I reran clang-format on my patch, it looks better now.

[ovyalov]

Files:

/lldb/trunk/source/Plugins/Platform/Android/AdbClient.cpp
/lldb/trunk/source/Plugins/Platform/Android/AdbClient.h
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp

Users:

ovyalov (Author)

http://reviews.llvm.org/rL274895

source/Plugins/Platform/Android/AdbClient.h

SetPortForwarding (const uint16_t local_port,
                        const UnixSocketNamespace socket_namespace);
 
     Error
     DeletePortForwarding (const uint16_t local_port);
 
     Error
     Shell (const char* command, uint32_t timeout_ms, std::string* output);
 
+    Error
+    ShellToFile(const char *command, uint32_t timeout_ms, const FileSpec &output_file_spec);
+
     std::unique_ptr<SyncService>
     GetSyncService (Error &error);
 
     Error
     SwitchDeviceTransport ();
 
 private:
     Error
private:
 
     Error
     Sync ();
 
     Error
     StartSync ();
 
     Error
-    ReadAllBytes (void *buffer, size_t size);
+    internalShell(const char *command, uint32_t timeout_ms, std::vector<char> &output_buf);
+
+    Error
+    ReadAllBytes(void *buffer, size_t size);
 
     std::string m_device_id;
     std::unique_ptr<Connection> m_conn;
 };
 
 } // namespace platform_android
 } // namespace lldb_private
 
 #endif  // liblldb_AdbClient_h_
 

source/Plugins/Platform/Android/AdbClient.cpp

 #include "lldb/Core/DataEncoder.h"
 #include "lldb/Core/DataExtractor.h"
 #include "lldb/Core/StreamString.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/FileSpec.h"
 
 #include <limits.h>
 
-#include <cstdlib>
 #include <algorithm>
+#include <cstdlib>
 #include <fstream>
 #include <sstream>
 
 // On Windows, transitive dependencies pull in <Windows.h>, which defines a
 // macro that clashes with a method name.
 #ifdef SendMessage
 #undef SendMessage
 #endif

 
 Error
 AdbClient::ReadAllBytes (void *buffer, size_t size)
 {
     return ::ReadAllBytes (*m_conn, buffer, size);
 }
 
 Error
-AdbClient::Shell (const char* command, uint32_t timeout_ms, std::string* output)
+AdbClient::internalShell(const char *command, uint32_t timeout_ms, std::vector<char> &output_buf)
 {
-    auto error = SwitchDeviceTransport ();
-    if (error.Fail ())
-        return Error ("Failed to switch to device transport: %s", error.AsCString ());
+    output_buf.clear();
+
+    auto error = SwitchDeviceTransport();
+    if (error.Fail())
+        return Error("Failed to switch to device transport: %s", error.AsCString());
 
     StreamString adb_command;
     adb_command.Printf("shell:%s", command);
-    error = SendMessage (adb_command.GetData(), false);
-    if (error.Fail ())
+    error = SendMessage(adb_command.GetData(), false);
+    if (error.Fail())
         return error;
 
-    error = ReadResponseStatus ();
-    if (error.Fail ())
+    error = ReadResponseStatus();
+    if (error.Fail())
         return error;
 
-    std::vector<char> in_buffer;
-    error = ReadMessageStream (in_buffer, timeout_ms);
+    error = ReadMessageStream(output_buf, timeout_ms);
+    if (error.Fail())
+        return error;
+
+    // ADB doesn't propagate return code of shell execution - if
+    // output starts with /system/bin/sh: most likely command failed.
+    static const char *kShellPrefix = "/system/bin/sh:";
+    if (output_buf.size() > strlen(kShellPrefix))
+    {
+        if (!memcmp(&output_buf[0], kShellPrefix, strlen(kShellPrefix)))
+            return Error("Shell command %s failed: %s", command,
+                         std::string(output_buf.begin(), output_buf.end()).c_str());
+    }
+
+    return Error();
+}
+
+Error
+AdbClient::Shell(const char *command, uint32_t timeout_ms, std::string *output)
+{
+    std::vector<char> output_buffer;
+    auto error = internalShell(command, timeout_ms, output_buffer);
     if (error.Fail())
         return error;
 
     if (output)
-        output->assign(in_buffer.begin(), in_buffer.end());
+        output->assign(output_buffer.begin(), output_buffer.end());
     return error;
 }
 
+Error
+AdbClient::ShellToFile(const char *command, uint32_t timeout_ms, const FileSpec &output_file_spec)
+{
+    std::vector<char> output_buffer;
+    auto error = internalShell(command, timeout_ms, output_buffer);
+    if (error.Fail())
+        return error;
+
+    const auto output_filename = output_file_spec.GetPath();
+    std::ofstream dst(output_filename, std::ios::out | std::ios::binary);
+    if (!dst.is_open())
+        return Error("Unable to open local file %s", output_filename.c_str());
+
+    dst.write(&output_buffer[0], output_buffer.size());
+    dst.close();
+    if (!dst)
+        return Error("Failed to write file %s", output_filename.c_str());
+    return Error();
+}
+
 std::unique_ptr<AdbClient::SyncService>
 AdbClient::GetSyncService (Error &error)
 {
     std::unique_ptr<SyncService> sync_service;
     error = StartSync ();
     if (error.Success ())
         sync_service.reset (new SyncService(std::move(m_conn)));
 

source/Plugins/Platform/Android/PlatformAndroid.cpp

PlatformAndroid::GetFile (const FileSpec& source,
     if (source_spec.IsRelative())
         source_spec = GetRemoteWorkingDirectory ().CopyByAppendingPathComponent (source_spec.GetCString (false));
 
     Error error;
     auto sync_service = GetSyncService (error);
     if (error.Fail ())
         return error;
 
-    return sync_service->PullFile (source_spec, destination);
+    uint32_t mode = 0, size = 0, mtime = 0;
+    error = sync_service->Stat(source_spec, mode, size, mtime);
+    if (error.Fail())
+        return error;
+
+    if (mode != 0)
+        return sync_service->PullFile(source_spec, destination);
+
+    auto source_file = source_spec.GetCString(false);
+
+    Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PLATFORM));
+    if (log)
+        log->Printf("Got mode == 0 on '%s': try to get file via 'shell cat'", source_file);
+
+    if (strchr(source_file, '\'') != nullptr)
+        return Error("Doesn't support single-quotes in filenames");
+
+    // mode == 0 can signify that adbd cannot access the file
+    // due security constraints - try "cat ..." as a fallback.
+    AdbClient adb(m_device_id);
+
+    char cmd[PATH_MAX];
+    snprintf(cmd, sizeof(cmd), "cat '%s'", source_file);
+
+    return adb.ShellToFile(cmd, 60000 /* ms */, destination);
 }
 
 Error
 PlatformAndroid::PutFile (const FileSpec& source,
                           const FileSpec& destination,
                           uint32_t uid,
                           uint32_t gid)
 {