rizinorg / rizin

UNIX-like reverse engineering framework and command-line toolset.
https://rizin.re
GNU Lesser General Public License v3.0
2.66k stars 357 forks source link

Environment variables parsed for rz-run prefixes when debugging #3646

Open imyxh opened 1 year ago

imyxh commented 1 year ago

Work environment

Questions Answers
OS/arch/bits (mandatory) Linux 6.4.1-arch2-1 x86_64
File format of the file you reverse (mandatory) any?
Architecture/bits of the file (mandatory) any?
rizin -v full output, not truncated (mandatory) rizin 0.5.2 @ linux-x86-64

Expected behavior

Debugging should start normally without warnings, but I need to zero out a few environment variables first for this to happen:

$ XMODIFIERS= DISPLAY= rizin -dN /bin/ls
Process with PID 768798 started...
[0x7f60188d3d70]> 

Actual behavior

If I don't unset those environment variables (XMODIFIERS=@im=fcitx and DISPLAY=:0), rizin attempts to parse them with the rz-run @ (filename) and : (hexpair) prefixes:

$ rizin -dN /bin/ls                     
ERROR: rz-run: invalid hexpair string `0`
ERROR: rz-run: invalid @<num>@ in `@im=fcitx`
Process with PID 759320 started...
[0x7faea9dadd70]> 

Steps to reproduce the behavior

See code blocks above. MWE is FOO=@ rizin -d /bin/ls.

wargio commented 1 year ago

I actually have the same behaviors, but is ok since it is just due how the special variables for rz-run works. These will be tossed as errors, but they are not important. I can whitelist some keywords if needed like for DISPLAY=: and @im= These errors does not impact the debug environment.

imyxh commented 1 year ago

@wargio I wouldn't quite say it doesn't impact the debug environment; what if you're debugging a program that expects (e.g.) DISPLAY to be properly set? Currently if you rizin -d env and dc you can see that the process does not get passed the variables that rz-run failed to parse.

As such I don't think that setenv and the environment variables inherited from the shell should have special parsing by rz-run. What if you want to do something like rizin -d -R "setenv=DISPLAY=:3" myprogram? You can't do it (afaik) as things currently are. I don't see a way to escape the : and even if there were such a way I don't think there's a clean way to do it.

wargio commented 1 year ago

i guess i can add a failover, if we fail to parse it then we just set it as it is?

imyxh commented 1 year ago

Yeah, but what if you want DISPLAY=:10 or something. You can probably put ":10" in a file as a workaround but it's non-obvious. I'd imagine that most users just expect the environment variables they have set to just work and not be messed with.

Is there much of a use case for environment variables with exotic values? If so it may be worth letting setenv parse the prefixes, and adding a second setenv directive (setenvr?) that doesn't parse any prefixes, and making sure inherited environment variables are parsed with setenvr and not setenv.

imyxh commented 1 year ago

btw I think the manpage is a little messed up too?

text        Escape characters useful for hex chars
'string'    Escape characters useful for hex chars

not sure what was supposed to be here but it doesn't look right

wargio commented 1 year ago

don't look to manpages. they have never been updated, even on r2

imyxh commented 1 year ago

Heh. Well I'm happy to work on them and can update this one once we've decided what'll happen.

wargio commented 1 year ago

i think we can just set the env variable to whatever if we fail to parse them.

ret2libc commented 1 year ago

@wargio why do we need to parse the env vars through rz-run by default? Can't we have a special setting for rz-run parsed vars?

wargio commented 1 year ago

because you can (don't ask me why) give env vars with special meanings outside of rz-run. for example XXX=@foo@ rizin -d /bin/ls and @foo@ gets substituted with whatever is set in the rizin session.

ret2libc commented 1 year ago

Shall we change this behaviour? It does not seem too good/useful. I think if one wants a particular run environment he should use rz-run or a rz-run profile or just the env variable from the rizin shell.

What do you think @wargio @XVilka ?

XVilka commented 1 year ago

I think we can remove this quirk in favor of specifying all necessary things in a profile.

wargio commented 1 year ago

same

XVilka commented 1 year ago

@kazarmy you recently worked on the related things. Maybe you could take a look at this one?