mm_last_pg+1 != filesize (was: openldap.git branch mdb.master updated. 209b56fead1afe8273db6c714c0a74a9c09b9cf6)
11:05 p.m.
On 21/12/15 03:39, openldap-commit2devel(a)OpenLDAP.org wrote:
commit 209b56fead1afe8273db6c714c0a74a9c09b9cf6
Author: Howard Chu <hyc(a)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.
11:23 p.m.
New subject: mm_last_pg+1 != filesize
Hallvard Breien Furuseth wrote:
On 21/12/15 03:39, openldap-commit2devel(a)OpenLDAP.org wrote:
> commit 209b56fead1afe8273db6c714c0a74a9c09b9cf6
> Author: Howard Chu <hyc(a)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.
--
-- Howard Chu
CTO, Symas Corp. http://www.symas.com
Director, Highland Sun http://highlandsun.com/hyc/
Chief Architect, OpenLDAP http://www.openldap.org/project/
11:52 p.m.
New subject: mm_last_pg+1 != filesize
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?
7:39 a.m.
New subject: mm_last_pg+1 != filesize
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.
--
-- Howard Chu
CTO, Symas Corp. http://www.symas.com
Director, Highland Sun http://highlandsun.com/hyc/
Chief Architect, OpenLDAP http://www.openldap.org/project/
2:39 p.m.
New subject: mm_last_pg+1 != filesize
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...
2822
days inactive
2836
days old
4 comments
2 participants
participants (2)
-
Hallvard Breien Furuseth -
Howard Chu