Re: [HACKERS] 0x1A in control file on Windows

I found a bug that pg_controldata ends with error if control files
contain 0x1A (Ctrl+Z) on Windows.

We probably need to add PG_BINARY when we open control files
because 0x1A is an end-of-file marker on Windows.
This fix needs to be applied in back versions (8.2, 8.3 and HEAD).

Index: src/bin/pg_controldata/pg_controldata.c
===================================================================
--- src/bin/pg_controldata/pg_controldata.c (head)
+++ src/bin/pg_controldata/pg_controldata.c (pg_control_0x1A)
@@ -107,7 +107,7 @@

snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir);

- if ((fd = open(ControlFilePath, O_RDONLY, 0)) == -1)
+ if ((fd = open(ControlFilePath, O_RDONLY | PG_BINARY, 0)) == -1)
{
fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"),
progname, ControlFilePath, strerror(errno));
Index: src/bin/pg_resetxlog/pg_resetxlog.c
===================================================================
--- src/bin/pg_resetxlog/pg_resetxlog.c (head)
+++ src/bin/pg_resetxlog/pg_resetxlog.c (pg_control_0x1A)
@@ -373,7 +373,7 @@
char *buffer;
pg_crc32 crc;

- if ((fd = open(XLOG_CONTROL_FILE, O_RDONLY, 0)) < 0)
+ if ((fd = open(XLOG_CONTROL_FILE, O_RDONLY | PG_BINARY, 0)) < 0)
{
/*
* If pg_control is not there at all, or we can't read it, the odds

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
> I found a bug that pg_controldata ends with error if control files
> contain 0x1A (Ctrl+Z) on Windows.

> We probably need to add PG_BINARY when we open control files
> because 0x1A is an end-of-file marker on Windows.

Well, why is that a bug? If the platform is so silly as to define text
files that way, who are we to argue?

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
> > We probably need to add PG_BINARY when we open control files
> > because 0x1A is an end-of-file marker on Windows.
>
> Well, why is that a bug? If the platform is so silly as to define text
> files that way, who are we to argue?

Google says it is for for backward compatibility with CP/M
http://en.wikipedia.org/wiki/End-of-file
and adding O_BINARY is the answer.
http://codenewbie.com/forum/standard-c-c/1208-binary-i-o-file-reading-0x1a-trouble.html

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
>>> We probably need to add PG_BINARY when we open control files
>>> because 0x1A is an end-of-file marker on Windows.
>> Well, why is that a bug? If the platform is so silly as to define text
>> files that way, who are we to argue?
>
> Google says it is for for backward compatibility with CP/M
> http://en.wikipedia.org/wiki/End-of-file
> and adding O_BINARY is the answer.
> http://codenewbie.com/forum/standard-c-c/1208-binary-i-o-file-reading-0x1a-trouble.html

Yes, apparently that's exactly why we have PG_BINARY, see c.h:

> /*
> * NOTE: this is also used for opening text files.
> * WIN32 treats Control-Z as EOF in files opened in text mode.
> * Therefore, we open files in binary mode on Win32 so we can read
> * literal control-Z. The other affect is that we see CRLF, but
> * that is OK because we can already handle those cleanly.
> */
> #if defined(WIN32) || defined(__CYGWIN__)
> #define PG_BINARY O_BINARY
> #define PG_BINARY_A "ab"
> #define PG_BINARY_R "rb"
> #define PG_BINARY_W "wb"
> #else
> #define PG_BINARY 0
> #define PG_BINARY_A "a"
> #define PG_BINARY_R "r"
> #define PG_BINARY_W "w"
> #endif

I don't see anything wrong with the patch, but I wonder if there's more
open() calls that need the same treatment? Like the one in
pg_resetxlog.c/ReadControlFile().

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:
> ITAGAKI Takahiro wrote:
>> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>
>>> ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
>>>> We probably need to add PG_BINARY when we open control files
>>>> because 0x1A is an end-of-file marker on Windows.
>>> Well, why is that a bug? If the platform is so silly as to define text
>>> files that way, who are we to argue?
>>
>> Google says it is for for backward compatibility with CP/M
>> http://en.wikipedia.org/wiki/End-of-file
>> and adding O_BINARY is the answer.
>>
>> http://codenewbie.com/forum/standard-c-c/1208-binary-i-o-file-reading-0x1a-trouble.html
>>
>
> Yes, apparently that's exactly why we have PG_BINARY, see c.h:
>
>> /*
>> * NOTE: this is also used for opening text files.
>> * WIN32 treats Control-Z as EOF in files opened in text mode.
>> * Therefore, we open files in binary mode on Win32 so we can read
>> * literal control-Z. The other affect is that we see CRLF, but
>> * that is OK because we can already handle those cleanly.
>> */
>> #if defined(WIN32) || defined(__CYGWIN__)
>> #define PG_BINARY O_BINARY
>> #define PG_BINARY_A "ab"
>> #define PG_BINARY_R "rb"
>> #define PG_BINARY_W "wb"
>> #else
>> #define PG_BINARY 0
>> #define PG_BINARY_A "a"
>> #define PG_BINARY_R "r"
>> #define PG_BINARY_W "w"
>> #endif
>
> I don't see anything wrong with the patch, but I wonder if there's more
> open() calls that need the same treatment? Like the one in
> pg_resetxlog.c/ReadControlFile().

Agreed, and I think that one would also need it - and pg_resetxlog
already does this when it writes the file. A quick look doesn't show any
other places, but I may have missed some?

/Magnus

Magnus Hagander wrote:
> Heikki Linnakangas wrote:
>> ITAGAKI Takahiro wrote:
>>> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>
>>>> ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
>>>>> We probably need to add PG_BINARY when we open control files
>>>>> because 0x1A is an end-of-file marker on Windows.
>>>> Well, why is that a bug? If the platform is so silly as to define text
>>>> files that way, who are we to argue?
>>> Google says it is for for backward compatibility with CP/M
>>> http://en.wikipedia.org/wiki/End-of-file
>>> and adding O_BINARY is the answer.
>>>
>>> http://codenewbie.com/forum/standard-c-c/1208-binary-i-o-file-reading-0x1a-trouble.html
>>>
>> Yes, apparently that's exactly why we have PG_BINARY, see c.h:
>>
>>> /*
>>> * NOTE: this is also used for opening text files.
>>> * WIN32 treats Control-Z as EOF in files opened in text mode.
>>> * Therefore, we open files in binary mode on Win32 so we can read
>>> * literal control-Z. The other affect is that we see CRLF, but
>>> * that is OK because we can already handle those cleanly.
>>> */
>>> #if defined(WIN32) || defined(__CYGWIN__)
>>> #define PG_BINARY O_BINARY
>>> #define PG_BINARY_A "ab"
>>> #define PG_BINARY_R "rb"
>>> #define PG_BINARY_W "wb"
>>> #else
>>> #define PG_BINARY 0
>>> #define PG_BINARY_A "a"
>>> #define PG_BINARY_R "r"
>>> #define PG_BINARY_W "w"
>>> #endif
>> I don't see anything wrong with the patch, but I wonder if there's more
>> open() calls that need the same treatment? Like the one in
>> pg_resetxlog.c/ReadControlFile().
>
> Agreed, and I think that one would also need it - and pg_resetxlog
> already does this when it writes the file. A quick look doesn't show any
> other places, but I may have missed some?

I had a chat with Heikki about this, and the proper way to fix it.

Should there actually be any reason not to *always* open our files with
O_BINARY? That seems to be what should mimic what Unix does, which would
be what we expect, no?

If that is so, then I propose we do that for 8.4, and just backpatch the
O_BINARY flag to these two locations for 8.3 and 8.2. Thoughts?

//Magnus

Magnus Hagander wrote:
> I had a chat with Heikki about this, and the proper way to fix it.
>
> Should there actually be any reason not to *always* open our files with
> O_BINARY? That seems to be what should mimic what Unix does, which would
> be what we expect, no?
>
> If that is so, then I propose we do that for 8.4, and just backpatch the
> O_BINARY flag to these two locations for 8.3 and 8.2. Thoughts?
>
>
>

ISTR there are a few places where we want CRLF translation (config files?)

I'd be fairly conservative about making changes like this.

cheers

andrew

Andrew Dunstan wrote:
>
>
> Magnus Hagander wrote:
>> I had a chat with Heikki about this, and the proper way to fix it.
>>
>> Should there actually be any reason not to *always* open our files with
>> O_BINARY? That seems to be what should mimic what Unix does, which would
>> be what we expect, no?
>>
>> If that is so, then I propose we do that for 8.4, and just backpatch the
>> O_BINARY flag to these two locations for 8.3 and 8.2. Thoughts?
>>
>>
>>
>
> ISTR there are a few places where we want CRLF translation (config files?)

These seem to be using fopen() (through AllocateFile()), which wouldn't
be affected by this.

> I'd be fairly conservative about making changes like this.

This is why my proposal would be not to backpatch such a change, but to
only do it for 8.4.

//Magnus

Tom Lane wrote:
> ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
> > I found a bug that pg_controldata ends with error if control files
> > contain 0x1A (Ctrl+Z) on Windows.
>
> > We probably need to add PG_BINARY when we open control files
> > because 0x1A is an end-of-file marker on Windows.
>
> Well, why is that a bug? If the platform is so silly as to define text
> files that way, who are we to argue?

The problem is that our pg_controldata might have binary values that
contain 0x1a that will be confused by the operating system as
end-of-file.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> Well, why is that a bug? If the platform is so silly as to define text
>> files that way, who are we to argue?

> The problem is that our pg_controldata might have binary values that
> contain 0x1a that will be confused by the operating system as
> end-of-file.

pg_controldata is certainly already being read as binary. The
discussion here is about *text* files, particularly configuration
files. Why should we not adhere to the platform standard about
what a text file is?

If you need a positive reason why this might be a bad idea, consider the
idea that someone is examining postgresql.conf with a text editor that
stops reading at control-Z. He might not be able to see items that the
postmaster is treating as valid.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>
>> Tom Lane wrote:
>>
>>> Well, why is that a bug? If the platform is so silly as to define text
>>> files that way, who are we to argue?
>>>
>
>
>> The problem is that our pg_controldata might have binary values that
>> contain 0x1a that will be confused by the operating system as
>> end-of-file.
>>
>
> pg_controldata is certainly already being read as binary.

Umm, no, it is in the backend I believe but not in the utilities. Hence
the original bug report. We need to add the binary flag in
pg_controldata.c and pg_resetxlog.c.

> The
> discussion here is about *text* files, particularly configuration
> files. Why should we not adhere to the platform standard about
> what a text file is?
>
> If you need a positive reason why this might be a bad idea, consider the
> idea that someone is examining postgresql.conf with a text editor that
> stops reading at control-Z. He might not be able to see items that the
> postmaster is treating as valid.
>
>
>

Yes, exactly right. We certainly can't just open everything in binary
mode. Magnus did say that all the current config files are opened in
text mode as far as he could see.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Tom Lane wrote:
>> pg_controldata is certainly already being read as binary.

> Umm, no, it is in the backend I believe but not in the utilities. Hence
> the original bug report. We need to add the binary flag in
> pg_controldata.c and pg_resetxlog.c.

Ah, okay, that's surely a bug. But I think the discussion here was
about adding PG_BINARY to *all* open requests. That I don't like.

regards, tom lane

Andrew Dunstan wrote:
>
>
> Tom Lane wrote:
>> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>>
>>> Tom Lane wrote:
>>>
>>>> Well, why is that a bug? If the platform is so silly as to define text
>>>> files that way, who are we to argue?
>>>>
>>
>>
>>> The problem is that our pg_controldata might have binary values that
>>> contain 0x1a that will be confused by the operating system as
>>> end-of-file.
>>>
>>
>> pg_controldata is certainly already being read as binary.
>
> Umm, no, it is in the backend I believe but not in the utilities. Hence
> the original bug report. We need to add the binary flag in
> pg_controldata.c and pg_resetxlog.c.

Right.
I'll go ahead and put that part in (I find two locations - the one in
the original patch, and the extra one Heikki noticed).

