common: (win) fix possible pool file coruption

[marcinslusarz]

It turns out that _wopen on Windows can truncate a file by 1 byte when O_BINARY flag is not used and the last byte has a value of 0x1a. It doesn't always happen, but it does happen frequently enough. Currently this behavior is not mentioned in _wopen documentation (https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/open-wopen?view=vs-2019), but is in fopen documentation: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-wfopen?view=vs-2019. Specifically it says:

In text mode, CTRL+Z is interpreted as an EOF character on input. In files that are opened for reading/writing by using "a+", fopen checks for a CTRL+Z at the end of the file and removes it, if it is possible. This is done because using fseek and ftell to move within a file that ends with CTRL+Z may cause fseek to behave incorrectly near the end of the file.

Opening files without O_BINARY was never an immediate problem for us, because we are not reading anything from the opened files using "read", just mapping them.

This bug affects:

• pmem_map_file
• pmempool_create
• pmempool_dump
• pmempool_info
• pmempool_transform

libpmemobj, libpmemblk, libpmemlog, librpmem, libvmem and libvmmalloc are not affected.

Ref: pmem/issues#972
Ref: pmem/issues#715
Ref: pmem/issues#603

---

This change is 

[pmem-bot]

@pbalcer and @wlemkows please review this pull request

[kilobyte]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @marcinslusarz)

---

src/common/file.c, line 293 at r1 (raw file):

	int flags = O_RDWR;
#ifdef _WIN32
	flags |= O_BINARY;

I wonder if it would be more maintenable to #define O_BINARY 0 on non-Windows, then use it unconditionally everywhere. Less #ifdefs.

[marcinslusarz]

---

src/common/file.c, line 293 at r1 (raw file):

Previously, kilobyte (Adam Borowski) wrote…

I wonder if it would be more maintenable to #define O_BINARY 0 on non-Windows, then use it unconditionally everywhere. Less #ifdefs.

Unfortunately it's not that simple. O_BINARY is in global/libc name space, so we can't just define it, because it may clash with future definitions of this constant. Using PMDK_O_BINARY with O_RDWR would look awkward. If I would start fixing this (and the whole mess of unicode support on Windows), it would be much longer effort. This is a minimal fix which works and will let us release 1.6.1 soon. It can be cleaned up later on master.

[kilobyte]

Reviewable status: 0 of 2 files reviewed, all discussions resolved

---

src/common/file.c, line 293 at r1 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Unfortunately it's not that simple. O_BINARY is in global/libc name space, so we can't just define it, because it may clash with future definitions of this constant. Using PMDK_O_BINARY with O_RDWR would look awkward. If I would start fixing this (and the whole mess of unicode support on Windows), it would be much longer effort. This is a minimal fix which works and will let us release 1.6.1 soon. It can be cleaned up later on master.

Right then.

[pbalcer]

Can we add a windows test for this specific case?

[marcinslusarz]

Can we add a windows test for this specific case?

https://github.com/marcinslusarz/pmdk/commits/win-trunc-test

I'll submit it once this PR and necessary bug fixes for Python test framework will land on master.

[krzycz]

Reviewed 1 of 2 files at r1, 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[marcinslusarz]

@pbalcer ?