Work around sqlite's incorrect handling of umask when creating databases.

Refactored some common code into initDb.

This only deals with the problem when creating new databases. If a repo
got bad permissions into it, it's up to the user to deal with it.

This commit was sponsored by Ole-Morten Duesund on Patreon.

CHANGELOG

@@ -52,6 +52,8 @@ git-annex (6.20170102) UNRELEASED; urgency=medium
   * Improve pid locking code to work on filesystems that don't support hard
     links.
   * S3: Fix check of uuid file stored in bucket, which was not working.
+  * Work around sqlite's incorrect handling of umask when creating
+    databases.
 
  -- Joey Hess <id@joeyh.name>  Fri, 06 Jan 2017 15:22:06 -0400
 

Database/Fsck.hs

@@ -22,11 +22,10 @@ module Database.Fsck (
 
 import Database.Types
 import qualified Database.Queue as H
+import Database.Init
 import Annex.Locations
-import Utility.PosixFiles
 import Utility.Exception
 import Annex.Common
-import Annex.Perms
 import Annex.LockFile
 
 import Database.Persist.TH
@@ -61,17 +60,8 @@ openDb u = do
 	dbdir <- fromRepo (gitAnnexFsckDbDir u)
 	let db = dbdir </> "db"
 	unlessM (liftIO $ doesFileExist db) $ do
-		let tmpdbdir = dbdir ++ ".tmp"
+		initDb db $ void $
-		let tmpdb = tmpdbdir </> "db"
+			runMigrationSilent migrateFsck
-		liftIO $ do
-			createDirectoryIfMissing True tmpdbdir
-			H.initDb tmpdb $ void $
-				runMigrationSilent migrateFsck
-		setAnnexDirPerm tmpdbdir
-		setAnnexFilePerm tmpdb
-		liftIO $ do
-			void $ tryIO $ removeDirectoryRecursive dbdir
-			rename tmpdbdir dbdir
 	lockFileCached =<< fromRepo (gitAnnexFsckDbLock u)
 	h <- liftIO $ H.openDbQueue db "fscked"
 	return $ FsckHandle h u

Database/Handle.hs

@@ -9,7 +9,6 @@
 
 module Database.Handle (
 	DbHandle,
-	initDb,
 	openDb,
 	TableName,
 	queryDb,
@@ -38,26 +37,6 @@ import System.IO
  - the database. It has a MVar which Jobs are submitted to. -}
 data DbHandle = DbHandle (Async ()) (MVar Job)
 
-{- Ensures that the database is initialized. Pass the migration action for
- - the database.
- -
- - The database is initialized using WAL mode, to prevent readers
- - from blocking writers, and prevent a writer from blocking readers.
- -}
-initDb :: FilePath -> SqlPersistM () -> IO ()
-initDb f migration = do
-	let db = T.pack f
-	enableWAL db
-	runSqlite db migration
-
-enableWAL :: T.Text -> IO ()
-enableWAL db = do
-	conn <- Sqlite.open db
-	stmt <- Sqlite.prepare conn (T.pack "PRAGMA journal_mode=WAL;")
-	void $ Sqlite.step stmt
-	void $ Sqlite.finalize stmt
-	Sqlite.close conn
-
 {- Name of a table that should exist once the database is initialized. -}
 type TableName = String
 

Database/Init.hs

@@ -0,0 +1,55 @@
+{- Persistent sqlite database initialization
+ -
+ - Copyright 2015-2017 Joey Hess <id@joeyh.name>
+ -
+ - Licensed under the GNU GPL version 3 or higher.
+ -}
+
+module Database.Init where
+
+import Annex.Common
+import Annex.Perms
+import Utility.FileMode
+
+import Database.Persist.Sqlite
+import qualified Database.Sqlite as Sqlite
+import Control.Monad.IO.Class (liftIO)
+import qualified Data.Text as T
+
+{- Ensures that the database is freshly initialized. Deletes any
+ - existing database. Pass the migration action for the database.
+ -
+ - The database is initialized using WAL mode, to prevent readers
+ - from blocking writers, and prevent a writer from blocking readers.
+ -
+ - The permissions of the database are set based on the
+ - core.sharedRepository setting. Setting these permissions on the main db
+ - file causes Sqlite to always use the same permissions for additional
+ - files it writes later on
+ -}
+initDb :: FilePath -> SqlPersistM () -> Annex ()
+initDb db migration = do
+	let dbdir = takeDirectory db
+	let tmpdbdir = dbdir ++ ".tmp"
+	let tmpdb = tmpdbdir </> "db"
+	liftIO $ do
+		createDirectoryIfMissing True tmpdbdir
+		let tdb = T.pack tmpdb
+		enableWAL tdb
+		runSqlite tdb migration
+	setAnnexDirPerm tmpdbdir
+	-- Work around sqlite bug that prevents it from honoring
+	-- less restrictive umasks.
+	liftIO $ setFileMode tmpdb =<< defaultFileMode
+	setAnnexFilePerm tmpdb
+	liftIO $ do
+		void $ tryIO $ removeDirectoryRecursive dbdir
+		rename tmpdbdir dbdir
+
+enableWAL :: T.Text -> IO ()
+enableWAL db = do
+	conn <- Sqlite.open db
+	stmt <- Sqlite.prepare conn (T.pack "PRAGMA journal_mode=WAL;")
+	void $ Sqlite.step stmt
+	void $ Sqlite.finalize stmt
+	Sqlite.close conn

Database/Keys.hs

@@ -25,11 +25,11 @@ import qualified Database.Keys.SQL as SQL
 import Database.Types
 import Database.Keys.Handle
 import qualified Database.Queue as H
+import Database.Init
 import Annex.Locations
 import Annex.Common hiding (delete)
 import Annex.Version (versionUsesKeysDatabase)
 import qualified Annex
-import Annex.Perms
 import Annex.LockFile
 import Utility.InodeCache
 import Annex.InodeSentinal
@@ -120,11 +120,7 @@ openDb createdb _ = catchPermissionDenied permerr $ withExclusiveLock gitAnnexKe
 	case (dbexists, createdb) of
 		(True, _) -> open db
 		(False, True) -> do
-			liftIO $ do
+			initDb db SQL.createTables
-				createDirectoryIfMissing True dbdir
-				H.initDb db SQL.createTables
-			setAnnexDirPerm dbdir
-			setAnnexFilePerm db
 			open db
 		(False, False) -> return DbUnavailable
   where

Database/Queue.hs

@@ -9,7 +9,6 @@
 
 module Database.Queue (
 	DbQueue,
-	initDb,
 	openDbQueue,
 	queryDbQueue,
 	closeDbQueue,

Utility/FileMode.hs

@@ -1,6 +1,6 @@
 {- File mode utilities.
  -
- - Copyright 2010-2012 Joey Hess <id@joeyh.name>
+ - Copyright 2010-2017 Joey Hess <id@joeyh.name>
  -
  - License: BSD-2-clause
  -}
@@ -130,6 +130,21 @@ withUmask umask a = bracket setup cleanup go
 withUmask _ a = a
 #endif
 
+getUmask :: IO FileMode
+#ifndef mingw32_HOST_OS
+getUmask = bracket setup cleanup return
+  where
+	setup = setFileCreationMask nullFileMode
+	cleanup = setFileCreationMask
+#else
+getUmask = return nullFileMode
+#endif
+
+defaultFileMode :: IO FileMode
+defaultFileMode = do
+	umask <- getUmask
+	return $ intersectFileModes (complement umask) stdFileMode
+
 combineModes :: [FileMode] -> FileMode
 combineModes [] = 0
 combineModes [m] = m

doc/bugs/Aborts_with_SQLite_error_when_dropping_contents.mdwn

@@ -81,3 +81,6 @@ extra copies.
 In other words, Git-Annex and I are very happy together, and I'd like to 
 marry it. And because you are the father, I hereby respectfully ask for 
 your blessing.
+
+> [[fixed|done]] (and I suppose you have my blessing, but I'm not sure
+> that's legal yet!) --[[Joey]]

doc/bugs/Aborts_with_SQLite_error_when_dropping_contents/comment_3_88a09558f2da0a5733aa3dd042f9d59b._comment

@@ -0,0 +1,53 @@
+[[!comment format=mdwn
+ username="joey"
+ subject="""comment 3"""
+ date="2017-02-13T20:21:09Z"
+ content="""
+Thanks for following up with the cause of this.
+
+In fact, assuming you're not using a v6 git-annex repository, it doesn't
+really need to update that database at all. But since we'll be upgrading to
+v6 eventually, I need to deal with problems like this. Also,
+this same problem will also impact the database used for incremental fsck.
+
+I can reproduce this with a v5 repository; dropping a file happens to run a
+code path that updates the database. And reproducing it w/o using git-annex too:
+
+	joey@darkstar:~/tmp>mkdir empty
+	joey@darkstar:~/tmp>umask
+	0002
+	joey@darkstar:~/tmp>touch empty/file
+	joey@darkstar:~/tmp>sqlite3 empty/db
+	SQLite version 3.16.2 2017-01-06 16:32:41
+	Enter ".help" for usage hints.
+	sqlite> create table foo;
+	Error: near ";": syntax error
+	sqlite> 
+	joey@darkstar:~/tmp>ls -l empty/
+	total 0
+	-rw-r--r-- 1 joey joey 0 Feb 13 16:33 db
+	-rw-rw-r-- 1 joey joey 0 Feb 13 16:32 file
+
+Seems that sqlite uses `0644 & umask` for the db permissions, 
+which is *bad* because it doesn't allow the umask to enable the group
+write bit. That 0644 is `SQLITE_DEFAULT_FILE_PERMISSIONS`, so it can
+be changed to something saner at compile time.
+
+`http://www.sqlite.org/src/doc/trunk/src/os_unix.c` has a useful comment.
+Seems that sqlite is careful to make -wal, -journal, and -shm files
+have the exact same permissions as main database file.
+
+So, if `.git/annex/keys/*` is updated to have the desired permissions when
+the database is created, every further write to the database will keep
+using the desired permissions.
+
+Hmm, it turns out that git-annex does already set the database permissions when
+creating it, but only if core.sharedRepository is set to group or all. So
+there's a workaround; just `git config core.sharedRepository group` when
+setting up a repository that's going to be accessed by multiple users. Almost
+certianly a better idea than relying on umask anyway; people mess up umask
+settings.
+
+I'll go ahead and make it always set sane permissions when creating databases.
+I'm not going to try to fix up permissions in existing repositories though.
+"""]]

git-annex.cabal

@@ -798,6 +798,7 @@ Executable git-annex
     Crypto
     Database.Fsck
     Database.Handle
+    Database.Init
     Database.Keys
     Database.Keys.Handle
     Database.Keys.SQL