include bug in 5.3 View as Markdown

Hi Dmitry,

Hi,

The attached patch is going to fix the problem.
It implements its own realpath() function, so we won't depend on system
anymore. It also improve realpath cache usage by caching intermediate
results.

I tested it on Linux and Windows only and it seems to work without
problems. It breaks one test related to clearstatcache() function, but
this break is expected.

Could you please test it.

I can test it asap on various windows, probably on Monday.

After a quick review, it seems that FindClose is missing in the two
buffer overflow check.

•                           state->cwd_length += ptr_length;
  

+#ifdef TSRM_WIN32

•           if ((hFind = FindFirstFile(to, &data)) != INVALID_HANDLE_VALUE) {
  

+fprintf(stderr, "1 %s (%d)\n", to, len);

•                   j = strlen(data.cFileName);
  

•                   i = len;
  

•                   while (!IS_SLASH(to[i-1])) {
  

•                           i--;
  

•                   }
  

•                   if (i + j >= MAXPATHLEN-1) {
  

•                           /*buffer overflow */
  

FindClose(hFind);

•                           return NULL;
  

•                   }
  

•                   if (CWDG(realpath_cache_size_limit)) {
  

•                           memcpy(tmp, to, i);
  

•                           memcpy(tmp + i, data.cFileName, j + 1);
  

•                           realpath_cache_add(to, len, tmp, i + j, t TSRMLS_CC);
  

•                   }
  

•                   ret -= (j + 1);
  

•                   if (ret < buf) {
  

•                           /* buffer overflow */
  

FindClose(hFind);

•                           return NULL;
  

•                   }
  

•                   *ret = DEFAULT_SLASH;
  

•                   memcpy(ret+1, data.cFileName, j);
  

•                   FindClose(hFind);
  

•                   len = i - 1;
  

•                   to[len] = 0;
  

+fprintf(stderr, "2 %s - %s\n", to, ret);

•                   continue;
  

•           } else {
  

•                   if (use_realpath == CWD_REALPATH) {
  

•                           return NULL;
  

•                   } else {
  

•                           goto next_dir;
                   }
  

•                   ptr = tsrm_strtok_r(NULL, TOKENIZER_STRING, &tok);
           }
  

Thanks for your work!

Pierre

http://blog.thepimp.net | http://www.libgd.org

Hi,

On Friday 08 August 2008 17:52:26 Dmitry Stogov wrote:

Hi,

The attached patch is going to fix the problem.
It implements its own realpath() function, so we won't depend on system
anymore. It also improve realpath cache usage by caching intermediate
results.

I tested it on Linux and Windows only and it seems to work without
problems. It breaks one test related to clearstatcache() function, but
this break is expected.

Could you please test it.
Does it really fix the bug on FreeBSD?

Yes, it fixes the bug on FreeBSD :)

Thanks. Dmitry.

Hannes Magnusson wrote:

virtual_file_ex() assumes that realpath() returns NULL when the file
does not exists. But BSD's realpath() will not return NULL if all
components but the last exist. So realpath(/foo/bar) will return
/foo/bar even if bar does not exists.

And that behavior is intended, but is sadly overwritten in ZTS :(

-Hannes

Regards,

Arnaud

Just semantics but I'd name the function 'tsrm_realpath[_r]' to be absolutely
clear what it does.

--Jani

Dmitry Stogov kirjoitti:

Hi,

The attached patch is going to fix the problem.
It implements its own realpath() function, so we won't depend on system
anymore. It also improve realpath cache usage by caching intermediate
results.

I tested it on Linux and Windows only and it seems to work without
problems. It breaks one test related to clearstatcache() function, but
this break is expected.

Could you please test it.
Does it really fix the bug on FreeBSD?

Thanks. Dmitry.

Hannes Magnusson wrote:

virtual_file_ex() assumes that realpath() returns NULL when the file does not
exists. But BSD's realpath() will not return NULL if all
components but the last exist. So realpath(/foo/bar) will return /foo/bar
even if bar does not exists.
And that behavior is intended, but is sadly overwritten in ZTS :(

-Hannes

Dmitry Stogov wrote:

Hi,

The attached patch is going to fix the problem.
It implements its own realpath() function, so we won't depend on system
anymore. It also improve realpath cache usage by caching intermediate
results.

Very nice. The intermediate caching is going to drastically reduce the
amount of memory we need for the cache. I have seen a number of cases
where sites had to increase the realpath cache size by quite a bit.

-Rasmus

Hi Dmitry

Hi,

The attached patch is going to fix the problem.

Works also on my OS X 10.4 box now. Thanks a lot for the effort (and if
it brings a performance boost, even better :) )

chregu

It implements its own realpath() function, so we won't depend on system
anymore. It also improve realpath cache usage by caching intermediate
results.

I tested it on Linux and Windows only and it seems to work without
problems. It breaks one test related to clearstatcache() function, but
this break is expected.

Could you please test it.
Does it really fix the bug on FreeBSD?

Thanks. Dmitry.

Hannes Magnusson wrote:

virtual_file_ex() assumes that realpath() returns NULL when the file does not
exists. But BSD's realpath() will not return NULL if all
components but the last exist. So realpath(/foo/bar) will return /foo/bar
even if bar does not exists.
And that behavior is intended, but is sadly overwritten in ZTS :(

-Hannes

--
New Office Zurich Opening: http://liip.to/party

Liip AG // Feldstrasse 133 // CH-8004 Zurich
Tel +41 43 500 39 81 // Mobile +41 76 561 88 60
www.liip.ch // blog.liip.ch // GnuPG 0x5CE1DECB

Hi,

The attached patch is going to fix the problem.
It implements its own realpath() function, so we won't depend on system
anymore. It also improve realpath cache usage by caching intermediate
results.

I tested it on Linux and Windows only and it seems to work without
problems. It breaks one test related to clearstatcache() function, but
this break is expected.

Could you please test it.
Does it really fix the bug on FreeBSD?

Yup. But it introduces a new one.
realpath() no longer works as documented, now requiring all the
components to exist.

-Hannes

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello Dmitry,

while you are at fixing realpath() it might be a good idea to fix the
../ nonsense.

What I mean is:

fopen("this_is_not_a_dir_but_a_file/../../../../../../../../etc/passwd",
"r");

works because of realpath() and PHP's wrapper.

Same for

fopen("this_is_not_existing/../../../../../../../../etc/passwd", "r");

Both is madness...

Stefan

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEUEARECAAYFAkidgR0ACgkQSuF5XhWr2nhovACXZpeATBITDai/M1wsCuavuZ3C
OgCgn46uM4XHwENW7si4aJzeNgnuTKg=
=QiYy
-----END PGP SIGNATURE