Bad bug in fopen() wrapper code

"Magnus Hagander" <mha@sollentuna.net> writes:

Attached patch sets the O_CREAT option when appending to files.

That looks correct, but I went looking to see if there were any other
mistakes of the same ilk, and I'm wondering what the sense is in
openFlagsToCreateFileFlags ... seems like it's ignoring O_EXCL in
some combinations and not others. Is that right?

regards, tom lane

Attached patch sets the O_CREAT option when appending to files.

That looks correct, but I went looking to see if there were
any other mistakes of the same ilk, and I'm wondering what
the sense is in openFlagsToCreateFileFlags ... seems like
it's ignoring O_EXCL in some combinations and not others. Is
that right?

That is part of the original open() code that Claudio did back for 8.0,
so it has definitly been working since then. I haven't really read into
the code, though... But a qiuck look doesn't show me any place wher eit
does ignore O_EXCL - which combination would that be?

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

That is part of the original open() code that Claudio did back for 8.0,
so it has definitly been working since then.

Hm, maybe best not to touch it, but still...

I haven't really read into
the code, though... But a qiuck look doesn't show me any place wher eit
does ignore O_EXCL - which combination would that be?

What's bugging me is that 0 and O_EXCL give the same answer, and
O_TRUNC and O_TRUNC | O_EXCL give the same answer, but O_CREAT and
O_CREAT | O_EXCL give different answers, as do O_CREAT | O_TRUNC
and O_CREAT | O_TRUNC | O_EXCL. I'm also pretty suspicious of
both O_CREAT | O_EXCL and O_CREAT | O_TRUNC | O_EXCL giving the
same answer. However, I have no idea what the semantics are of
the symbols the function is mapping into, so maybe it's OK.

regards, tom lane

"Magnus Hagander" <mha@sollentuna.net> writes:

This is pretty bad and pretty urgent - with this, systems installed by
the MSI installer simply *do not start*, because they are by default
configured to write logs to a file...

Attached patch sets the O_CREAT option when appending to files.

Applied, but I'm still wondering about openFlagsToCreateFileFlags() ...

regards, tom lane

Hello guys,

it's been a while, but...

What's bugging me is that 0 and O_EXCL give the same answer, and
O_TRUNC and O_TRUNC | O_EXCL give the same answer,

This is ok, as (iirc) O_EXCL only has effect in the presence of O_CREAT.
(a comment to this effect would help here, as well as perhaps links to the CreateFile and open specs)

but O_CREAT and O_CREAT | O_EXCL give different answers,
as do O_CREAT | O_TRUNC and O_CREAT | O_TRUNC | O_EXCL.

See above.

I'm also pretty suspicious of both O_CREAT | O_EXCL and
O_CREAT | O_TRUNC | O_EXCL giving the same answer.

O_CREAT | O_EXCL is effectively "create, but fail if the file exists", which is the behaviour specified by CREATE_NEW. Adding O_TRUNC to this combination, which can only apply to a successfully opened existent file, can have no impact afaics.

Cheers,
Claudio

What's bugging me is that 0 and O_EXCL give the same answer, and
O_TRUNC and O_TRUNC | O_EXCL give the same answer,

This is ok, as (iirc) O_EXCL only has effect in the presence
of O_CREAT.

<snip more explanation>

Thanks, Claudio!

After looking at the code some more, and actually reading up on the
specs a bit more, it certainly does look like it's safe. So I don't
think we need to do anything about that.

Now, I still twist my head around the lines:
if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0
||
(fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd,
fileFlags & (O_TEXT | O_BINARY)) < 0)))

With the _setmode() call deep in the if statement... I would suggest we
split that up into a couple of lines to make it more readable - I'm sure
all compilers will easily optimise it into the same code anyway.
Reasonable?

//Magnus

Magnus Hagander writes:

Now, I still twist my head around the lines:
if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0
||
(fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd,
fileFlags & (O_TEXT | O_BINARY)) < 0)))

