Fixed bug CORE-1468 : Database corruption possible when database file extension and read\write activity is performed simultaneously

src/jrd/jrd.cpp

@@ -6458,6 +6458,8 @@ static void shutdown_database(Database* dbb, const bool release_pools)
 	}
 
 	if (dbb->dbb_flags & DBB_lck_init_done) {
+		dbb->dbb_page_manager.releaseLocks();
+
 		LCK_fini(tdbb, LCK_OWNER_database);	// For the database
 		dbb->dbb_flags &= ~DBB_lck_init_done;
 	}

src/jrd/os/pio.h

@@ -82,9 +82,31 @@ class jrd_file : public pool_alloc_rpt<SCHAR, type_fil>
 const int MAX_FILE_IO	= 32;			/* Maximum "allocated" overlapped I/O events */
 #endif
 
+class thread_db;
+class GlobalRWLock;
+
+class FileExtendLock 
+{
+public:
+	FileExtendLock(Firebird::MemoryPool& p, size_t lock_len, UCHAR* lock_string);
+	~FileExtendLock();
+
+	void lock(thread_db* tdbb, bool exclusive);
+	void release(thread_db* tdbb, bool exclusive);
+	
+private:
+	GlobalRWLock* m_lock;
+};
+
+
 class jrd_file : public pool_alloc_rpt<SCHAR, type_fil>
 {
     public:
+
+	~jrd_file() {
+		delete fil_ext_lock;
+	}
+
 	jrd_file*	fil_next;		/* Next file in database */
 	ULONG fil_min_page;			/* Minimum page number in file */
 	ULONG fil_max_page;			/* Maximum page number in file */
@@ -93,6 +115,7 @@ class jrd_file : public pool_alloc_rpt<SCHAR, type_fil>
 	HANDLE fil_desc;			// File descriptor
 	//int *fil_trace;				/* Trace file, if any */
 	Firebird::Mutex fil_mutex;
+	FileExtendLock* fil_ext_lock;	// file extend lock
 #ifdef SUPERSERVER_V2
 	void* fil_io_events[MAX_FILE_IO];	/* Overlapped I/O events */
 #endif

src/jrd/os/win32/winnt.cpp

@@ -50,10 +50,92 @@
 #include "../jrd/lck_proto.h"
 #include "../jrd/mov_proto.h"
 #include "../jrd/os/pio_proto.h"
-#include "../jrd/thd.h"
+#include "../jrd/GlobalRWLock.h"
+#include "../jrd/thread_proto.h"
 
 #include <windows.h>
 
+namespace Jrd {
+
+
+FileExtendLock::FileExtendLock(Firebird::MemoryPool& p, size_t lock_len, UCHAR* lock_string)
+{
+	thread_db* tdbb = NULL;
+	SET_TDBB(tdbb);
+
+	// hvlad: logical lock owner better would be a thread, but this is not
+	// implemented currently. Fortunately only place when we need an exclusive
+	// lock is PIO_extend and it always called with existing attachment
+	m_lock = FB_NEW(p) GlobalRWLock(tdbb, p, LCK_file_extend, 
+		lock_len, lock_string, 
+		LCK_OWNER_database, LCK_OWNER_attachment, true);
+}
+
+FileExtendLock::~FileExtendLock()
+{
+	delete m_lock;
+}
+
+void FileExtendLock::lock(thread_db* tdbb, bool exclusive)
+{
+	const bool in_engine = SCH_thread_enter_check();
+	if (!in_engine)
+		THREAD_ENTER();
+
+	SET_TDBB(tdbb);
+	fb_assert(tdbb->tdbb_attachment);
+
+	m_lock->lock(tdbb, exclusive ? LCK_write : LCK_read, LCK_WAIT);
+
+	if (!in_engine)
+		THREAD_EXIT();
+}
+
+void FileExtendLock::release(thread_db* tdbb, bool exclusive)
+{
+	const bool in_engine = SCH_thread_enter_check();
+	if (!in_engine)
+		THREAD_ENTER();
+
+	SET_TDBB(tdbb);
+	fb_assert(tdbb->tdbb_attachment);
+
+	m_lock->unlock(tdbb, exclusive ? LCK_write : LCK_read);
+
+	if (!in_engine)
+		THREAD_EXIT();
+}
+
+
+class FileExtendLockGuard
+{
+public:
+	FileExtendLockGuard(thread_db* tdbb, FileExtendLock *lock, bool exclusive) :
+	  m_tdbb(tdbb), m_lock(lock), m_exclusive(exclusive) 
+	{
+		if (exclusive) {
+			fb_assert(m_lock);
+		}
+		if (m_lock) {
+			m_lock->lock(m_tdbb, m_exclusive);
+		}
+	}
+
+	~FileExtendLockGuard()
+	{
+		if (m_lock) {
+			m_lock->release(m_tdbb, m_exclusive);
+		}
+	}
+
+private:
+	thread_db*		m_tdbb;
+	FileExtendLock*	m_lock;
+	bool			m_exclusive;
+};
+
+
+} // namespace Jrd
 
 using namespace Jrd;
 
@@ -257,10 +339,20 @@ void PIO_extend(jrd_file* main_file, const ULONG extPages, const USHORT pageSize
 	const DWORD INVALID_SET_FILE_POINTER = 0xFFFFFFFF;
 #endif
 
+	// hvlad: prevent other threads from read\write changing file pointer
+	// It solved issue CORE-1468 (database file corruption when file extension 
+	// and read\write activity performed simultaneously)
+	// It looks like a Windows bug as all our ReadFile\WriteFile calls used
+	// overlapped to set read\write operation offset not touching file pointer
+	if (!main_file->fil_ext_lock)
+		return;
+
+	FileExtendLockGuard extLock(NULL, main_file->fil_ext_lock, true);
+
 	ULONG leftPages = extPages;
 	for (jrd_file* file = main_file; file && leftPages; file = file->fil_next)
 	{
-		ULONG filePages = PIO_get_number_of_pages(file, pageSize);
+		const ULONG filePages = PIO_get_number_of_pages(file, pageSize);
 		const ULONG fileMaxPages = (file->fil_max_page == MAX_ULONG) ? MAX_ULONG : 
 									file->fil_max_page - file->fil_min_page + 1;
 		if (filePages < fileMaxPages)
@@ -272,14 +364,27 @@ void PIO_extend(jrd_file* main_file, const ULONG extPages, const USHORT pageSize
 			LARGE_INTEGER newSize; 
 			newSize.QuadPart = (ULONGLONG) (filePages + extendBy) * pageSize;
 
+			if (ostype == OS_CHICAGO) {
+				file->fil_mutex.enter();
+			}
+
 			const DWORD ret = SetFilePointer(hFile, newSize.LowPart, &newSize.HighPart, FILE_BEGIN);
 			if (ret == INVALID_SET_FILE_POINTER && GetLastError() != NO_ERROR) {
+				if (ostype == OS_CHICAGO) {
+					file->fil_mutex.leave();
+				}
 				nt_error("SetFilePointer", file, isc_io_write_err, 0);
 			}
 			if (!SetEndOfFile(hFile)) {
+				if (ostype == OS_CHICAGO) {
+					file->fil_mutex.leave();
+				}
 				nt_error("SetEndOfFile", file, isc_io_write_err, 0);
 			}
 
+			if (ostype == OS_CHICAGO) {
+				file->fil_mutex.leave();
+			}
 			leftPages -= extendBy;
 		}
 	}
@@ -566,6 +671,9 @@ bool PIO_read(jrd_file* file, BufferDesc* bdb, Ods::pag* page, ISC_STATUS* statu
 	Database* dbb = bdb->bdb_dbb;
 	const DWORD size = dbb->dbb_page_size;
 
+	FileExtendLockGuard extLock(NULL, 
+		dbb->dbb_attachments ? file->fil_ext_lock : NULL, false);
+
 	OVERLAPPED overlapped, *overlapped_ptr;
 	if (!(file = seek_file(file, bdb, status_vector, &overlapped, &overlapped_ptr)))
 		return false;
@@ -784,6 +892,9 @@ bool PIO_write(jrd_file* file, BufferDesc* bdb, Ods::pag* page, ISC_STATUS* stat
 	Database*   dbb  = bdb->bdb_dbb;
 	const DWORD size = dbb->dbb_page_size;
 
+	FileExtendLockGuard extLock(NULL, 
+		dbb->dbb_attachments ? file->fil_ext_lock : NULL, false);
+
 	file = seek_file(file, bdb, status_vector, &overlapped,
 				   &overlapped_ptr);
 	if (!file) {
@@ -1004,6 +1115,7 @@ static jrd_file* setup_file(Database*					dbb,
 	file->fil_desc = desc;
 	file->fil_length = file_name.length();
 	file->fil_max_page = (ULONG) -1;
+	file->fil_ext_lock = NULL;
 #ifdef SUPERSERVER_V2
 	memset(file->fil_io_events, 0, MAX_FILE_IO * sizeof(void*));
 #endif
@@ -1020,7 +1132,7 @@ static jrd_file* setup_file(Database*					dbb,
    under Windows/NT or Chicago */
 // CVC: local variable to all this unit, it means.
 
-	ostype = ISC_is_WinNT() ? OS_WINDOWS_NT : OS_CHICAGO;
+	ostype = /*ISC_is_WinNT() ? OS_WINDOWS_NT : */OS_CHICAGO;
 
 /* Build unique lock string for file and construct lock block */
 
@@ -1049,6 +1161,9 @@ static jrd_file* setup_file(Database*					dbb,
 	l = p - lock_string;
 	fb_assert(l <= sizeof(lock_string)); // In case we continue adding information.
 
+	file->fil_ext_lock = FB_NEW(*dbb->dbb_permanent) 
+		FileExtendLock(*dbb->dbb_permanent, l, lock_string);
+
 	Lock* lock = FB_NEW_RPT(*dbb->dbb_permanent, l) Lock;
 	dbb->dbb_lock = lock;
 	lock->lck_type = LCK_database;

src/jrd/pag.cpp

@@ -2288,18 +2288,15 @@ bool PageSpace::extend(thread_db* tdbb, const ULONG pageNum)
 	if (pageNum < maxPageNumber || MAX_EXTEND_BYTES < MIN_EXTEND_BYTES)
 		return true;
 
+#ifdef WIN_NT
+	// hvlad: we need an attachment lock to get exclusive LCK_file_extend lock
+	// see comments in winnt.cpp\FileExtendLock::FileExtendLock
+	if (!tdbb->tdbb_attachment || !tdbb->tdbb_attachment->att_lock_owner_handle)
+		return false;
+#endif
+
 	Database* dbb = tdbb->tdbb_database;
 
-	Lock temp_lock;
-	temp_lock.lck_dbb = dbb;
-	temp_lock.lck_object = this;
-	temp_lock.lck_type = LCK_file_extend;
-	temp_lock.lck_owner_handle = LCK_get_owner_handle(tdbb, temp_lock.lck_type);
-	temp_lock.lck_parent = dbb->dbb_lock;
-	temp_lock.lck_length = sizeof(SLONG);
-	temp_lock.lck_key.lck_long = this->pageSpaceID;
-
-	LCK_lock(tdbb, &temp_lock, LCK_EX, LCK_WAIT);
 	if (pageNum >= maxAlloc(dbb->dbb_page_size))
 	{
 		const ULONG minExtendPages = MIN_EXTEND_BYTES / dbb->dbb_page_size;
@@ -2332,7 +2329,6 @@ bool PageSpace::extend(thread_db* tdbb, const ULONG pageNum)
 
 					gds__log("Error extending file \"%s\" by %lu page(s).\nCurrently allocated %lu pages, requested page number %lu", 
 						file->fil_string, extPages, maxPageNumber, pageNum);
-					LCK_release(tdbb, &temp_lock);
 					return false;
 				}
 			}
@@ -2341,7 +2337,6 @@ bool PageSpace::extend(thread_db* tdbb, const ULONG pageNum)
 		THREAD_ENTER();
 		maxPageNumber = 0; 
 	}
-	LCK_release(tdbb, &temp_lock);
 	return true;
 }
 
@@ -2386,6 +2381,15 @@ void PageManager::closeAll()
 		}
 }
 
+void PageManager::releaseLocks()
+{
+	for (size_t i = 0; i < pageSpaces.getCount(); i++)
+		if (pageSpaces[i]->file && pageSpaces[i]->file->fil_ext_lock) {
+			delete pageSpaces[i]->file->fil_ext_lock;
+			pageSpaces[i]->file->fil_ext_lock = NULL;
+		}
+}
+
 USHORT PageManager::getTempPageSpaceID(thread_db* tdbb)
 {
 #ifdef SUPERSERVER

src/jrd/pag.h

@@ -138,6 +138,7 @@ public:
 	USHORT getTempPageSpaceID(thread_db* tdbb);
 
 	void closeAll();
+	void releaseLocks();
 
 	SLONG pagesPerPIP;			// Pages per pip 
 	ULONG bytesBitPIP;			// Number of bytes of bit in PIP