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

Fix stack organization #268

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix stack organization #268

wants to merge 1 commit into from

Conversation

rafajunio
Copy link

@rafajunio rafajunio commented Nov 1, 2024

Wrong calculation near the limits of argv, envv and auxv

Copy link
Contributor

@dledda-r7 dledda-r7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello! Thanks for your PR.
I am leaving couple of questions to get a bit of context for these changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were you observing any strange behavior before making these changes? If so, what were you experiencing?
Could you provide a PoC of an unexpected result you had and how this fix is solving the problem?
This will really help us understand what is going on.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @dledda-r7 , for testing it is possible to use the follow code (print_stack.c):

#include <stdio.h>

extern char **environ;

int main(int argc, char **argv)
{
	int cnt;
	size_t *stack = (size_t *)argv - 1;

	printf("argc: %d\n", argc);
	printf("\n");
	for (cnt = 0; argv[cnt] != NULL; cnt++) {
		printf("argv[%d]: %s\n", cnt, argv[cnt]);
	}
	printf("\n");

	for (cnt = 0; environ[cnt] != NULL; cnt++) {
		printf("envv[%d]: %s\n", cnt, environ[cnt]);
	}

	printf("Stack:\n");
	for (cnt = 0; stack[cnt] || stack[cnt + 1]; cnt++) {
		printf("  0x%08zx\n", stack[cnt]);
	}
	return 0;
}

Compile as follow:
gcc print_stack.c -o print_stack
Testing:
./print_stack hello this is a test

From the libreflect it is possible to use the follow code:
./noexec print_stack hello this is a test

@dledda-r7 dledda-r7 self-assigned this Dec 6, 2024
@rafajunio rafajunio requested a review from dledda-r7 December 30, 2024 13:36
Copy link

@acammack acammack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, off-by-one errors, the bane of long nights and blurry debugger sessions. This one is made even worse by the debug output looking correct as it mangles the stack it's trying to produce and that stack mostly working 😈 . Thanks for the catch, cheers!

@rafajunio rafajunio requested a review from acammack February 1, 2025 12:58
Copy link

@acammack acammack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the code tests correctly and looks good to me! As a heads up, Rapid7 usually requires PRs to be made from unique topic branches, though I'll let the current team members take it from here

@adfoster-r7
Copy link
Contributor

@rafajunio It'd be great to rebase this PR, we had to make some changes for CI to run: #272

Wrong calculation near the limits of argv, envv and auxv
@rafajunio rafajunio reopened this Feb 27, 2025
@rafajunio rafajunio requested a review from acammack February 27, 2025 20:40
@rafajunio
Copy link
Author

Lesson learned, not use master to create PR anymore =) (my fork)

Thank you, I hope that now is everything good. I merge the commits in the rebase as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants