Clean up patch

Here I clean up anon's patch with a few of nickm's suggestions from comment 12:
https://trac.torproject.org/projects/tor/ticket/11291#comment:12

I did not yet completely implement all his suggestions.

6 files changed, 33 insertions, 60 deletions

diff --git a/src/common/util.c b/src/common/util.c
index 6971e4d627..0865fe7c7f 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -1914,10 +1914,9 @@ check_private_dir(const char *dirname, cpd_check_t check,
       r = mkdir(dirname);
 #else
       if (check & CPD_GROUP_READ) {
-        r = mkdir(dirname, STAT_RWXU|STAT_RGRP|STAT_XGRP);
-      }
-      else {
-        r = mkdir(dirname, STAT_RWXU);
+        r = mkdir(dirname, 0750);
+      } else {
+        r = mkdir(dirname, 0700);
       }
 #endif
       if (r) {
diff --git a/src/common/util.h b/src/common/util.h
index c98a32c19c..755ef4b82a 100644
--- a/src/common/util.h
+++ b/src/common/util.h
@@ -349,29 +349,6 @@ typedef unsigned int cpd_check_t;
 int check_private_dir(const char *dirname, cpd_check_t check,
                       const char *effective_user);
 
-#if !defined(_WIN32) && !defined (WINCE)
-/** Unix like (non win32) group and world permission constants.
- * These constants are directly compatible with *nix ABI, e.g. S_IRWXU,*
- * NOTE: these are distinct from the private directory flags CPD_*
- *        which are a portable way for storing private data cross platform.
- */
-#define STAT_RWXU 00700
-#define STAT_RUSR 00400
-#define STAT_WUSR 00200
-#define STAT_XUSR 00100
-
-#define STAT_RWXG 00070
-#define STAT_RGRP 00040
-#define STAT_WGRP 00020
-#define STAT_XGRP 00010
-
-#define STAT_RWXO 00007
-#define STAT_ROTH 00004
-#define STAT_WOTH 00002
-#define STAT_XOTH 00001
-#endif
- 
-/** File open and create options */
 #define OPEN_FLAGS_REPLACE (O_WRONLY|O_CREAT|O_TRUNC)
 #define OPEN_FLAGS_APPEND (O_WRONLY|O_CREAT|O_APPEND)
 #define OPEN_FLAGS_DONT_REPLACE (O_CREAT|O_EXCL|O_APPEND|O_WRONLY)
diff --git a/src/or/config.c b/src/or/config.c
index 1753722f95..97b3601706 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -271,7 +271,7 @@ static config_var_t option_vars_[] = {
   V(AccelDir,                    FILENAME, NULL),
   V(HashedControlPassword,       LINELIST, NULL),
   V(HidServDirectoryV2,          BOOL,     "1"),
-  V(HiddenServiceGroupReadable,  BOOL,     "0"),
+  V(HiddenServiceDirGroupReadable,  BOOL,     "0"),
   VAR("HiddenServiceDir",    LINELIST_S, RendConfigLines,    NULL),
   OBSOLETE("HiddenServiceExcludeNodes"),
   OBSOLETE("HiddenServiceNodes"),
diff --git a/src/or/or.h b/src/or/or.h
index 9207dbaa91..1544b70996 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -4225,7 +4225,7 @@ typedef struct {
   int Support022HiddenServices;
 
   /** Create the Hidden Service directories and hostname files group readable. */
-  int HiddenServiceGroupReadable;
+  int HiddenServiceDirGroupReadable;
 
 } or_options_t;
 
diff --git a/src/or/rendservice.c b/src/or/rendservice.c
index 83e6a3b82c..456b548715 100644
--- a/src/or/rendservice.c
+++ b/src/or/rendservice.c
@@ -368,12 +368,10 @@ rend_config_services(const or_options_t *options, int validate_only)
   for (line = options->RendConfigLines; line; line = line->next) {
     if (!strcasecmp(line->key, "HiddenServiceDir")) {
       if (service) { /* register the one we just finished parsing */
-        if (validate_only) {
+        if (validate_only)
           rend_service_free(service);
-        }
-        else {
+        else
           rend_add_service(service);
-        }
       }
       service = tor_malloc_zero(sizeof(rend_service_t));
       service->directory = tor_strdup(line->value);
@@ -517,8 +515,7 @@ rend_config_services(const or_options_t *options, int validate_only)
   if (service) {
     if (validate_only) {
       rend_service_free(service);
-    }
-    else {
+    } else {
       rend_add_service(service);
     }
   }
@@ -699,7 +696,7 @@ rend_service_load_keys(rend_service_t *s)
   char buf[128];
   cpd_check_t  check_opts = CPD_CREATE;
 
-  if (get_options()->HiddenServiceGroupReadable) {
+  if (get_options()->HiddenServiceDirGroupReadable) {
     check_opts |= CPD_GROUP_READ;
   }
   /* Check/create directory */
@@ -707,9 +704,9 @@ rend_service_load_keys(rend_service_t *s)
     return -1;
   }
 #ifndef _WIN32
-  if (get_options()->HiddenServiceGroupReadable) {
-    /** Only new dirs created get new opts, also enforce group read. */
-    if (chmod(s->directory, STAT_RWXU|STAT_RGRP|STAT_XGRP)) {
+  if (get_options()->HiddenServiceDirGroupReadable) {
+    /* Only new dirs created get new opts, also enforce group read. */
+    if (chmod(s->directory, 0750)) {
       log_warn(LD_FS,"Unable to make %s group-readable.", s->directory);
     }
   }
@@ -751,9 +748,9 @@ rend_service_load_keys(rend_service_t *s)
     return -1;
   }
 #ifndef _WIN32
-  if (get_options()->HiddenServiceGroupReadable) {
-    /** Also verify hostname file created with group read. */
-    if (chmod(fname, STAT_RUSR|STAT_WUSR|STAT_RGRP)) {
+  if (get_options()->HiddenServiceDirGroupReadable) {
+    /* Also verify hostname file created with group read. */
+    if (chmod(fname, 0640)) {
       log_warn(LD_FS,"Unable to make hidden hostname file %s group-readable.", fname);
     }
   }
diff --git a/src/test/test_checkdir.c b/src/test/test_checkdir.c
index 877c789bcc..76fe1315a7 100644
--- a/src/test/test_checkdir.c
+++ b/src/test/test_checkdir.c
@@ -20,46 +20,46 @@ test_checkdir_perms(void *testdata)
   cpd_check_t  unix_verify_optsmask;
   struct stat st;
 
-  /** setup data directory before tests. */
+  /* setup data directory before tests. */
   tor_free(options->DataDirectory);
   options->DataDirectory = tor_strdup(get_fname(subdir));
   tt_int_op(mkdir(options->DataDirectory, STAT_RWXU), ==, 0);
 
-  /** test: create new dir, no flags. */
+  /* test: create new dir, no flags. */
   testdir = get_datadir_fname("checkdir_new_none");
   cpd_chkopts = CPD_CREATE;
-  unix_verify_optsmask = STAT_RWXO|STAT_RWXG; /* 0077 */
+  unix_verify_optsmask = 0077;
   tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
   tt_int_op(0, ==, stat(testdir, &st));
   tt_int_op(0, ==, (st.st_mode & unix_verify_optsmask));
   tor_free(testdir);
 
-  /** test: create new dir, CPD_GROUP_OK option set. */
+  /* test: create new dir, CPD_GROUP_OK option set. */
   testdir = get_datadir_fname("checkdir_new_groupok");
   cpd_chkopts = CPD_CREATE|CPD_GROUP_OK;
-  unix_verify_optsmask = STAT_RWXO|STAT_RWXG; /* 0077 */
+  unix_verify_optsmask = 0077;
   tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
   tt_int_op(0, ==, stat(testdir, &st));
   tt_int_op(0, ==, (st.st_mode & unix_verify_optsmask));
   tor_free(testdir);
 
 
-  /** test: create new dir, CPD_GROUP_READ option set. */
+  /* test: create new dir, CPD_GROUP_READ option set. */
   testdir = get_datadir_fname("checkdir_new_groupread");
   cpd_chkopts = CPD_CREATE|CPD_GROUP_READ;
-  unix_verify_optsmask = STAT_RWXO|STAT_WGRP; /* 0027 */
+  unix_verify_optsmask = 0027;
   tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
   tt_int_op(0, ==, stat(testdir, &st));
   tt_int_op(0, ==, (st.st_mode & unix_verify_optsmask));
   tor_free(testdir);
 
 
-  /** test: check existing dir created with defaults,
+  /* test: check existing dir created with defaults,
             and verify with CPD_CREATE only. */
   testdir = get_datadir_fname("checkdir_exists_none");
   cpd_chkopts = CPD_CREATE;
-  unix_create_opts = STAT_RWXU; /* 0700 */
-  unix_verify_optsmask = STAT_RWXO|STAT_RWXG; /* 0077 */
+  unix_create_opts = 0700;
+  unix_verify_optsmask = 0077;
   tt_int_op(0, ==, mkdir(testdir, unix_create_opts));
   tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
   tt_int_op(0, ==, stat(testdir, &st));
@@ -67,11 +67,11 @@ test_checkdir_perms(void *testdata)
   tor_free(testdir);
 
 
-  /** test: check existing dir created with defaults,
+  /* test: check existing dir created with defaults,
             and verify with CPD_GROUP_OK option set. */
   testdir = get_datadir_fname("checkdir_exists_groupok");
   cpd_chkopts = CPD_CREATE;
-  unix_verify_optsmask = STAT_RWXO|STAT_RWXG; /* 0077 */
+  unix_verify_optsmask = 0077;
   tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
   cpd_chkopts = CPD_GROUP_OK;
   tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
@@ -80,11 +80,11 @@ test_checkdir_perms(void *testdata)
   tor_free(testdir);
 
 
-  /** test: check existing dir created with defaults,
+  /* test: check existing dir created with defaults,
             and verify with CPD_GROUP_READ option set. */
   testdir = get_datadir_fname("checkdir_exists_groupread");
   cpd_chkopts = CPD_CREATE;
-  unix_verify_optsmask = STAT_RWXO|STAT_WGRP; /* 0027 */
+  unix_verify_optsmask = 0027;
   tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
   cpd_chkopts = CPD_GROUP_READ;
   tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
@@ -93,11 +93,11 @@ test_checkdir_perms(void *testdata)
   tor_free(testdir);
 
 
-  /** test: check existing dir created with CPD_GROUP_READ,
+  /* test: check existing dir created with CPD_GROUP_READ,
             and verify with CPD_GROUP_OK option set. */
   testdir = get_datadir_fname("checkdir_existsread_groupok");
   cpd_chkopts = CPD_CREATE|CPD_GROUP_READ;
-  unix_verify_optsmask = STAT_RWXO|STAT_WGRP; /* 0027 */
+  unix_verify_optsmask = 0027;
   tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
   cpd_chkopts = CPD_GROUP_OK;
   tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
@@ -106,11 +106,11 @@ test_checkdir_perms(void *testdata)
   tor_free(testdir);
 
 
-  /** test: check existing dir created with CPD_GROUP_READ,
+  /* test: check existing dir created with CPD_GROUP_READ,
             and verify with CPD_GROUP_READ option set. */
   testdir = get_datadir_fname("checkdir_existsread_groupread");
   cpd_chkopts = CPD_CREATE|CPD_GROUP_READ;
-  unix_verify_optsmask = STAT_RWXO|STAT_WGRP; /* 0027 */
+  unix_verify_optsmask = 0027;
   tt_int_op(0, ==, check_private_dir(testdir, cpd_chkopts, NULL));
   tt_int_op(0, ==, stat(testdir, &st));
   tt_int_op(0, ==, (st.st_mode & unix_verify_optsmask));