Everything

php

Windows SAPI possible binary file operation bug and possible fix

php.internals

Pierre Joye

19 years ago

Hello,

While debugging a zip bug (http://pecl.php.net/bugs/bug.php?id=9278),
I was wondering what's the difference between CLI/CGI and ISAPI or
Apache2. The same code works perfectly in CLI and CGI on windows and
using all SAPIs on linux (apache1 or 2, cli, cgi). It was not that
obvious, even after having read the MS manual.

The problem seems to be the default mode for the file operation and
the binary mode being ignored by windows ("rb" or "wb").

We only force the default mode in Embed, CLI and CGI. I would suggest
to do it as well in all SAPIs:
./sapi/cgi/cgi_main.c:1042:     _fmode = _O_BINARY; /* sets default
for file streams to binary */
./sapi/cli/php_cli.c:626:       _fmode = _O_BINARY;
/*sets default for file streams to binary */
./sapi/embed/php_embed.c:157:  _fmode = _O_BINARY;
/*sets default for file streams to binary */

_fmode is global per process. We should set the mode of stdin/out:

_setmode( _fileno( stdin ), _O_BINARY );

If we need it and if it is already opened when we init the SAPI, but
someone has to verify this point.

I do not have the time to test it now, but it should also solve a
couple of other issues in HEAD.  I can write a patch and commit if we
agree on this fix.

Thanks for your upcoming comments,

--Pierre
[1] http://msdn2.microsoft.com/en-us/library/z5hh6ee9(VS.80).aspx

Pierre Joye

19 years ago

On 11/15/06, Pierre <pierre.php@gmail.com> wrote:

> _fmode is global per process. We should set the mode of stdin/out:
>
> _setmode( _fileno( stdin ), _O_BINARY );
>
> If we need it and if it is already opened when we init the SAPI, but
> someone has to verify this point.

I just checked it, we do set the mode for stderr, in and out.

--Pierre

Pierre Joye

19 years ago

Hi Dmitry,

> I think that changing global behavior (_fmode = _O_BINARY;) is not a good
> idea.
> May be this code should be removed from CGI and CLI too.

A big -1. It works and is known to work. It has been there since years.

> May be we have a bug somewhere and _O_BINARY is not passed to open().

It is a knonw issue on windows (which I did not know). You can check
other projects, they all force the mode using _fmode and _setmode for
std*. As CGI/CLI have proven, it works well with the mode forced to
binary. Or do you have informations telling that we should change that
in CGI/CLI and embed?

> Do you think the bug is in CRT?

It is a broken behavior somewhere in the windows API. As far as I can
see, it is not in the public part, I run my code until I reached the
ASM level in the debugger but the mode was correct (try codesearch).

> What VC version do you use?

It is a bug in php releases (vc6), and in all my attempts (vc express and 2003).

Unless there is a good reason to do not force the mode, I'm in favour
to do it as well in the other SAPI. We do it in FCGI and CLI which the
most used SAPI on windows, I don't think it is a good idea to
introduce bugs by changing the default mode.

I can fix this problem only in zip (like I have to do it for PECL) by
initializing the mode when I enter a zip function and restore it on
leaving. But it will be a pita and will not solve this problem for any
other extensions relying on the same windows function.

--Pierre

Wez Furlong

19 years ago

We cannot change it in SAPI where we are not the .exe because we don't
own the process.
Messing with that flag affects the entire process and could break
things at a higher level.

I call this kind of thing "library abuse", where one piece of code
assumes that it is the only consumer of the library.  In a modular
application, this is not the case.

I think your actual bug is elsewhere, and that the binary flag isn't
being explicitly set on a stdio file handle somewhere in the zip
library code.

I'm -1 on making this change to the other SAPI.

Incidentally, using stdio is not portable in a server application that
might have more than 255 open files at one time.  Best to use the
streams layer throughout if possible.

--Wez.

On 11/15/06, Pierre <pierre.php@gmail.com> wrote:

Pierre Joye

19 years ago

Hello,

On 11/15/06, Wez Furlong <kingwez@gmail.com> wrote:

> We cannot change it in SAPI where we are not the .exe because we don't
> own the process.
> Messing with that flag affects the entire process and could break
> things at a higher level.
>
> I call this kind of thing "library abuse", where one piece of code
> assumes that it is the only consumer of the library.  In a modular
> application, this is not the case.
>
> I think your actual bug is elsewhere, and that the binary flag isn't
> being explicitly set on a stdio file handle somewhere in the zip
> library code.

As I said, it uses "b" everywhere. It is a windows bug. Many other
projects do the same trick to get the problem solved. I'm not sure if
it affects the non binary mode or if it only fixes the binary mode. I
think fopen still works as expected in non binary mode.

> I'm -1 on making this change to the other SAPI.

Why do we use it in FGCI, CLI and embed? The impact on the other
libraries used by the PHP process is the same, even if it does not
affect the server.

> Incidentally, using stdio is not portable in a server application that
> might have more than 255 open files at one time.  Best to use the
> streams layer throughout if possible.

Not possible right now but it is in the Todo, I'm not sure it will fix
the problem either, unless I use parse_mode and php_stream_fopen();

--Pierre

Pierre Joye

19 years ago

Hello,

On 11/15/06, Wez Furlong <kingwez@gmail.com> wrote:

> We cannot change it in SAPI where we are not the .exe because we don't
> own the process.
> Messing with that flag affects the entire process and could break
> things at a higher level.
>
> I call this kind of thing "library abuse", where one piece of code
> assumes that it is the only consumer of the library.  In a modular
> application, this is not the case.

I just commited a better fix. At least it has no impact on other
functions in the same process. However I still think that we have a
problem here, we were only lucky until now :)

--Pierre