>> The
>> discussion here is about *text* files, particularly configuration
>> files. Why should we not adhere to the platform standard about
>> what a text file is?
>>
>> If you need a positive reason why this might be a bad idea, consider the
>> idea that someone is examining postgresql.conf with a text editor that
>> stops reading at control-Z. He might not be able to see items that the
>> postmaster is treating as valid.
>>
>>
>>
>
> Yes, exactly right. We certainly can't just open everything in binary
> mode. Magnus did say that all the current config files are opened in
> text mode as far as he could see.

The point being that the config files are opened with AllocateFile(),
which in turn calls fopen(). It doesn't use open(). The proposal was
only to make all *open()* calls do it binary. I was under the impression
that on Unix, that's what open() did, so we should behave the same?

//Magnus

Magnus Hagander wrote:
> Andrew Dunstan wrote:
>>
>> Tom Lane wrote:
>>> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>>>
>>>> Tom Lane wrote:
>>>>
>>>>> Well, why is that a bug? If the platform is so silly as to define text
>>>>> files that way, who are we to argue?
>>>>>
>>>
>>>> The problem is that our pg_controldata might have binary values that
>>>> contain 0x1a that will be confused by the operating system as
>>>> end-of-file.
>>>>
>>> pg_controldata is certainly already being read as binary.
>> Umm, no, it is in the backend I believe but not in the utilities. Hence
>> the original bug report. We need to add the binary flag in
>> pg_controldata.c and pg_resetxlog.c.
>
> Right.
> I'll go ahead and put that part in (I find two locations - the one in
> the original patch, and the extra one Heikki noticed).

