[rust] Better handling for gnirehtet.apk path

[stek29]

Currently it'd just execute adb install -r gnirehtet.apk in whatever dir user was.

Not only is that insecure, that also makes it impossible to be run from any location except same dir, and makes distribution impossible.

So, I have two suggestions:

• chdir to gnirehtet file location before running any commands
• provide a way to override the path (via environment variable for example)

[stek29]

This is stopping Homebrew Cask for example.

[rom1v]

chdir to gnirehtet file location before running any commands

See https://github.com/Genymobile/gnirehtet/issues/6.

provide a way to override the path (via environment variable for example)

I was going to answer that it was already available, but you're right it is not. I only implemented it for scrcpy for now.

Thank you for the report.

[stek29]

@rom1v

To make it callable from everywhere, we have to detect the actual directory of the script (after symlink resolution), which is unfortunately not easily done by calling commands in a portable way.

use std::env;
use std::error::Error;
use std::fs;
use std::path::PathBuf;

fn chdir_to_executable_dirname() -> bool {
    fn get_executable_dir_path() -> Result<PathBuf, Box<Error>> {
        let exe = env::current_exe()?;
        let real_exe = fs::canonicalize(exe.as_path())?;
        match real_exe.as_path().parent() {
            Some(real_dir) => Ok(real_dir.to_path_buf()),
            None => Err(From::from(""))
        }
    }

    match get_executable_dir_path() {
        Ok(s) => env::set_current_dir(s).is_ok(),
        Err(_) => false
    }
}

fn main() {
    println!("chdir: {}", chdir_to_executable_dirname());
}

took me ~1.5 hours with 0 prior rust knowledge :)

[Lonami]

None => Err(From::from(""))

Well while you're at this thing of creating errors you could provide some more meaningful string :P

Edit: I could further clean it up to the following (I'm also learning…):

use std::env;
use std::fs;

fn chdir_to_executable_dirname() -> bool {
    let exe = match env::current_exe() {
        Ok(x) => x,
        Err(_) => return false
    };

    let real_exe = match fs::canonicalize(exe.as_path()) {
        Ok(x) => x,
        Err(_) => return false
    };

    if let Some(real_dir) = real_exe.as_path().parent() {
        env::set_current_dir(real_dir).is_ok()
    } else {
        false
    }
}

[stek29]

@Lonami you know that's just a PoC

[Lonami]

Pinging @JuanPotato for further review!

[JuanPotato]

fn chdir_to_executable_dirname() -> bool {
    env::current_exe()
        .and_then(|exe| fs::canonicalize(exe.as_path()))
        .ok()
        .map_or(false, |real_exe| {
            real_exe
                .as_path()
                .parent()
                .map_or(false, |real_dir| env::set_current_dir(real_dir).is_ok())
        })
}

I'm sorry, I like chaining things.

[stek29]

@JuanPotato Now change this

fn get_apk_path() -> Result<String, Box<Error>> {
    fn get_executable_path() -> Result<PathBuf, Box<Error>> {
        let exe = env::current_exe()?;
        let real_exe = fs::canonicalize(exe.as_path())?;
        Ok(real_exe)
    }

    let candidate_path = match env::var("GNIREHTET_APK") {
        Ok(s) => Ok(PathBuf::from(s)),
        Err(_) => match get_executable_path() {
            Ok(path) => Ok(path.with_file_name("gnirehtet.apk")),
            Err(e) => Err(e)
        }
    }?;

    if candidate_path.as_path().is_file() {
        Ok(candidate_path.to_str().unwrap().to_string())
    } else {
        Err(From::from(format!("Expected to find APK at {}. You may want to set GNIREHTET_APK enviroment variable.", candidate_path.display())))
    }
}

Note: I have absolutely 0 rust experience, I'm sorry if reading this code hurts. Also, It probably should return &str but I haven't figured out lifetimes. Probably return it as Option without any Error's since it won't be easy to incorporate Errors into current code, but Option would fit well (if None then error! can be used)

For the reference, it would be used here:

https://github.com/Genymobile/gnirehtet/blob/a943db0af49ed06e9483f04ea8e6bd07f44bec50/relay-rust/src/main.rs#L300-L303

[rom1v]

provide a way to override the path (via environment variable for example)

I just did this on dev: 15d10c79ef18364361966b6fecf658efa41dc3b6 (I also added a way to customize adb: 081714825ff3cc5ace8a6055c6a239cfb8887841)

[SalvatoreT]

@rom1v, when you land those changes from dev on master, can you cut a minor/patch release? I'll update the Homebrew formula to point to it.

[SalvatoreT]

@rom1v, are we just waiting on https://github.com/Genymobile/gnirehtet/issues/273 at this point?

[rom1v]

@SalvatoreT I just published a new release v1.5 with this change and the fix for #273.

Sorry for the delay. (I work on gnirehtet and scrcpy on my free time, and scrcpy has higher priority.)

I wanted to change other things in gnirehtet before the release (like automatically close gnirehtet on the device on disconnection, unless an option is passed), but I had no time to work on it, so I just published the current dev branch.

[SalvatoreT]

Thank you, @rom1v! I just opened a PR to update the Homebrew formula. https://github.com/Homebrew/homebrew-core/pull/59767

Sorry for the delay. (I work on gnirehtet and scrcpy on my free time, and scrcpy has higher priority.)

Not problem! I can definitely relate. I use both gnirehtet and scrcpy all the time and they make my development experience so much better! Thank you for everything you do!

I wanted to change other things in gnirehtet before the release (like automatically close gnirehtet on the device on disconnection, unless an option is passed), but I had no time to work on it, so I just published the current dev branch.

Roger that. Thank you for cutting early.