mm_last_pg+1 != filesize (was: openldap.git branch mdb.master updated. 209b56fead1afe8273db6c714c0a74a9c09b9cf6)
On 21/12/15 03:39, openldap-commit2devel@OpenLDAP.org wrote:
commit 209b56fead1afe8273db6c714c0a74a9c09b9cf6 Author: Howard Chu hyc@openldap.org Date: Mon Dec 21 02:36:20 2015 +0000
ITS#8324 fix for WRITEMAP We called FlushViewOfFile with (map,mapsize) which worked fine when we had allocated the entire map already. Now we have to make sure to only flush as much as was actually written. Add a numpgs argument to tell how much to flush in env_sync0().
Do env_sync() and commit() survive the test program from ITS#7886? It creates a datafile which ends before mm_last_pg+1.
Hallvard Breien Furuseth wrote:
On 21/12/15 03:39, openldap-commit2devel@OpenLDAP.org wrote:
commit 209b56fead1afe8273db6c714c0a74a9c09b9cf6 Author: Howard Chu hyc@openldap.org Date: Mon Dec 21 02:36:20 2015 +0000
ITS#8324 fix for WRITEMAP We called FlushViewOfFile with (map,mapsize) which worked fine when we had allocated the entire map already. Now we have to make sure to only flush as much as was actually written. Add a numpgs argument to tell how much to flush in env_sync0().Do env_sync() and commit() survive the test program from ITS#7886? It creates a datafile which ends before mm_last_pg+1.
That would probably crash on Windows, I'll check it later. Annoying that Windows doesn't just flush whatever exists and move on.
Could add a filesize check to env_open and fail if last_pgno doesn't match. I'm not too fussed about it, you have to be deliberately malicious to create this condition. You get a Bus Error or a SEGV - that's a lot safer than tamely returning an error code that the caller could ignore.
On 22/12/15 08:23, Howard Chu wrote:
Hallvard Breien Furuseth wrote:
Do env_sync() and commit() survive the test program from ITS#7886? It creates a datafile which ends before mm_last_pg+1.
That would probably crash on Windows, I'll check it later. Annoying that Windows doesn't just flush whatever exists and move on.
Could add a filesize check to env_open and fail if last_pgno doesn't match.
Not enough if there are several writer processes. For a growing file, the check would have to go in mdb_txn_begin().
I'm not too fussed about it, you have to be deliberately malicious to create this condition. You get a Bus Error or a SEGV - that's a lot safer than tamely returning an error code that the caller could ignore.
Not malicious. Omit some commits like slapadd -q does, and it can happen naturally. Maybe slapadd -q can do just that with large IDLs.
I think MDB_meta and MDB_txn need "last written page" maintained by mdb_page_flush(). Can VL32 wait for MDB_DATA_VERSION > 1?
Hallvard Breien Furuseth wrote:
On 22/12/15 08:23, Howard Chu wrote:
Hallvard Breien Furuseth wrote:
Do env_sync() and commit() survive the test program from ITS#7886? It creates a datafile which ends before mm_last_pg+1.
That would probably crash on Windows, I'll check it later. Annoying that Windows doesn't just flush whatever exists and move on.
Could add a filesize check to env_open and fail if last_pgno doesn't match.
Not enough if there are several writer processes. For a growing file, the check would have to go in mdb_txn_begin().
Not an issue. The test program from its7886 runs just fine.
I'm not too fussed about it, you have to be deliberately malicious to create this condition. You get a Bus Error or a SEGV - that's a lot safer than tamely returning an error code that the caller could ignore.
Not malicious. Omit some commits like slapadd -q does, and it can happen naturally. Maybe slapadd -q can do just that with large IDLs.
The change in question was for msync, which is only used for MDB_WRITEMAP. On Windows, for WRITEMAP, we always have to expand the file to make sure referenced pages exist, before referencing them. The filesize growth occurs regardless of whether a subsequent commit happens or not.
Regular fdatasync doesn't require a length parameter, so it's irrelevant.
I think MDB_meta and MDB_txn need "last written page" maintained by mdb_page_flush(). Can VL32 wait for MDB_DATA_VERSION > 1?
mdb_page_flush() already maintains last_pgno for MDB_VL32.
There is no issue here.
I forgot this thread...
On 22/12/15 16:39, Howard Chu wrote:
Not malicious. Omit some commits like slapadd -q does, and it can happen naturally. Maybe slapadd -q can do just that with large IDLs.
The change in question was for msync, which is only used for MDB_WRITEMAP.
True, I missed that.
Regular fdatasync doesn't require a length parameter, so it's irrelevant.
I think MDB_meta and MDB_txn need "last written page" maintained by mdb_page_flush(). Can VL32 wait for MDB_DATA_VERSION > 1?
mdb_page_flush() already maintains last_pgno for MDB_VL32.
Yes, but it initializes mt_last_pgno = meta->mm_last_pg, which can be beyond the filesize.
I mean to get back to that test program and test it on Windows with bigger memory chunks, but I don't seem to be getting around to it...
-
Hallvard Breien Furuseth -
Howard Chu