refactor path resolving #265

xvuko · 2023-06-20T18:04:23Z

Main point of this is to remove char tmp_buf[PATH_MAX]; from stack. I refactored rest of the function to make it (hopefully) easier to read.

This should give same output as before. Only change is case of empty symlink as it was setting errno to 0.

Description

Motivation and Context

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

Already covered by automatic testing.
New test added: (add PR link here).
Tested by hand on: (list targets here).

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing linter checks and tests passed.
My changes generate no new compilation warnings for any of the targets.

Special treatment

This PR needs additional PRs to work (list the PRs, preferably in merge-order).
I will merge this PR by myself when appropriate.

unistd/dir.c

niewim19

Hey, I have done some research on this change and I found some issues within this change and in current implementation. @damianloew I have described current issue in section 2 of this comment. I will also make an issue on this.

Section 1, issue in proposed change

Test code below works on host and current implementation but causes segmentation fault on proposed change.

Section 2, error in current implementation

~~Makeshift code that I have produced to check Section 1 error has found a discrepancy between current implementation of resolve_path() (via realpath()) and the host one.~~

~~Test code below resolves to /etc on host, but /symShort on phoenix, which is in violation with goal of realpath() which should remove symlinks.~~

Edit: not valid, current implementation works according to POSIX

Test code

Test code tries to resolve very long path that starts with symlink. It instantiates:

symlink symNameLong with long name that points to "/"
symlink symNameShort that points to symNameLong
basePath path that goes like: "symShort/etc/../etc/<and so on up to almost PATH_MAX length>/../etc" that should finally land in etc.

#include <stdio.h>
#include <unistd.h>
#include <sys/stat.h>
#include <stdlib.h>
#include <limits.h>
#include <string.h>
#include <errno.h>

#define NAME_LEN (NAME_MAX / 2)

int main(int argc, char **argv) {
	char symNameShort[] = "symShort";
	char symNameLong[NAME_LEN];
	char basePath[] = "symShort/etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/../etc/";
	char resolved[PATH_MAX + 1];

	memset(symNameLong, 'a', NAME_LEN);
	symNameLong[NAME_LEN - 1] = 0;

	if (symlink("/", symNameLong) < 0)
		printf("%s\n", strerror(errno));

	if (symlink(symNameLong, symNameShort) < 0)
		printf("%s\n", strerror(errno));

	realpath(basePath, resolved);
	resolved[PATH_MAX] = 0;
	printf("%s\n", resolved);

	/* cleanup */
	remove(symNameShort);
	remove(symNameLong);
}

basePath is resolved to:

/etc on host
/symShort on phoenix (tested on zturn, and confimed by @damianloew on ia32)
segmentation fault on proposed change

nalajcie · 2023-06-21T08:32:01Z

small notes from me:

the code looks sane (but I haven't tested it), I was thinking about similar approach at the initial approach but decided the increased code complexity wasn't worth the case
please write in commit message body regarding WHY this change is needed (or what we gained) to leave trail
@niewim19 the _readlink_abs instead of lstat was in the original approach so this is not a functionality change
if there is a test that highlights possible error in current / new implementation - please try to add it to unit tests :)
I'll try to test it in the common use cases on DTR project, will follow up then

unistd/dir.c

niewim19 · 2023-06-21T08:40:03Z

small notes from me:

* the code looks sane (but I haven't tested it), I was thinking about similar approach at the initial approach but decided the increased code complexity wasn't worth the case

* please write in commit message body regarding WHY this change is needed (or what we gained) to leave trail

* @niewim19 the `_readlink_abs` instead of `lstat` was in the original approach so this is not a functionality change

* if there is a test that highlights possible error in current / new implementation - please try to add it to unit tests :)

* I'll try to test it in the common use cases on DTR project, will follow up then

I`m sorry for that - it is indeed not a functionality change. I have misunderstood diff. I will amend previous message
I spoke with test team (@damianloew) about this issue, they will take it from here.

damianloew · 2023-06-21T08:41:51Z

small notes from me:

* the code looks sane (but I haven't tested it), I was thinking about similar approach at the initial approach but decided the increased code complexity wasn't worth the case

* please write in commit message body regarding WHY this change is needed (or what we gained) to leave trail

* @niewim19 the `_readlink_abs` instead of `lstat` was in the original approach so this is not a functionality change

* if there is a test that highlights possible error in current / new implementation - please try to add it to unit tests :)

* I'll try to test it in the common use cases on DTR project, will follow up then

I`m sorry for that - it is indeed not a functionality change. I have misunderstood diff. I will amend previous message
I have spoke with test team (@damianloew) about this issue, they will take it from here.

