Date: Mon, 8 Mar 1999 11:35:44 +0200 (IST) From: Eli Zaretskii X-Sender: eliz AT is To: "Mark E." cc: djgpp-workers AT delorie DOT com Subject: Re: chroot patches v4 In-Reply-To: <199903041811.SAA92588@out5.ibm.net> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Reply-To: djgpp-workers AT delorie DOT com X-Mailing-List: djgpp-workers AT delorie DOT com X-Unsubscribes-To: listserv AT delorie DOT com Precedence: bulk On Thu, 4 Mar 1999, Mark E. wrote: > Below are my latest chroot patches. I'm hoping they are complete and > correct. Thanks! Unfortunately, I'm afraid I need to be the Bad Guy again ;-) (Will somebody else please review the code as well, at least to share the blame, if not for other good reasons? ;-) > + else if (*path != '/' && *path != '\\' && __djgpp_root_len > 0) > + { > + /* Relative paths are relative to the root, not the cwd. */ > + char path_buf[PATH_MAX+1]; > + path_buf[0] = '/'; > + strcpy(path_buf+1, path); I don't see such interpretation in the Unix man pages. Relative file names are allowed and are interpreted as usual: relative to cwd. > + if (unix_emu) > + { > + /* If a root directory already exists, the new root directory > + must be relative to the existing root directory. > + If not, then reject the path. */ > + if (__djgpp_root_len > 0 && *buf != '/') > + { > + errno = EACCES; > + return -1; > + } Nate tried on Linux and said that the argument passed to `chroot' is always interpreted as relative to the current root. However, your implementation calls `_fixpath' too early, and thus deviates from this behavior. For example, if the current root is "c:/foo" and `chroot' is called with "/foo/bar/baz", then `chroot' will allow it (I think; I didn't actually try this) because it generates "c:/foo/bar/baz" and then removes "c:/foo" from it. But the Unix description says you should append the argument to the current root, to get "c:/foo/foo/bar/baz", and *then* see if that directory is valid. I think the problem is that you call `_fixpath' too early in some cases. How about the following logic: - if the file name begins with a slash, append to current root, then run the result through `_fixpath'; - else run the result through `_fixpath', then append to the current root; - now call `access' to see if the result exists and is a directory; - if it is a valid directory, proceed with the rest of code. The current implementation also has problems with ANSI name-space pollution: since `chroot' is called from the startup code, it is linked into every DJGPP program. Therefore, it cannot call any non-ANSI functions whose names don't begin with an underscore. The problems are with `chroot' itself, with `setenv', and with `strnicmp'. `chroot' must be stubbed (see ). I suggest to use `stricmp' instead of `strnicmp' and `putenv' instead of `setenv', since the replacements are already stubbed, and to add the necessary code for the replacements to work. Sorry I didn't catch the name-space pollution problems earlier. Did I said that others should look at this code as well? ;-) > + /* If the path is absolute and a root path is set, > + then add the root path to the output. */ > + else if (path[0] == '/' && __djgpp_root_len > 0 ) > + { > + char *root = __djgpp_root; > + for ( ; *root; root++) > + { > + _farnspokeb (o++, *root); > + space -= 1; > + } I think the "else if" clause above (this is from putpath.c) should also catch backslashes, not only forward slashes. Also, what about file names like "c:/foo" when the current root is "c:/foo/bar"? I think `_put_path2' should fail such arguments when the Unix emulation flag is set, otherwise ``chdir ("c:/foo")'' and such likes will defeat the purpose of this feature. The simplest way to do that is to prepend the current root to the argument, if the latter starts with a drive letter. The ``permissive'' mode doesn't need to enforce this, I think. > + /* Succeeds because 'c:/djgpp/bin' is relative to 'c:/djgpp'. > + Passing in '/bin' would have also worked. */ > + chroot("c:/djgpp/bin"); The current code correctly fails in this case, so the docs needs to be changed. > + @subheading Return Value > + > + Always returns zero for success, unless @var{file_handle} is less than > + zero. This is no longer true: `fchroot' always ignores the handle. > + @subheading Portability > + > + @portability !ansi, !posix I suggest to add a @portability-note to the effect that `fchroot' only allows to return to the original root x:/; you can't save away several previous values of the root (by saving the handles) and then return to some of them. > *** src/libc/crt0/c1root.c.orig Sun Feb 28 14:08:22 1999 > --- src/libc/crt0/c1root.c Thu Mar 4 12:46:52 1999 The new files, such as c1root.c and others, need to be added to the list of files in the makefile's in their respective directories, otherwise the library build procedure won't pick them up. Thanks again for working on this.