# Opinion needed (bug #45928)

- Thread: https://externals.io/message/40400
- Original archive: https://news-web.php.net/php.internals/40400
- Started by Alexey Zakhlestin on 2008-09-09T12:39:05+00:00
- Messages: 7
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #40400 — Alexey Zakhlestin — 2008-09-09T12:39:05+00:00

URL: https://externals.io/message/40400#40400

http://bugs.php.net/bug.php?id=45928

Christian Schneider wrote:

> I had a quick look at this bug and found the problem to be in
> Zend/zend_stream.c function zend_stream_fsize(): It uses `fstat()` to
> determine the filesize which on MacOS X for pipes returns either 0 (my
> interpretation: no data from the pipe ready yet) or a number up to 16384
> (my interpretation: data from the pipe ready but the maximum buffer size
> is 16k).
>
> I see several solutions but I'm not sure which is the desired one:
> - return 0 (size unknown) if the file is a pipe (or socket, ...)
> - return 0 if the file is not a regular file (or symlink, dir?)
> - look into a way of determining EOF reached
>
> As a quick test I changed
>     return buf.st_size;
> in function zend_stream_fsize() to
>     return 0;
> and cat 30k.php | php worked after that.
>
> I posted this to internals@php.net but did not get any reply so I'm not
> sure how to proceed.

-- 
Alexey Zakhlestin
http://blog.milkfarmsoft.com/

---

## #40410 — Arnaud Le Blanc — 2008-09-09T14:51:54+00:00

URL: https://externals.io/message/40400#40410 · In reply to: #40400

Hi,

On Tuesday 09 September 2008 14:39:05 Alexey Zakhlestin wrote:
> http://bugs.php.net/bug.php?id=45928
> 
> Christian Schneider wrote:
> 
> > […]

It seems to be a good solution. zend_stream seems to already rely on size == 0 
for non-regular files and falls back to something like "while (!feof){ read }" 
in that case.

The following may (no MacOS X to test) fix the problem by returning 0 from 
zend_stream_fsize() when the file descriptor is not a regular file:
http://arnaud.lb.s3.amazonaws.com/45928.patch

Regards,

Arnaud

---

## #40412 — Christian Schneider — 2008-09-09T15:35:54+00:00

URL: https://externals.io/message/40400#40412 · In reply to: #40410

Arnaud Le Blanc wrote:
> The following may (no MacOS X to test) fix the problem by returning 0 from 
> zend_stream_fsize() when the file descriptor is not a regular file:
> http://arnaud.lb.s3.amazonaws.com/45928.patch

Your patch:
+#ifdef S_ISREG
+		if (!S_ISREG(buf.st_mode)) {
+			return 0;
+		}
+#endif
might prevent mmap-ing symbolically linked files, I'll do a quick test
when I'm at home.
Not sure if we should also check for symbolic links (and what they point
to) or if that's overkill because the performance penalty is small
enough anyway.

Anyone from the core developers got an opinion here?

- Chris

---

## #40414 — Scott MacVicar — 2008-09-09T15:44:34+00:00

URL: https://externals.io/message/40400#40414 · In reply to: #40412

Christian Schneider wrote:
> Arnaud Le Blanc wrote:
> > […]
> 
> Your patch:
> +#ifdef S_ISREG
> +		if (!S_ISREG(buf.st_mode)) {
> +			return 0;
> +		}
> +#endif
> might prevent mmap-ing symbolically linked files, I'll do a quick test
> when I'm at home.
> Not sure if we should also check for symbolic links (and what they point
> to) or if that's overkill because the performance penalty is small
> enough anyway.
> 
> Anyone from the core developers got an opinion here?
> 
> - Chris
> 

Check for symbolic links, it could potentially come up if you have a
common file.

The fsize code was written and tested on a Mac but I admit I was testing
with interactive mode and whole files. I can test this later on tonight
if I get time and if its good I'll commit it.

Scott

---

## #40415 — Arnaud Le Blanc — 2008-09-09T15:59:39+00:00

URL: https://externals.io/message/40400#40415 · In reply to: #40412

Hi,

On Tuesday 09 September 2008 17:35:54 Christian Schneider wrote:
> Arnaud Le Blanc wrote:
> > […]
> 
> Your patch:
> +#ifdef S_ISREG
> +		if (!S_ISREG(buf.st_mode)) {
> +			return 0;
> +		}
> +#endif
> might prevent mmap-ing symbolically linked files, I'll do a quick test
> when I'm at home.

No, `fstat()` will resolve the link and stat the file the link points to. And I 
believe script paths are fully resolved before being opened.

Anyway, given the `fstat()` Linux man page, its a bug to return st_size when the 
file is not a regular file.

> Not sure if we should also check for symbolic links (and what they point
> to) or if that's overkill because the performance penalty is small
> enough anyway.
> 
> Anyone from the core developers got an opinion here?
> 
> - Chris
> 

Regards,

Arnaud

---

## #40430 — Christian Schneider — 2008-09-10T09:25:40+00:00

URL: https://externals.io/message/40400#40430

Am 10.09.2008 um 10:52 schrieb Tullio Andreatta ML:
> fstat on a "open-ed fd" can't stat a symbolic link, since  
> open("symlink") returns a file descriptor of the target file, or -1  
> if it's a dangling link ...


Yes, that's right. So Arnaud's patch at http://arnaud.lb.s3.amazonaws.com/45928.patch 
  looks fine to me.
Someone with karma should commit this...

One more thought: Maybe the same patch should also be applied to  
function zend_stream_stdio_fsizer() in the same file?

- Chris

---

## #40432 — Arnaud Le Blanc — 2008-09-10T10:35:02+00:00

URL: https://externals.io/message/40400#40432 · In reply to: #40430

On Wednesday 10 September 2008 11:25:40 Christian Schneider wrote:
> Am 10.09.2008 um 10:52 schrieb Tullio Andreatta ML:
> > […]
> 
> 
> Yes, that's right. So Arnaud's patch at 
http://arnaud.lb.s3.amazonaws.com/45928.patch 
>   looks fine to me.
> Someone with karma should commit this...
> 
> One more thought: Maybe the same patch should also be applied to  
> function zend_stream_stdio_fsizer() in the same file?

Committed, applied to zend_stream_stdio_fsizer() too.

> 
> - Chris