Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 342983 - sys-apps/sandbox-2.3 mishandles relative mkdirat() when fd!=AT_FDCWD
Summary: sys-apps/sandbox-2.3 mishandles relative mkdirat() when fd!=AT_FDCWD
Status: RESOLVED FIXED
Alias: None
Product: Portage Development
Classification: Unclassified
Component: Sandbox (show other bugs)
Hardware: All Linux
: High normal (vote)
Assignee: Sandbox Maintainers
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 342681
  Show dependency tree
 
Reported: 2010-10-27 19:18 UTC by Xake
Modified: 2010-11-24 01:01 UTC (History)
6 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
sample code to show the behaviour (sandbox-poc.c,545 bytes, text/plain)
2010-10-27 19:21 UTC, Xake
Details
Make mkdirat_pre_check resolv the dirfd if dirfd != AT_FDCWD (0001-Patch-that-makes-mkdirat_pre_check-resolv-the-dirfd-.patch,2.49 KB, patch)
2010-10-28 08:28 UTC, Xake
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xake 2010-10-27 19:18:01 UTC
So I investigated bug #342681 and the test 20 and realized that sandbox currently does not handle mkdirat very well.

The problem shows itself if you have a directory, open a file director to that directory and try with mkdirat create a directory in the old directory with the same name as its parent.

For example if you have code that
mkdir("~/one"); fd=open("~/one"); mkdirat(fd,"one");
this works fine outside of sandbox, but will fail inside sandbox because erealpath/canonicalize checks ~/one, and sends back to the program calling mkdirat that EEXISTS.

however the following works fine inside of sandbox as well:
mkdir("~/one"); fd=open("~/one"); mkdirat(fd,"two");
Comment 1 Xake 2010-10-27 19:21:15 UTC
Created attachment 252287 [details]
sample code to show the behaviour

I know my comment may sound vague, but here is code that shows the behaviour.

This is what happends n my system if I run it:

xake@lillen ~/devel $ cc -o poc sandbox-poc.c
xake@lillen ~/devel $ rm -rf one && ./poc
Have created and open dir "one"
About to create dir "one/one"...
Have created "one/one"
xake@lillen ~/devel $ sandbox
[s] xake@lillen ~/devel $ rm -rf one && ./poc
Have created and open dir "one"
About to create dir "one/one"...
Failed create "one/one": -1, File exists
[s] xake@lillen ~/devel $ 


As you may see it runs fine outside of sandbox, but inside of sandbox it does not, and with SANDBOX_DEBUG you will get:
...
About to create dir "one/one"...
EARLY FAIL  mkdirat(one[/home/xake/devel/one]) @ lstat: Success
Failed create "one/one": -1, File exists
...
So it checks one level too low...
Comment 2 Xake 2010-10-28 06:50:36 UTC
So mkdirat_pre_check canonicalize the pathname, and then it EEXISTS if the canonical filepath already exists.
This is done before before_syscall() is run.

The problem is that it seems like if you give canonicalize a relative path it always return CWD/RELPATH. So if you for example try to with mkdirat(A,A) this will fail since CWD/A does already exists. Likewise does sandbox return EEXISTS for mkdirat(A,B) if CWD/B exists (this is what happends in tars testsuite).

Why I mentioned before_syscall() is that sandbox seems to create the file on the right place becouse if mkdirat_pre_check pass then before_syscall() will run and resolvs the dirfd and creates the file in the right place.

So the way to fix this seems to be to have mkdirat_pre_check if (dirfd == AT_FDCWD) { readlink(/proc/self/fd/dirfd) (like before_syscall do) to resolv the path to dirfd/RELPATH } else { do the canonicalize to CWD/RELPATH (since AT_FDCWD == -100, and does not resolv to anything inside of /proc/self/fd/)}. Then look if the result exists.

Does this sound correct?
Comment 3 Xake 2010-10-28 08:28:57 UTC
Created attachment 252345 [details, diff]
Make mkdirat_pre_check resolv the dirfd if dirfd != AT_FDCWD