Eh, both were in the original patch, I just didn't scroll far enough :-)

Applied to HEAD and backpatched back to 8.2 - since it only affects
Windows, we don't go further.

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Andrew Dunstan wrote:
>> Tom Lane wrote:
>>> If you need a positive reason why this might be a bad idea, consider the
>>> idea that someone is examining postgresql.conf with a text editor that
>>> stops reading at control-Z. He might not be able to see items that the
>>> postmaster is treating as valid.
>>
>> Yes, exactly right. We certainly can't just open everything in binary
>> mode. Magnus did say that all the current config files are opened in
>> text mode as far as he could see.

> The point being that the config files are opened with AllocateFile(),
> which in turn calls fopen(). It doesn't use open(). The proposal was
> only to make all *open()* calls do it binary. I was under the impression
> that on Unix, that's what open() did, so we should behave the same?

That seems just weird. I do not think there's any correlation between
whether we use open or fopen and whether the file is text or binary.
Even if it happens to be true right now, depending on it would be
fragile.

regards, tom lane

Tom Lane wrote:
>> The point being that the config files are opened with AllocateFile(),
>> which in turn calls fopen(). It doesn't use open(). The proposal was
>> only to make all *open()* calls do it binary. I was under the impression
>> that on Unix, that's what open() did, so we should behave the same?
>>
>
> That seems just weird. I do not think there's any correlation between
> whether we use open or fopen and whether the file is text or binary.
> Even if it happens to be true right now, depending on it would be
> fragile.
>
>
>

I agree. If you really want something like that you should invent
OpenConfigFile() or some such. But it hardly seems worth it.

cheers

andrew

Andrew Dunstan wrote:
>
>
> Tom Lane wrote:
>>> The point being that the config files are opened with AllocateFile(),
>>> which in turn calls fopen(). It doesn't use open(). The proposal was
>>> only to make all *open()* calls do it binary. I was under the impression
>>> that on Unix, that's what open() did, so we should behave the same?
>>>
>>
>> That seems just weird. I do not think there's any correlation between
>> whether we use open or fopen and whether the file is text or binary.
>> Even if it happens to be true right now, depending on it would be
>> fragile.
>>
>>
>>
>
> I agree. If you really want something like that you should invent
> OpenConfigFile() or some such. But it hardly seems worth it.

Well, the AllocateFile() API already has the ability to specify if it's
a text file or not (using the fopen syntax).

And given that we have a way to specify it for open (PG_BINARY), we can
just leave it that way I guess. Even if we end up always specifying
PG_BINARY, that may keep things less fragile. As long as we remember to
specify it. (PG_BINARY is a no-op on all other platforms than Windows,
btw, because there is no way to specify anything other than binary mode
for open() on Unix)

//Magnus