With the _setmode() call deep in the if statement... I would suggest we
split that up into a couple of lines to make it more readable - I'm sure
all compilers will easily optimise it into the same code anyway.
Reasonable?

I agree it would be clearer if split up.

Without having studied it closely, it might also highlight a bug on failure of the second clause -- if the _setmode fails, shouldn't _close be called instead of CloseHandle, and -1 returned? (CloseHandle would still be called on failure of the _open_osfhandle, obviously)

Cheers,
Claudio

"Claudio Natoli" <claudio.natoli@memetrics.com> writes:

Magnus Hagander writes:

Now, I still twist my head around the lines:
if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0
||
(fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd,
fileFlags & (O_TEXT | O_BINARY)) < 0)))

Without having studied it closely, it might also highlight a bug on failure of the second clause -- if the _setmode fails, shouldn't _close be called instead of CloseHandle, and -1 returned? (CloseHandle would still be called on failure of the _open_osfhandle, obviously)

I agree that this code is both wrong and unreadable (although in
practice the _setmode will probably never fail, which is why our
attention hasn't been drawn to it). Is someone going to submit a
patch? I'm hesitant to change the code myself since I'm not in
a position to test it.

regards, tom lane

Now, I still twist my head around the lines:
if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0
||
(fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd, fileFlags &

(O_TEXT

| O_BINARY)) < 0)))

Without having studied it closely, it might also highlight a bug

on

failure of the second clause -- if the _setmode fails, shouldn't
_close be called instead of CloseHandle, and -1 returned?
(CloseHandle would still be called on failure of the

_open_osfhandle,

obviously)

I agree that this code is both wrong and unreadable (although in
practice the _setmode will probably never fail, which is why our
attention hasn't been drawn to it). Is someone going to submit a
patch? I'm hesitant to change the code myself since I'm not in a
position to test it.

I can look at fixing that. Is it something we want to do for 8.2, or
wait until 8.3? If there's a hidden bug, I guess 8.2?

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

I agree that this code is both wrong and unreadable (although in
practice the _setmode will probably never fail, which is why our
attention hasn't been drawn to it). Is someone going to submit a
patch? I'm hesitant to change the code myself since I'm not in a
position to test it.

I can look at fixing that. Is it something we want to do for 8.2, or
wait until 8.3? If there's a hidden bug, I guess 8.2?

Yeah, I think it's 8.2 material.

regards, tom lane

Magnus Hagander wrote:

Now, I still twist my head around the lines:
if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0
||
(fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd, fileFlags &

(O_TEXT

| O_BINARY)) < 0)))

Without having studied it closely, it might also highlight a bug

on

failure of the second clause -- if the _setmode fails, shouldn't
_close be called instead of CloseHandle, and -1 returned?
(CloseHandle would still be called on failure of the

_open_osfhandle,

obviously)

I agree that this code is both wrong and unreadable (although in
practice the _setmode will probably never fail, which is why our
attention hasn't been drawn to it). Is someone going to submit a
patch? I'm hesitant to change the code myself since I'm not in a
position to test it.

I can look at fixing that. Is it something we want to do for 8.2, or
wait until 8.3? If there's a hidden bug, I guess 8.2?

Magnus, is this the right fix?

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

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

Bruce Momjian wrote:

Magnus Hagander wrote:

Now, I still twist my head around the lines:
if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0
||
(fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd, fileFlags &

(O_TEXT

| O_BINARY)) < 0)))

Without having studied it closely, it might also highlight a bug

on

failure of the second clause -- if the _setmode fails, shouldn't
_close be called instead of CloseHandle, and -1 returned?
(CloseHandle would still be called on failure of the

_open_osfhandle,

obviously)