Yeah, the issue will be created and we will add tests to cover this issue.

nalajcie · 2023-06-21T09:16:33Z

Yeah, the issue will be created and we will add tests to cover this issue.

If the current code works, there is no need to create the issue. Please just add the test case to help @xvuko narrow down the issue :)

agkaminski · 2023-06-21T09:24:35Z

@xvuko Hmmm, I don't know... Doesn't this solution cause us to be unable to on a long path not being able to solve even longer symlink? Total path length being shorter than PATH_MAX in both cases, but us being unable to solve it anyway

nalajcie · 2023-06-21T09:58:59Z

@xvuko Hmmm, I don't know... Doesn't this solution cause us to be unable to on a long path not being able to solve even longer symlink? Total path length being shorter than PATH_MAX in both cases, but us being unable to solve it anyway

I think the original solution would also fail if symlink resolved length + remaining (unresolved) path len exceeds PATH_MAX (while resolving, even if the expected resolve result would be < PATH_MAX).

damianloew · 2023-06-21T10:47:25Z

@xvuko Hmmm, I don't know... Doesn't this solution cause us to be unable to on a long path not being able to solve even longer symlink? Total path length being shorter than PATH_MAX in both cases, but us being unable to solve it anyway

I think the original solution would also fail if symlink resolved length + remaining (unresolved) path len exceeds PATH_MAX (while resolving, even if the expected resolve result would be < PATH_MAX).

We checked it and in fact there is no issue in the current libphoenix implementation as you mentioned @nalajcie So I've updated the issue and closed it. However the exception shouldn't be raised when applying changes from the PR when running code from the mentioned case.

unistd/dir.c

xvuko · 2023-06-21T14:32:22Z

* `segmentation fault` on proposed change

Thanks for testing this. There was a bug in my code:

assert(symlink_len <= path - p);                                                                                                                                                                                                                                                                                                                                          
if (symlink_len == path - p) {                                                                                                                                                                                                                                                                                                                                            
      return SET_ERRNO(-ENAMETOOLONG);                                                                                                                                                                                                                                                                                                                                  
}

I changed path - p to p - path and now /symShort is printed.

nalajcie · 2023-06-21T14:52:31Z

