Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Function Editor: Togglable auto return storage #6401

Open
astrelsky opened this issue Apr 9, 2024 · 5 comments
Open

Function Editor: Togglable auto return storage #6401

astrelsky opened this issue Apr 9, 2024 · 5 comments
Assignees
Labels
Feature: Functions Function signature, creation, management Feature: Language/Rust Status: Triage Information is being gathered

Comments

@astrelsky
Copy link
Contributor

astrelsky commented Apr 9, 2024

Is your feature request related to a problem? Please describe.
I'm always frustrated when I'm reversing something written in a garbage programming language, such as rust, which is consistently inconsistent about how the return type is stored. Sometimes it is in RAX:RDX and sometimes it is auto return storage even if it will fit in RAX:RDX.

Describe the solution you'd like
A button to toggle whether auto return storage is used or not.

Describe alternatives you've considered
Having to use custom storage every time and then deal with the decompiler being unable to split the return variable so you can change the type in every function that uses it. This of course leaves a huge unmanageable mess.

Additional Context
The inability to split variables when necessary is probably unrelated since it's a well known problem 😞.

@ryanmkurtz ryanmkurtz added Feature: Functions Function signature, creation, management Status: Triage Information is being gathered Type: Enhancement New feature or request labels Apr 10, 2024
@ghidra1
Copy link
Collaborator

ghidra1 commented Apr 10, 2024

The use of auto-return-storage is only supported by dynamic storage allocation (non-custom storage) based upon the current calling convention and the return datatype. If using custom storage we rely on using a parameter named __return_storage_ptr__ and the return datatype reflecting the actual returned datatype and not its formal datatype which may have been specified within source code.

The Rust programming language is a work-in-progress and has know issues with the handling of the return type which is quite complicated. It's unclear how adding a button would help this. Ideally, non-custom storage should be able to make the approriate determination based upon the assigned calling convention and the datatypes in use. Unfortunately, one case is particularly problematic when the return datatype is a structure whose deinition is incomplete. If you could provide a few specific examples of signatures which are not allocated correctly when all the datatype are defined that could help.

I will pass this along to someone on the team more familiar with Rust.

@ghidra1 ghidra1 added Feature: Language/Rust and removed Type: Enhancement New feature or request labels Apr 10, 2024
@astrelsky
Copy link
Contributor Author

The use of auto-return-storage is only supported by dynamic storage allocation (non-custom storage) based upon the current calling convention and the return datatype. If using custom storage we rely on using a parameter named __return_storage_ptr__ and the return datatype reflecting the actual returned datatype and not its formal datatype which may have been specified within source code.

The Rust programming language is a work-in-progress and has know issues with the handling of the return type which is quite complicated. It's unclear how adding a button would help this. Ideally, non-custom storage should be able to make the approriate determination based upon the assigned calling convention and the datatypes in use. Unfortunately, one case is particularly problematic when the return datatype is a structure whose deinition is incomplete. If you could provide a few specific examples of signatures which are not allocated correctly when all the datatype are defined that could help.

I will pass this along to someone on the team more familiar with Rust.

It is the same problem we see with C++ when it has a non-trivial destructor. Even if the struct will fit in RAX:RDX it will not be put in the register pair. Unfortunately the problem is that it appears to be much more prevalent with rust. So instead of having to manually use custom storage and fix the parameters for every case, it would be nice to be able to just check a box and be done.

The problem can be seen with std::sys::unix::fs::File::open_c. I don't have a sample on hand but it should be pretty simple to reproduce. I put some appropriate C-like definitions below to save you from having to figure out how the hell it's actually represented in memory.

#include <stdbool.h>
#include <stdint.h>

enum ResultTag4 { // 4-byte size
	Ok,
	Err
};

// std::os::fd::owned::OwnedFd
struct OwnedFd {
	// closed on drop
	int fd;
};

// std::sys::unix::fd::FileDesc
struct FileDesc {
	struct OwnedFd __0;
};

// std::sys::unix::fs::File
struct File {
	struct FileDesc __0;
};

// std::io::error::Error
struct Error {
	void *impl;
};

// std::sys::unix::fs::OpenOptions
struct OpenOptions {
	int custom_flags;
	int mode;
	bool read;
	bool write;
	bool append;
	bool truncate;
	bool create;
	bool create_new;
};

// std::io::error::Result<std::sys::unix::fs::File, std::io::error::Error>
struct Result__File__Error {
	enum ResultTag4 tag;
	// below is actually a union but can't be put in one due to alignment
	// it ends up being layed out correctly just like this
	struct File File;
	struct Error Error;
};

struct CStr;

// std::sys::unix::fs::File::open_c(path: &CStr, opts: &OpenOptions) -> Result<File>
struct Result__File__Error File__open_c(struct CStr *path, struct OpenOptions *opts);

@ghidra1
Copy link
Collaborator

ghidra1 commented Apr 10, 2024

It is important that we try to get the compiler specification and allocations correct since it not only affects the forward direction (i.e., allocation when signature is known), it also affect the reverse direction when decompiler attempts to make the determination. Not sure I am ready to cave and add the easy-button quite yet, although I will discuss with others. It's is not quite as a straight-forward as you may think and would need to ripple through the Function and Prototype Model API. Such a mechanism would only be applicable when custom storage is disabled.

@ghidra1
Copy link
Collaborator

ghidra1 commented Apr 11, 2024

Since this same issue also impacts how parameters are passed this needs to be addressed at the compiler spec level. The current thinking is that an optional attribute is needed on composite datatypes (e.g., class structure, etc.) that could force the use of a cspec rule with the convert_to_ptr action for both return/output and parameters/inputs.

@astrelsky
Copy link
Contributor Author

astrelsky commented Apr 11, 2024

Since this same issue also impacts how parameters are passed this needs to be addressed at the compiler spec level. The current thinking is that an optional attribute is needed on composite datatypes (e.g., class structure, etc.) that could force the use of a cspec rule with the convert_to_ptr action for both return/output and parameters/inputs.

Yes, putting an attribute/setting on the datatype makes the most sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: Functions Function signature, creation, management Feature: Language/Rust Status: Triage Information is being gathered
Projects
None yet
Development

No branches or pull requests

3 participants