On 02/09/09 13:52, Stephen Smalley wrote:
> On Wed, 2009-09-02 at 13:24 +0100, Martin Orr wrote:
>> The reason I named the function that way was because the function with
>> _realpath expects a real path as an argument, but if this is confusing or
>> goes against a convention used elsewhere then it could be changed.
>
> Except that symlink_realpath() internally calls realpath(). I think it
> makes more sense to use process_one_realpath() for the function that
> invokes realpath(), and process_one() for the function that merely acts
> on its argument.
>
>> I copied the symlink_realpath code from what used to be in match. I assumed
>> that it needed a buffer on the stack because it then concatenates the last
>> component of the path on to that buffer, and realpath might not allocate a
>> long enough buffer.
>
> That's fair.
>
>>>> Not sure it is worth warning about the broken symlink case, and that
>>>> will trigger warnings for your existing users in the
>>>> restorecon /dev/stdin case, right?
>> I agree, it is not worth warning about broken symlinks, except maybe if -v
>> is specified.
>
> If the behavior of restorecon on symlinks is to always relabel the
> symlink and then if the target exists, relabel it as well, then it isn't
> truly an error if the target doesn't exist. It would be a bit clearer
> if we used an explicit flag like -h, as existing utilities like chown
> and chcon do, when we want to act on symlinks rather than their targets,
> but that would break existing usage it seems.
>
> Modified patch below. Seem reasonable?
Looks fine to me.
> diff --git a/policycoreutils/setfiles/setfiles.c b/policycoreutils/setfiles/setfiles.c
> index 4c47f21..313767a 100644
> --- a/policycoreutils/setfiles/setfiles.c
> +++ b/policycoreutils/setfiles/setfiles.c
> @@ -545,7 +545,47 @@ int canoncon(char **contextp)
> return rc;
> }
>
> -static int process_one(char *name)
> +static int symlink_realpath(char *name, char *path)
> +{
> + char *p = NULL, *file_sep;
> + char *tmp_path = strdupa(name);
> + size_t len = 0;
> +
> + if (!tmp_path) {
> + fprintf(stderr, "strdupa on %s failed: %s\n", name,
> + strerror(errno));
> + return -1;
> + }
> + file_sep = strrchr(tmp_path, '/');
> + if (file_sep == tmp_path) {
> + file_sep++;
> + p = strcpy(path, "");
> + } else if (file_sep) {
> + *file_sep = 0;
> + file_sep++;
> + p = realpath(tmp_path, path);
> + } else {
> + file_sep = tmp_path;
> + p = realpath("./", path);
> + }
> + if (p)
> + len = strlen(p);
> + if (!p || len + strlen(file_sep) + 2 > PATH_MAX) {
> + fprintf(stderr, "symlink_realpath(%s) failed %s\n", name,
> + strerror(errno));
> + return -1;
> + }
> + p += len;
> + /* ensure trailing slash of directory name */
> + if (len == 0 || *(p - 1) != '/') {
> + *p = '/';
> + p++;
> + }
> + strcpy(p, file_sep);
> + return 0;
> +}
> +
> +static int process_one(char *name, int recurse_this_path)
> {
> int rc = 0;
> const char *namelist[2];
> @@ -553,18 +593,6 @@ static int process_one(char *name)
> FTS *fts_handle;
> FTSENT *ftsent;
>
> - if (expand_realpath) {
> - char *p;
> - p = realpath(name, NULL);
> - if (!p) {
> - fprintf(stderr, "realpath(%s) failed %s\n", name,
> - strerror(errno));
> - return -1;
> - }
> - name = p;
> - }
> -
> -
> if (!strcmp(name, "/"))
> mass_relabel = 1;
>
> @@ -604,7 +632,7 @@ static int process_one(char *name)
> fts_set(fts_handle, ftsent, FTS_SKIP);
> if (rc == ERR)
> goto err;
> - if (!recurse)
> + if (!recurse_this_path)
> break;
> } while ((ftsent = fts_read(fts_handle)) != NULL);
>
> @@ -619,8 +647,6 @@ out:
> }
> if (fts_handle)
> fts_close(fts_handle);
> - if (expand_realpath)
> - free(name);
> return rc;
>
> err:
> @@ -630,6 +656,52 @@ err:
> goto out;
> }
>
> +static int process_one_realpath(char *name)
> +{
> + int rc = 0;
> + char *p;
> + struct stat sb;
> +
> + if (!expand_realpath) {
> + return process_one(name, recurse);
> + } else {
> + rc = lstat(name, &sb);
> + if (rc < 0) {
> + fprintf(stderr, "%s: lstat(%s) failed: %s\n",
> + progname, name, strerror(errno));
> + return -1;
> + }
> +
> + if (S_ISLNK(sb.st_mode)) {
> + char path[PATH_MAX + 1];
> +
> + rc = symlink_realpath(name, path);
> + if (rc < 0)
> + return rc;
> + rc = process_one(path, 0);
> + if (rc < 0)
> + return rc;
> +
> + p = realpath(name, NULL);
> + if (p) {
> + rc = process_one(p, recurse);
> + free(p);
> + }
> + return rc;
> + } else {
> + p = realpath(name, NULL);
> + if (!p) {
> + fprintf(stderr, "realpath(%s) failed %s\n", name,
> + strerror(errno));
> + return -1;
> + }
> + rc = process_one(p, recurse);
> + free(p);
> + return rc;
> + }
> + }
> +}
> +
> #ifndef USE_AUDIT
> static void maybe_audit_mass_relabel(void)
> {
> @@ -987,13 +1059,13 @@ int main(int argc, char **argv)
> delim = (null_terminated != 0) ? '\0' : '\n';
> while ((len = getdelim(&buf, &buf_len, delim, f)) > 0) {
> buf[len - 1] = 0;
> - errors |= process_one(buf);
> + errors |= process_one_realpath(buf);
> }
> if (strcmp(input_filename, "-") != 0)
> fclose(f);
> } else {
> for (i = optind; i < argc; i++) {
> - errors |= process_one(argv[i]);
> + errors |= process_one_realpath(argv[i]);
> }
> }
>
>
--
Martin Orr
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