ok, so maybe adding the mentioned test would still be beneficial?
we could introduce symlink_max_len = p - path but I'm not sure if it would increase readability or not
(Just analyzing code) I see that / is being added unconditionally (also if leaf) so we might end up in with eg. /dir/realfile/ in path - but from reading the code it seems it would correctly be handled? (this reduces max resolved symlink len by one from the previous implementation but IMHO doesn't really matter)

xvuko · 2023-06-21T15:42:24Z

ok, so maybe adding the mentioned test would still be beneficial?

Yes, I will add this test.

* we could introduce `symlink_max_len = p - path`  but I'm not sure if it would increase readability or not

Yeah, I was thinking about that. Added as it is a good place for comment.

* (Just analyzing code) I see that `/` is being added unconditionally (also if leaf) so we might end up in with eg. `/dir/realfile/` in `path` - but from reading the code it seems it would correctly be handled? (this reduces max resolved symlink len by one from the previous implementation but IMHO doesn't really matter)

'/' at the and is not an issue as trailing slashes are always ignored.

agkaminski

Some MISRA-flavored nitpicks, otherwise LGTM

unistd/dir.c

damianloew · 2023-07-03T10:10:23Z

ok, so maybe adding the mentioned test would still be beneficial?

Yes, I will add this test.
* we could introduce `symlink_max_len = p - path`  but I'm not sure if it would increase readability or not
Yeah, I was thinking about that. Added as it is a good place for comment.
* (Just analyzing code) I see that `/` is being added unconditionally (also if leaf) so we might end up in with eg. `/dir/realfile/` in `path` - but from reading the code it seems it would correctly be handled? (this reduces max resolved symlink len by one from the previous implementation but IMHO doesn't really matter)
'/' at the and is not an issue as trailing slashes are always ignored.

The test is already added and it's now running during CI.

PATH_MAX buffer is removed from stack frame. This reduces stack usage and makes it more uniform between platforms. This should give same output as before. Only change is case of empty symlink as it was setting errno to 0. JIRA: RTOS-504 Change-Id: I6402583e76f3bd12a951fe78830d48ac0c126af3

agkaminski

LGTM

github-actions bot reviewed Jun 20, 2023

View reviewed changes

unistd/dir.c Outdated Show resolved Hide resolved

xvuko requested review from nalajcie and niewim19 June 20, 2023 18:14

xvuko marked this pull request as ready for review June 20, 2023 18:14

xvuko force-pushed the xvuko/rtos-504 branch 2 times, most recently from b8e6905 to d560418 Compare June 20, 2023 18:24

niewim19 reviewed Jun 21, 2023

View reviewed changes

nalajcie reviewed Jun 21, 2023

View reviewed changes

unistd/dir.c Outdated Show resolved Hide resolved

nalajcie reviewed Jun 21, 2023

View reviewed changes

unistd/dir.c Outdated Show resolved Hide resolved

mateusz-bloch mentioned this pull request Jun 21, 2023

Discrepancy in resolve_path() Behavior and Segmentation Fault with PR phoenix-rtos/libphoenix#265 phoenix-rtos/phoenix-rtos-project#761

Closed

xvuko force-pushed the xvuko/rtos-504 branch from d560418 to fdf372a Compare June 21, 2023 14:23

github-actions bot reviewed Jun 21, 2023

View reviewed changes

unistd/dir.c Outdated Show resolved Hide resolved

xvuko force-pushed the xvuko/rtos-504 branch from fdf372a to b56a6e3 Compare June 21, 2023 14:33

xvuko force-pushed the xvuko/rtos-504 branch from b56a6e3 to c1ad669 Compare June 21, 2023 15:31

agkaminski reviewed Jul 3, 2023

View reviewed changes

unistd/dir.c Outdated Show resolved Hide resolved

unistd/dir.c Outdated Show resolved Hide resolved

unistd/dir.c Outdated Show resolved Hide resolved

unistd/dir.c Outdated Show resolved Hide resolved

unistd/dir.c Outdated Show resolved Hide resolved

xvuko force-pushed the xvuko/rtos-504 branch from c1ad669 to ee2c2f9 Compare July 3, 2023 11:56

agkaminski approved these changes Jul 3, 2023

View reviewed changes

xvuko merged commit 9ca11e5 into master Jul 3, 2023
24 checks passed

xvuko deleted the xvuko/rtos-504 branch July 3, 2023 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor path resolving #265

refactor path resolving #265

xvuko commented Jun 20, 2023

niewim19 left a comment •

edited

Loading

nalajcie commented Jun 21, 2023

niewim19 commented Jun 21, 2023 •

edited

Loading

damianloew commented Jun 21, 2023

nalajcie commented Jun 21, 2023

agkaminski commented Jun 21, 2023

nalajcie commented Jun 21, 2023

damianloew commented Jun 21, 2023

xvuko commented Jun 21, 2023

nalajcie commented Jun 21, 2023

xvuko commented Jun 21, 2023 •

edited

Loading

agkaminski left a comment

damianloew commented Jul 3, 2023

agkaminski left a comment

refactor path resolving #265

refactor path resolving #265

Conversation

xvuko commented Jun 20, 2023

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

Special treatment

niewim19 left a comment • edited Loading

Choose a reason for hiding this comment

Section 1, issue in proposed change

Section 2, error in current implementation

Test code

nalajcie commented Jun 21, 2023

niewim19 commented Jun 21, 2023 • edited Loading

damianloew commented Jun 21, 2023

nalajcie commented Jun 21, 2023

agkaminski commented Jun 21, 2023

nalajcie commented Jun 21, 2023

damianloew commented Jun 21, 2023

xvuko commented Jun 21, 2023

nalajcie commented Jun 21, 2023

xvuko commented Jun 21, 2023 • edited Loading

agkaminski left a comment

Choose a reason for hiding this comment

damianloew commented Jul 3, 2023

agkaminski left a comment

Choose a reason for hiding this comment

niewim19 left a comment •

edited

Loading

niewim19 commented Jun 21, 2023 •

edited

Loading

xvuko commented Jun 21, 2023 •

edited

Loading