I agree that this code is both wrong and unreadable (although in
practice the _setmode will probably never fail, which is why our
attention hasn't been drawn to it). Is someone going to submit a
patch? I'm hesitant to change the code myself since I'm not in a
position to test it.

I can look at fixing that. Is it something we want to do for 8.2, or
wait until 8.3? If there's a hidden bug, I guess 8.2?

Magnus, is this the right fix?

Claudio sent me some small updates to the patch; updated version
attached.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

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

Without having studied it closely, it might also

highlight a bug

on

failure of the second clause -- if the _setmode

fails, shouldn't

_close be called instead of CloseHandle, and -1 returned?
(CloseHandle would still be called on failure of the

_open_osfhandle,

obviously)

I agree that this code is both wrong and unreadable

(although in

practice the _setmode will probably never fail, which

is why our

attention hasn't been drawn to it). Is someone going

to submit a

patch? I'm hesitant to change the code myself since

I'm not in a

position to test it.

I can look at fixing that. Is it something we want to do

for 8.2, or

wait until 8.3? If there's a hidden bug, I guess 8.2?

Magnus, is this the right fix?

Claudio sent me some small updates to the patch; updated
version attached.

If we're going for maximum readability, I'd actually split
+ 	else if (fileFlags & (O_TEXT | O_BINARY) &&
+ 		_setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0)

into two different if statements as wlel. Ee.g.
else if (fileFlags (O_TEXT | O_BINARY)) {
if (_setmode() < 0) {
failure();
}
}

Haven't tested the patch yet, but it looks ok.

//Magnus

Magnus, is this the right fix?

Well, actually msdn states:

"Return Value
If successful, _setmode returns the previous translation mode. A return
value of -1 indicates an error"

So, shouldn't we be testing for -1 instead of < 0 ?

The thing is probably academic, since _setmode is only supposed to fail
on invalid file handle or invalid mode.
So basically, given our code, it should only fail if filemode is
(O_BINARY | O_TEXT) both flags set.

Andreas

"Zeugswetter Andreas DCP SD" <ZeugswetterA@spardat.at> writes:

"If successful, _setmode returns the previous translation mode. A return
value of -1 indicates an error"

So, shouldn't we be testing for -1 instead of < 0 ?

I think the usual convention is to test for < 0, unless there are other
negative return values that are legal. This is doubtless a silly
cycle-shaving habit (on nearly all machines, test against 0 is a bit
more compact than test against other constants), but it is a widespread
habit anyway, and if you sometimes do it one way and sometimes another
you just create a distraction for readers.

regards, tom lane

I agree that this code is both wrong and unreadable

(although in

practice the _setmode will probably never fail, which

is why our

attention hasn't been drawn to it). Is someone going

to submit a

patch? I'm hesitant to change the code myself since

I'm not in a

position to test it.

I can look at fixing that. Is it something we want to do

for 8.2, or

wait until 8.3? If there's a hidden bug, I guess 8.2?

Magnus, is this the right fix?

Claudio sent me some small updates to the patch; updated
version attached.

Tested, passes all regression tests on MingW.

//Magnus

Applied.

---------------------------------------------------------------------------

Bruce Momjian wrote:

Bruce Momjian wrote:

Magnus Hagander wrote:

Now, I still twist my head around the lines:
if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0
||
(fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd, fileFlags &

(O_TEXT

| O_BINARY)) < 0)))

Without having studied it closely, it might also highlight a bug

on

failure of the second clause -- if the _setmode fails, shouldn't
_close be called instead of CloseHandle, and -1 returned?
(CloseHandle would still be called on failure of the

_open_osfhandle,

obviously)

I agree that this code is both wrong and unreadable (although in
practice the _setmode will probably never fail, which is why our
attention hasn't been drawn to it). Is someone going to submit a
patch? I'm hesitant to change the code myself since I'm not in a
position to test it.

I can look at fixing that. Is it something we want to do for 8.2, or
wait until 8.3? If there's a hidden bug, I guess 8.2?

Magnus, is this the right fix?

Claudio sent me some small updates to the patch; updated version
attached.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

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

[ text/x-diff is unsupported, treating like TEXT/PLAIN ]

Index: src/port/open.c
===================================================================
RCS file: /cvsroot/pgsql/src/port/open.c,v
retrieving revision 1.15
diff -c -c -r1.15 open.c
*** src/port/open.c	24 Sep 2006 17:19:53 -0000	1.15
--- src/port/open.c	3 Oct 2006 01:16:43 -0000
***************
*** 105,113 ****
}