quick, dirty patch. Need some cleanup since there are some things I am uncertain about when it comes to ENAMETOOLONG and so. However this patch does not break anything in the sandbox testsuite, and it makes the tar testsuite (at least tar.git) work with sandbox.
Comment 4 Ed Tomlinson 2010-11-02 23:57:31 UTC
(In reply to comment #3)
> Created an attachment (id=252345) [details]
> Make mkdirat_pre_check resolv the dirfd if dirfd != AT_FDCWD
> 
> quick, dirty patch. Need some cleanup since there are some things I am
> uncertain about when it comes to ENAMETOOLONG and so. However this patch does
> not break anything in the sandbox testsuite, and it makes the tar testsuite (at
> least tar.git) work with sandbox.

tar-1.24-r2 still breaks emerges here
Comment 5 Samuli Suominen gentoo-dev 2010-11-03 00:17:52 UTC
(In reply to comment #4)
> tar-1.24-r2 still breaks emerges here

With, or without the patch from this bug applied (to sandbox)? The patch is not in portage yet. Try to make sense...
Comment 6 Ryan Hill (RETIRED) gentoo-dev 2010-11-03 03:08:29 UTC
He's saying tar-1.24-r2 should be masked.
Comment 7 Ed Tomlinson 2010-11-03 12:08:57 UTC
(In reply to comment #6)
> He's saying tar-1.24-r2 should be masked.

Ryan has it exactly right.  This bug (sandbox patch not applied) causes emerge to silently corrupt your system.  Items that trigger it, like tar 1.24, need to be masked other wise your system gets corrupted and emerge/portage thinks all is ok.  IMHO this is really bad.

I understand that emerge/portage depends on some tools working correctly and hense does not check some for some errors.  In this case, since tar keeps getting rereleased, it _might_ be a good idea to add a check on the tar rc to emerge/portage.  From a user's POV I would _much_ rather emerge tell me some ebuilds have failed and the install is corrupt rather than have to parse logs and manually reemerge 10-20 ebuilds each time this happens (third time here).

Thanks
Comment 8 Ed Tomlinson 2010-11-03 12:10:53 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > He's saying tar-1.24-r2 should be masked.
> 
> Ryan has it exactly right.  This bug (sandbox patch not applied) causes emerge
> to silently corrupt your system.  Items that trigger it, like tar 1.24, need to
> be masked other wise your system gets corrupted and emerge/portage thinks all
> is ok.  IMHO this is really bad.
> 
> I understand that emerge/portage depends on some tools working correctly and
> hense does not check some for some errors.  In this case, since tar keeps
> getting rereleased, it _might_ be a good idea to add a check on the tar rc to
> emerge/portage.  From a user's POV I would _much_ rather emerge tell me some
> ebuilds have failed and the install is corrupt rather than have to parse logs
> and manually reemerge 10-20 ebuilds each time this happens (third time here).
> 
> Thanks
> 

BTW this time xorg-server-1.9.2 was the one that hit the bug - a missing /usr/bin/X is not fun...
Comment 9 Ed Tomlinson 2010-11-11 13:06:32 UTC
tar-1.25 also has this problem and causes silent corruption.  Here qt-core-4.7.1 did not setup files needed by qt-sql-4.71 causing that emerge to fail.  Works with tar-1.23-r3 installed.

Please mask tar-1.25

Here all the 1.24 and 1.25 releases of tar silently corrupt my install.  This is NOT good.

Comment 10 Ryan Hill (RETIRED) gentoo-dev 2010-11-11 18:04:00 UTC
Works fine here.  Open a new bug that has some details (files missing, error message, emerge --info, etc.).
Comment 11 Ryan Hill (RETIRED) gentoo-dev 2010-11-11 18:39:10 UTC
Nevermind, confirmed on IRC it's the same issue.
Comment 12 Xake 2010-11-12 13:34:27 UTC
So where should we go from this?

Should we do this in another way or is the method in the patch acceptable? If so should we fraction out this method and make it into a function before_syscall() and sb_mkdirat_pre_check() can share?
Comment, comment, so we can fix this bug?
Comment 13 SpanKY gentoo-dev 2010-11-15 10:47:30 UTC
i'm guessing comments #4 - comments #11 are dupes of completely different bugs and have no business being here.

mkdirat() isnt the only pre-check func that mishandles dirfd.  simply grep the source to see more:
mkdirat_pre_check.c:    /* XXX: need to check pathname with dirfd */
openat_pre_check.c:             if (dirfd == AT_FDCWD || pathname[0] == '/') {
unlinkat_pre_check.c:   /* XXX: need to check pathname with dirfd */

so rather than duplicate code, ive unified things:
http://git.overlays.gentoo.org/gitweb/?p=proj/sandbox.git;a=commitdiff;h=c473c10a447a285f8c7b762f34c0650f587e1ff4

and added test cases:
http://git.overlays.gentoo.org/gitweb/?p=proj/sandbox.git;a=commitdiff;h=46b108abbbc306357d949fd4cfd91ddab9749b00
Comment 14 SpanKY gentoo-dev 2010-11-19 00:58:48 UTC
Xake: can you verify the change i committed ?
Comment 15 Xake 2010-11-20 08:59:18 UTC
(In reply to comment #14)
> Xake: can you verify the change i committed ?
> 

My preliminary testing suggests that your fix works for me.:-)

Planning on a release?