umask(0) in all binaries
Christof Schmitt
cs at samba.org
Mon Nov 4 16:54:31 MST 2013
On Mon, Nov 04, 2013 at 03:34:24PM -0800, Jeremy Allison wrote:
> On Mon, Nov 04, 2013 at 02:26:13PM -0700, Christof Schmitt wrote:
> > On Mon, Nov 04, 2013 at 12:25:29PM -0800, Jeremy Allison wrote:
> > > On Mon, Nov 04, 2013 at 12:54:23PM -0700, Christof Schmitt wrote:
> > > > I have been chasing an issue where the permissions on /var/ctdb/samba
> > > > are set to 0700 instead of 0755. Commit 59462f2 "winbindd and nmbd don't
> > > > set their umask to zero on startup like smbd does." fixed this in case
> > > > the directory is created by winbindd or nmbd. Other binaries from
> > > > source3/utils will also call into cache_path() that will create the
> > > > directory through xx_path() in case it does not exist. Depending on the
> > > > umask of the caller, the permissions will be different than expected.
> > > >
> > > > What is the best way to fix this? Call umask(0) from every main function
> > > > to ensure that we use the intended permissions?
> > >
> > > As the utilities are designed to act on databases created by
> > > the running daemons, I don't think the utilities have any
> > > business creating those directories.
> > >
> > > Maybe add a function varient that is only used by the utility
> > > functions (e.g. xx_path_client()) which uses a flags to a xx_path_internal() function
> > > that will never create the directory. The long running daemons
> > > call xx_path(), which then calls xx_path_internal() setting
> > > the "create" flag.
> >
> > Looking at the code, it will be difficult in some places to decide if
> > the call has been made froma daemon or from a utility, e.g.
> > db_open_ctdb -> g_lock_ctx_init -> lock_path
>
> Ah, ok. I hadn't considered that.
>
> > If we decide that only smbd, winbindd and nmbd should create those
> > directories in case they are missing: Wouldn't it be simpler to create
> > the directories directly during startup of those daemons, and remove the
> > if (!directory_exist(fname)) from xx_path?
>
> Yes, I would support that change.
Ok, here is my attempt on making that change. The first patch ensures
that the directories are only created during daemon startup, and the
second one adds checks against NULL pointers in codepaths that can be
reached from utility code.
Christof
-------------- next part --------------
>From 3166d6cf0185b3bca454cfa1a81cfcbd83f5ba6d Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Mon, 4 Nov 2013 15:37:40 -0700
Subject: [PATCH 1/2] Create LOCKDIR, STATEDIR and CACHEDIR on daemon startup
Add an explicit call to check and create LOCKDIR, STATEDIR and CACHEDIR
on startup of the smbd, winbindd and nmbd daemons. Also remove the
implicit creation of those directories from xx_path.
This fixes an issue where one of the directories does not exist, and a
call to a utility creates it, but with a wrong umask. The daemon
processes are already setup to use the correct umask, so the best way is
to defer the directory creation to those processes.
Signed-off-by: Christof Schmitt <cs at samba.org>
---
source3/include/proto.h | 1 +
source3/lib/util.c | 58 +++++++++++++++++++++++++++++++++++++------
source3/nmbd/nmbd.c | 5 +---
source3/smbd/server.c | 5 ++-
source3/winbindd/winbindd.c | 5 +---
5 files changed, 56 insertions(+), 18 deletions(-)
diff --git a/source3/include/proto.h b/source3/include/proto.h
index 277547b..47f9625 100644
--- a/source3/include/proto.h
+++ b/source3/include/proto.h
@@ -429,6 +429,7 @@ char *myhostname_upper(void);
char *lock_path(const char *name);
char *state_path(const char *name);
char *cache_path(const char *name);
+bool check_create_directories();
bool parent_dirname(TALLOC_CTX *mem_ctx, const char *dir, char **parent,
const char **name);
bool ms_has_wild(const char *s);
diff --git a/source3/lib/util.c b/source3/lib/util.c
index 5168092..3eab881 100644
--- a/source3/lib/util.c
+++ b/source3/lib/util.c
@@ -1494,14 +1494,7 @@ static char *xx_path(const char *name, const char *rootpath)
trim_string(fname,"","/");
if (!directory_exist(fname)) {
- if (mkdir(fname,0755) == -1) {
- /* Did someone else win the race ? */
- if (errno != EEXIST) {
- DEBUG(1, ("Unable to create directory %s for file %s. "
- "Error was %s\n", fname, name, strerror(errno)));
- return NULL;
- }
- }
+ return NULL;
}
return talloc_asprintf_append(fname, "/%s", name);
@@ -1546,6 +1539,55 @@ char *cache_path(const char *name)
return xx_path(name, lp_cachedir());
}
+static bool check_create_directory(const char *name)
+{
+ int ret;
+
+ if (xx_path("", name)) {
+ return true;
+ }
+
+ ret = mkdir(name,0755);
+ if (ret == -1) {
+ /* Did someone else win the race ? */
+ if (errno != EEXIST) {
+ DEBUG(1, ("Unable to create directory %s, error %s\n",
+ name, strerror(errno)));
+ return false;
+ }
+ }
+
+ return true;
+}
+
+/**
+ * @brief Verify that LOCKDIR, STATEDIR and CACHEDIR exist or try to create them
+ *
+ * @retval True if LOCKDIR, STATEDIR and CACHEDIR exist or could be created.
+ **/
+
+bool check_create_directories()
+{
+ bool b;
+
+ b = check_create_directory(lp_lockdir());
+ if (b == false) {
+ return false;
+ }
+
+ b = check_create_directory(lp_statedir());
+ if (b == false) {
+ return false;
+ }
+
+ b = check_create_directory(lp_cachedir());
+ if (b == false) {
+ return false;
+ }
+
+ return true;
+}
+
/*******************************************************************
Given a filename - get its directory name
********************************************************************/
diff --git a/source3/nmbd/nmbd.c b/source3/nmbd/nmbd.c
index f31de08..d1cec91 100644
--- a/source3/nmbd/nmbd.c
+++ b/source3/nmbd/nmbd.c
@@ -969,10 +969,7 @@ static bool open_sockets(bool isdaemon, int port)
}
#endif
- ok = directory_create_or_exist(lp_lockdir(), geteuid(), 0755);
- if (!ok) {
- DEBUG(0, ("Failed to create directory %s for lock files - %s\n",
- lp_lockdir(), strerror(errno)));
+ if (check_create_directories() == false) {
exit(1);
}
diff --git a/source3/smbd/server.c b/source3/smbd/server.c
index 36be019..5c30195 100644
--- a/source3/smbd/server.c
+++ b/source3/smbd/server.c
@@ -1308,8 +1308,9 @@ extern void build_options(bool screen);
setpgid( (pid_t)0, (pid_t)0);
#endif
- if (!directory_exist(lp_lockdir()))
- mkdir(lp_lockdir(), 0755);
+ if (check_create_directories() == false) {
+ exit(1);
+ }
if (!directory_exist(lp_piddir()))
mkdir(lp_piddir(), 0755);
diff --git a/source3/winbindd/winbindd.c b/source3/winbindd/winbindd.c
index 361d02c..e32eeee 100644
--- a/source3/winbindd/winbindd.c
+++ b/source3/winbindd/winbindd.c
@@ -1480,10 +1480,7 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
- ok = directory_create_or_exist(lp_lockdir(), geteuid(), 0755);
- if (!ok) {
- DEBUG(0, ("Failed to create directory %s for lock files - %s\n",
- lp_lockdir(), strerror(errno)));
+ if (check_create_directories() == false) {
exit(1);
}
--
1.7.1
-------------- next part --------------
>From 0b8ac9ec914936dee0233c9a3f5245b0d53411f2 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Mon, 4 Nov 2013 15:40:34 -0700
Subject: [PATCH 2/2] Add checks for {lock,cache,state}_path calls from utilities
Those calls already can return NULL when a directory does not exist and
it is not possible to create them. Add checks to the codepaths that can
be called from utilities to avoid unnecessary segfaults. The daemons
already ensure on startup that those directories exist.
Signed-off-by: Christof Schmitt <cs at samba.org>
---
source3/lib/dbwrap/dbwrap_watch.c | 10 ++++++++--
source3/lib/g_lock.c | 8 +++++++-
source3/lib/gencache.c | 6 ++++++
source3/lib/serverid.c | 9 ++++++++-
source3/lib/sharesec.c | 8 +++++++-
source3/utils/smbcontrol.c | 17 ++++++++++++++---
source3/utils/status.c | 17 +++++++++++++----
7 files changed, 63 insertions(+), 12 deletions(-)
diff --git a/source3/lib/dbwrap/dbwrap_watch.c b/source3/lib/dbwrap/dbwrap_watch.c
index 7bdcd99..34bfec5 100644
--- a/source3/lib/dbwrap/dbwrap_watch.c
+++ b/source3/lib/dbwrap/dbwrap_watch.c
@@ -31,8 +31,14 @@ static struct db_context *dbwrap_record_watchers_db(void)
static struct db_context *watchers_db;
if (watchers_db == NULL) {
- watchers_db = db_open(
- NULL, lock_path("dbwrap_watchers.tdb"), 0,
+ const char *path;
+
+ path = lock_path("dbwrap_watchers.tdb");
+ if (path == NULL) {
+ return NULL;
+ }
+
+ watchers_db = db_open(NULL, path, 0,
TDB_CLEAR_IF_FIRST | TDB_INCOMPATIBLE_HASH,
O_RDWR|O_CREAT, 0600, DBWRAP_LOCK_ORDER_3);
}
diff --git a/source3/lib/g_lock.c b/source3/lib/g_lock.c
index 8c7a6c2..3a795c1 100644
--- a/source3/lib/g_lock.c
+++ b/source3/lib/g_lock.c
@@ -51,6 +51,7 @@ struct g_lock_ctx *g_lock_ctx_init(TALLOC_CTX *mem_ctx,
struct messaging_context *msg)
{
struct g_lock_ctx *result;
+ const char *path;
result = talloc(mem_ctx, struct g_lock_ctx);
if (result == NULL) {
@@ -58,7 +59,12 @@ struct g_lock_ctx *g_lock_ctx_init(TALLOC_CTX *mem_ctx,
}
result->msg = msg;
- result->db = db_open(result, lock_path("g_lock.tdb"), 0,
+ path = lock_path("g_lock.tdb");
+ if (path == NULL) {
+ return NULL;
+ }
+
+ result->db = db_open(result, path, 0,
TDB_CLEAR_IF_FIRST|TDB_INCOMPATIBLE_HASH,
O_RDWR|O_CREAT, 0600,
DBWRAP_LOCK_ORDER_2);
diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index 835f5e1..f4ea9dc 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -63,6 +63,9 @@ static bool gencache_init(void)
if (cache) return True;
cache_fname = cache_path("gencache.tdb");
+ if (cache_fname == NULL) {
+ return False;
+ }
DEBUG(5, ("Opening cache file at %s\n", cache_fname));
@@ -106,6 +109,9 @@ static bool gencache_init(void)
}
cache_fname = lock_path("gencache_notrans.tdb");
+ if (cache_fname == NULL) {
+ return False;
+ }
DEBUG(5, ("Opening cache file at %s\n", cache_fname));
diff --git a/source3/lib/serverid.c b/source3/lib/serverid.c
index cb49520..6074c39 100644
--- a/source3/lib/serverid.c
+++ b/source3/lib/serverid.c
@@ -71,11 +71,18 @@ bool serverid_parent_init(TALLOC_CTX *mem_ctx)
static struct db_context *serverid_db(void)
{
static struct db_context *db;
+ const char *path;
if (db != NULL) {
return db;
}
- db = db_open(NULL, lock_path("serverid.tdb"), 0,
+
+ path = lock_path("serverid.tdb");
+ if (path == NULL) {
+ return NULL;
+ }
+
+ db = db_open(NULL, path, 0,
TDB_DEFAULT|TDB_CLEAR_IF_FIRST|TDB_INCOMPATIBLE_HASH,
O_RDWR|O_CREAT, 0644, DBWRAP_LOCK_ORDER_2);
return db;
diff --git a/source3/lib/sharesec.c b/source3/lib/sharesec.c
index c7a8e51..4cd7431 100644
--- a/source3/lib/sharesec.c
+++ b/source3/lib/sharesec.c
@@ -142,12 +142,18 @@ bool share_info_db_init(void)
int32 vers_id = 0;
bool upgrade_ok = true;
NTSTATUS status;
+ const char *path;
if (share_db != NULL) {
return True;
}
- share_db = db_open(NULL, state_path("share_info.tdb"), 0,
+ path = state_path("share_info.tdb");
+ if (!path) {
+ return False;
+ }
+
+ share_db = db_open(NULL, path, 0,
TDB_DEFAULT, O_RDWR|O_CREAT, 0600,
DBWRAP_LOCK_ORDER_1);
if (share_db == NULL) {
diff --git a/source3/utils/smbcontrol.c b/source3/utils/smbcontrol.c
index 00b23f7..e80b7ab 100644
--- a/source3/utils/smbcontrol.c
+++ b/source3/utils/smbcontrol.c
@@ -938,6 +938,7 @@ static bool do_winbind_online(struct tevent_context *ev_ctx,
const int argc, const char **argv)
{
TDB_CONTEXT *tdb;
+ const char *path;
if (argc != 1) {
fprintf(stderr, "Usage: smbcontrol winbindd online\n");
@@ -947,10 +948,14 @@ static bool do_winbind_online(struct tevent_context *ev_ctx,
/* Remove the entry in the winbindd_cache tdb to tell a later
starting winbindd that we're online. */
- tdb = tdb_open_log(state_path("winbindd_cache.tdb"), 0, TDB_DEFAULT, O_RDWR, 0600);
+ path = state_path("winbindd_cache.tdb");
+ if (path == NULL) {
+ return False;
+ }
+
+ tdb = tdb_open_log(path, 0, TDB_DEFAULT, O_RDWR, 0600);
if (!tdb) {
- fprintf(stderr, "Cannot open the tdb %s for writing.\n",
- state_path("winbindd_cache.tdb"));
+ fprintf(stderr, "Cannot open the tdb %s for writing.\n", path);
return False;
}
@@ -968,6 +973,7 @@ static bool do_winbind_offline(struct tevent_context *ev_ctx,
TDB_CONTEXT *tdb;
bool ret = False;
int retry = 0;
+ const char *path;
if (argc != 1) {
fprintf(stderr, "Usage: smbcontrol winbindd offline\n");
@@ -978,6 +984,11 @@ static bool do_winbind_offline(struct tevent_context *ev_ctx,
starting winbindd that we're offline. We may actually create
it here... */
+ path = state_path("winbindd_cache.tdb");
+ if (path == NULL) {
+ return False;
+ }
+
tdb = tdb_open_log(state_path("winbindd_cache.tdb"),
WINBINDD_CACHE_TDB_DEFAULT_HASH_SIZE,
TDB_DEFAULT|TDB_INCOMPATIBLE_HASH /* TDB_CLEAR_IF_FIRST */,
diff --git a/source3/utils/status.c b/source3/utils/status.c
index be7c52f..f5a431f 100644
--- a/source3/utils/status.c
+++ b/source3/utils/status.c
@@ -363,6 +363,7 @@ static void print_notify_recs(const char *path,
TALLOC_CTX *frame = talloc_stackframe();
int ret = 0;
struct messaging_context *msg_ctx;
+ const char *conn_path, *locking_path;
sec_init();
load_case_tables();
@@ -375,6 +376,15 @@ static void print_notify_recs(const char *path,
goto done;
}
+ conn_path = lock_path("connections.tdb");
+ locking_path = lock_path("locking.tdb");
+
+ if (conn_path == NULL || lock_path == NULL) {
+ d_printf("Lock directory does not exist\n");
+ ret = 1;
+ goto done;
+ }
+
pc = poptGetContext(NULL, argc, (const char **) argv, long_options,
POPT_CONTEXT_KEEP_FIRST);
@@ -484,7 +494,7 @@ static void print_notify_recs(const char *path,
if ( show_shares ) {
if (verbose) {
- d_printf("Opened %s\n", lock_path("connections.tdb"));
+ d_printf("Opened %s\n", conn_path);
}
if (brief) {
@@ -506,13 +516,12 @@ static void print_notify_recs(const char *path,
if ( show_locks ) {
int result;
struct db_context *db;
- db = db_open(NULL, lock_path("locking.tdb"), 0,
+ db = db_open(NULL, locking_path, 0,
TDB_CLEAR_IF_FIRST|TDB_INCOMPATIBLE_HASH, O_RDONLY, 0,
DBWRAP_LOCK_ORDER_1);
if (!db) {
- d_printf("%s not initialised\n",
- lock_path("locking.tdb"));
+ d_printf("%s not initialised\n", locking_path);
d_printf("This is normal if an SMB client has never "
"connected to your server.\n");
exit(0);
--
1.7.1
More information about the samba-technical
mailing list