/* _open_osfhandle will, on error, set errno accordingly */
! if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0 ||
! (fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0)))
CloseHandle(h); /* will not affect errno */
return fd;
}

--- 105,119 ----
}

/* _open_osfhandle will, on error, set errno accordingly */
! 	if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0)
CloseHandle(h);			/* will not affect errno */
+ 	else if (fileFlags & (O_TEXT | O_BINARY) &&
+ 		_setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0)
+ 	{
+ 		_close(fd);
+ 		return -1;
+ 	}
+ 
return fd;
}

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

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

Tom Lane wrote:

"Magnus Hagander" <mha@sollentuna.net> writes:

That is part of the original open() code that Claudio did back for 8.0,
so it has definitly been working since then.

Hm, maybe best not to touch it, but still...

I haven't really read into
the code, though... But a qiuck look doesn't show me any place wher eit
does ignore O_EXCL - which combination would that be?

What's bugging me is that 0 and O_EXCL give the same answer, and
O_TRUNC and O_TRUNC | O_EXCL give the same answer, but O_CREAT and
O_CREAT | O_EXCL give different answers, as do O_CREAT | O_TRUNC
and O_CREAT | O_TRUNC | O_EXCL. I'm also pretty suspicious of
both O_CREAT | O_EXCL and O_CREAT | O_TRUNC | O_EXCL giving the
same answer. However, I have no idea what the semantics are of
the symbols the function is mapping into, so maybe it's OK.

I am CC'ing Claudio Natoli on this question about
open.c::openFlagsToCreateFileFlags(), but in looking at the code, it
seems OK because:

o O_EXCL doesn't have any meaning unless O_CREAT is used

o O_TRUNC has no meaning when O_CREAT | O_EXCL are used
because you are guaranteed to be creating a new file

Claudio, the function is here, at the top of the file:

http://developer.postgresql.org/cvsweb.cgi/pgsql/src/port/open.c?rev=1.18;content-type=text%2Fx-cvsweb-markup

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

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

I got a private email from Claudio saying he already answered this
question:

http://archives.postgresql.org/pgsql-patches/2006-09/msg00285.php

I forgot I saw it, so I have added comments to the C code.

---------------------------------------------------------------------------

bruce wrote:

Tom Lane wrote:

"Magnus Hagander" <mha@sollentuna.net> writes:

That is part of the original open() code that Claudio did back for 8.0,
so it has definitly been working since then.

Hm, maybe best not to touch it, but still...

I haven't really read into
the code, though... But a qiuck look doesn't show me any place wher eit
does ignore O_EXCL - which combination would that be?

What's bugging me is that 0 and O_EXCL give the same answer, and
O_TRUNC and O_TRUNC | O_EXCL give the same answer, but O_CREAT and
O_CREAT | O_EXCL give different answers, as do O_CREAT | O_TRUNC
and O_CREAT | O_TRUNC | O_EXCL. I'm also pretty suspicious of
both O_CREAT | O_EXCL and O_CREAT | O_TRUNC | O_EXCL giving the
same answer. However, I have no idea what the semantics are of
the symbols the function is mapping into, so maybe it's OK.

I am CC'ing Claudio Natoli on this question about
open.c::openFlagsToCreateFileFlags(), but in looking at the code, it
seems OK because:

o O_EXCL doesn't have any meaning unless O_CREAT is used

o O_TRUNC has no meaning when O_CREAT | O_EXCL are used
because you are guaranteed to be creating a new file

Claudio, the function is here, at the top of the file:

http://developer.postgresql.org/cvsweb.cgi/pgsql/src/port/open.c?rev=1.18;content-type=text%2Fx-cvsweb-markup

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

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

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

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