# I want to work against Bug 44522 - Upload limit 2G - Thread: https://externals.io/message/67923 - Original archive: https://news-web.php.net/php.internals/67923 - Started by Ralf Lang on 2013-06-27T08:21:32+00:00 - Messages: 23 - 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 --- ## #67923 — Ralf Lang — 2013-06-27T08:21:32+00:00 URL: https://externals.io/message/67923#67923 Hi, I want to deal with Bug 44522 which disallows uploads > 2G https://bugs.php.net/bug.php?id=44522&edit=1 Today with PHP cloud solutions, we are running into more situations where this really hurts. Before I begin providing a github patch (as I have no php svn access and I don't think it's needed) against master and possibly 5.5.next: Does anybody know of intricate reasons why the existing patch https://bugs.php.net/patch-display.php?bug_id=44522&patch=uploads_larger_than_2g_HEAD_v2&revision=latest could not be included into php 5.5? Or is it only what stas said in 2012: "The patch however needs to be cleaned up (no IGNORE vars, etc.) and changing signature for zend_atoi may not be safe if any code out there presumes it returns int (integer overflow). Also, no reason to use signed long there where we used unsigned long." This would be my first patch against php core. Any pointers apreciated. -- Ralf Lang Linux Consultant / Developer Tel.: +49-170-6381563 Mail: lang@b1-systems.de B1 Systems GmbH Osterfeldstraße 7 / 85088 Vohburg / http://www.b1-systems.de GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt,HRB 3537 --- ## #67968 — johannes@schlueters.de — 2013-06-27T16:30:13+00:00 URL: https://externals.io/message/67923#67968 · In reply to: #67923 Hi, > Hi, > > I want to deal with Bug 44522 which disallows uploads > 2G > https://bugs.php.net/bug.php?id=44522&edit=1 Great! > Today with PHP cloud solutions, we are running into more situations > where this really hurts. Yay cloud :-D (i don't see why cloud matters, here though it's more about modern bandwidth and storage space) > Before I begin providing a github patch (as I have no php svn access and > I don't think it's needed) against master and possibly 5.5.next: > > Does anybody know of intricate reasons why the existing patch > > https://bugs.php.net/patch-display.php?bug_id=44522&patch=uploads_larger_than_2g_HEAD_v2&revision=latest > > could not be included into php 5.5? Most likely since nobody cared enough and pushed it. (both, pushed in the tree and pushed as in making people aware) Unfortunately the patch can't make it in 5.5. It changes the size and offsets in the sapi globals which breaks binary compatibility which we can't do in a dot release. Has to be 5.6/PHP.next. I haven't looked close enough to comment on the patch itself. johannes --- ## #67970 — Ralf Lang — 2013-06-27T16:33:31+00:00 URL: https://externals.io/message/67923#67970 · In reply to: #67968 > Yay cloud :-D > (i don't see why cloud matters, here though it's more about modern > bandwidth and storage space) Just did not want to name the product which prompted me to finally go at it. ;) > > […] > > Most likely since nobody cared enough and pushed it. (both, pushed in > the tree and pushed as in making people aware) > > Unfortunately the patch can't make it in 5.5. It changes the size and > offsets in the sapi globals which breaks binary compatibility which we > can't do in a dot release. Has to be 5.6/PHP.next. > > I haven't looked close enough to comment on the patch itself. OK, no 5.5 then. Anyway, I have built a version of the patch (using unsigned long instead of signed long as the original did) against old php 5.3.8 and currently a test is running with a 4.7 GiB DVD Image. If it works well, I will submit the patch and attach it to the bug record. Thank you for your response. -- Ralf Lang Linux Consultant / Developer Tel.: +49-170-6381563 Mail: lang@b1-systems.de B1 Systems GmbH Osterfeldstraße 7 / 85088 Vohburg / http://www.b1-systems.de GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt,HRB 3537 --- ## #67973 — Christopher Jones — 2013-06-27T16:52:27+00:00 URL: https://externals.io/message/67923#67973 · In reply to: #67970 > Anyway, I have built a version of the patch (using unsigned long instead > of signed long as the original did) against old php 5.3.8 and currently > a test is running with a 4.7 GiB DVD Image. If it works well, I will > submit the patch and attach it to the bug record. > > Thank you for your response. > To make it more reviewable, can you base the patch on the master branch (aka PHP 5.6)? If you're a github user, a PR would be great. Some supporting documentation about what the patch changes, what platforms it was tested on etc would be helpful. I think a brief RFC would be the best place to record the details: https://wiki.php.net/rfc/howto Did you review all the previous mail list discussion about the patch? IIRC there were a bunch of reasons it never got merged. Chris -- christopher.jones@oracle.com http://twitter.com/ghrd Free PHP & Oracle book: http://www.oracle.com/technetwork/topics/php/underground-php-oracle-manual-098250.html --- ## #67976 — johannes@schlueters.de — 2013-06-27T17:00:40+00:00 URL: https://externals.io/message/67923#67976 · In reply to: #67973 > > Did you review all the previous mail list discussion about the patch? > IIRC there were a bunch of reasons it never got merged. what we never merged for reasons is large file support. So 32bit machines can use files >2G. The patches there open a can of worm with libraries/environments we link to and where we might exchange file pointers. Besides the binary compatibility and required review and testing there is no such reason here. The biggest issue here is that people might be careful about changing request handling as that's quite central to PHP and a bug there can lead to remote exploitable bugs, but doing this early, like now for 5.6, should be fine. joahnnes --- ## #67985 — Yasuo Ohgaki — 2013-06-27T21:47:23+00:00 URL: https://externals.io/message/67923#67985 · In reply to: #67923 Hi Just a comment for the patch. 2013/6/27 Ralf Lang > Does anybody know of intricate reasons why the existing patch > > > https://bugs.php.net/patch-display.php?bug_id=44522&patch=uploads_larger_than_2g_HEAD_v2&revision=latest > > could not be included into php 5.5? > It uses long instead of uint but it makes large files available only to 64bit platforms. It should use off_t along with "#define _FILE_OFFSET_BITS 64", but I'm not sure if this work under windows. There may be other places that data type should be changed. It should be checked carefully, otherwise memory fault may occur due to integer overflow. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #67986 — Anthony Ferrara — 2013-06-27T22:13:39+00:00 URL: https://externals.io/message/67923#67986 · In reply to: #67985 To be fair, the overall limit is going to be 32 bit anyway (due to the use of int for string lengths)... I'm working on a patch to replace all references of string sizes with size_t, but as you can imagine, it's going to take some time... Just throwing it out there that a better fix may be coming down the road anyway. I'll post on-list in the coming days about this... Anthony > Hi > > Just a comment for the patch. > > 2013/6/27 Ralf Lang > > > […] > https://bugs.php.net/patch-display.php?bug_id=44522&patch=uploads_larger_than_2g_HEAD_v2&revision=latest > > […] > > It uses long instead of uint but it makes large files available only to > 64bit platforms. > > It should use off_t along with "#define _FILE_OFFSET_BITS 64", but I'm not > sure if this work under windows. There may be other places that data type > should be changed. It should be checked carefully, otherwise memory fault > may occur due to integer overflow. > > Regards, > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net --- ## #67987 — Rasmus Lerdorf — 2013-06-27T22:21:50+00:00 URL: https://externals.io/message/67923#67987 · In reply to: #67986 > To be fair, the overall limit is going to be 32 bit anyway (due to the use > of int for string lengths)... For file uploads? Why? We never hold the entire file in a string. -Rasmus --- ## #67997 — Laruence — 2013-06-28T05:12:03+00:00 URL: https://externals.io/message/67923#67997 · In reply to: #67923 Hey: Instead of change the uint to long or size_t, Maybe make the max_file_upload_size 0 means unlimited? like Apached did, ulimited or <= 2Gb https://httpd.apache.org/docs/2.2/mod/core.html#limitrequestbody thanks > Hi, > > I want to deal with Bug 44522 which disallows uploads > 2G > https://bugs.php.net/bug.php?id=44522&edit=1 > > Today with PHP cloud solutions, we are running into more situations > where this really hurts. > > Before I begin providing a github patch (as I have no php svn access and > I don't think it's needed) against master and possibly 5.5.next: > > Does anybody know of intricate reasons why the existing patch > > https://bugs.php.net/patch-display.php?bug_id=44522&patch=uploads_larger_than_2g_HEAD_v2&revision=latest > > could not be included into php 5.5? > > Or is it only what stas said in 2012: > > "The patch however > needs to be cleaned up (no IGNORE vars, etc.) and changing signature for > zend_atoi may not be safe if any code out there presumes it returns int > (integer > overflow). Also, no reason to use signed long there where we used > unsigned long." > > This would be my first patch against php core. Any pointers apreciated. > > > -- > Ralf Lang > Linux Consultant / Developer > Tel.: +49-170-6381563 > Mail: lang@b1-systems.de > B1 Systems GmbH > Osterfeldstraße 7 / 85088 Vohburg / http://www.b1-systems.de > GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt,HRB 3537 > -- Laruence Xinchen Hui http://www.laruence.com/ --- ## #67998 — Laruence — 2013-06-28T05:16:01+00:00 URL: https://externals.io/message/67923#67998 · In reply to: #67997 > Hey: > > Instead of change the uint to long or size_t, Maybe make the > max_file_upload_size 0 means unlimited? like Apached did, ulimited or > <= 2Gb s ,Gb,GB, > > https://httpd.apache.org/docs/2.2/mod/core.html#limitrequestbody > > thanks > > > > […] > > > > -- > Laruence Xinchen Hui > http://www.laruence.com/ -- Laruence Xinchen Hui http://www.laruence.com/ --- ## #68000 — Ralf Lang — 2013-06-28T06:42:54+00:00 URL: https://externals.io/message/67923#68000 · In reply to: #67997 > Hey: > > Instead of change the uint to long or size_t, Maybe make the > max_file_upload_size 0 means unlimited? like Apached did, ulimited or > <= 2Gb > > https://httpd.apache.org/docs/2.2/mod/core.html#limitrequestbody This doesn't change the behaviour that the user sets a size and is not notified that indeed a much lower (overflowed) size is configured. -- Ralf Lang Linux Consultant / Developer Tel.: +49-170-6381563 Mail: lang@b1-systems.de B1 Systems GmbH Osterfeldstraße 7 / 85088 Vohburg / http://www.b1-systems.de GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt,HRB 3537 --- ## #68001 — Ralf Lang — 2013-06-28T06:45:18+00:00 URL: https://externals.io/message/67923#68001 · In reply to: #67973 > > > > > > […] > > To make it more reviewable, can you base the patch on the master > branch (aka PHP 5.6)? If you're a github user, a PR would be great. Done that. https://github.com/php/php-src/pull/372 > Did you review all the previous mail list discussion about the patch? > IIRC there were a bunch of reasons it never got merged. I read the bug report discussion, stas' mails and what I found in the archives. I might have missed information though. -- Ralf Lang Linux Consultant / Developer Tel.: +49-170-6381563 Mail: lang@b1-systems.de B1 Systems GmbH Osterfeldstraße 7 / 85088 Vohburg / http://www.b1-systems.de GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt,HRB 3537 --- ## #68049 — Ralf Lang — 2013-07-03T17:38:20+00:00 URL: https://externals.io/message/67923#68049 · In reply to: #68001 > > > […] > > Done that. > https://github.com/php/php-src/pull/372 > > > […] > > I read the bug report discussion, stas' mails and what I found in the > archives. I might have missed information though. Any additional action required from my side or is it just waiting for a review timeslot? -- Ralf Lang Linux Consultant / Developer Tel.: +49-170-6381563 Mail: lang@b1-systems.de B1 Systems GmbH Osterfeldstraße 7 / 85088 Vohburg / http://www.b1-systems.de GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt,HRB 3537 --- ## #68358 — Michael Wallner — 2013-08-02T06:59:50+00:00 URL: https://externals.io/message/67923#68358 · In reply to: #68049 > Any additional action required from my side or is it just waiting for a > review timeslot? I'll soon be able to merge/align it with a solution that's been running in production for years, just give me a few days. Thank you for your patience! -- Regards, Mike --- ## #68379 — Michael Wallner — 2013-08-05T12:05:17+00:00 URL: https://externals.io/message/67923#68379 · In reply to: #68358 I have added a simple test case for Linux to verify it's basic functionality via the CLI server, and think it's ready to be merged to master to be able to test it within a wider audience. Objections, anyone? https://github.com/m6w6/php-src/compare/2Guploads Thank you Ralf! -- Regards, Mike --- ## #68380 — Michael Wallner — 2013-08-05T13:58:12+00:00 URL: https://externals.io/message/67923#68380 · In reply to: #68379 > I have added a simple test case for Linux to verify it's basic > functionality via the CLI server, and think it's ready to be merged to > master to be able to test it within a wider audience. > > Objections, anyone? > https://github.com/m6w6/php-src/compare/2Guploads Johannes reminded me, that we don't have C99 stdint portable typedefs in a central PHP header file available yet. -- Regards, Mike --- ## #68381 — Pierre Joye — 2013-08-05T14:19:14+00:00 URL: https://externals.io/message/67923#68381 · In reply to: #68380 Hi Mike, > > > > […] > > Johannes reminded me, that we don't have C99 stdint portable typedefs > in a central PHP header file available yet. In 5.3+ it is easier. Other parts of php deals with that. M4 Macros are there for unices, on windows you have php_stdint (which transparently includes stdint with vc11 and up). > -- > Regards, > Mike --- ## #68386 — Michael Wallner — 2013-08-05T18:34:46+00:00 URL: https://externals.io/message/67923#68386 · In reply to: #68381 > Hi Mike, > > > > […] > > In 5.3+ it is easier. Other parts of php deals with that. M4 Macros are > there for unices, on windows you have php_stdint (which transparently > includes stdint with vc11 and up). Yes, Anatol gave quite a few comments on the PR, so I knew about win32/php_stdint.h, but I was tricked by my memory about a globally (PHP-wide) available portability header :) Thanks for the heads up though! -- Regards, Mike --- ## #68393 — Michael Wallner — 2013-08-06T21:00:08+00:00 URL: https://externals.io/message/67923#68393 · In reply to: #68386 > > > […] > > Yes, Anatol gave quite a few comments on the PR, so I knew about > win32/php_stdint.h, but I was tricked by my memory about a globally > (PHP-wide) available portability header :) Alright, I rebased the 2Gupload branch against stdint: https://github.com/m6w6/php-src/compare/2Guploads https://github.com/m6w6/php-src/compare/stdint A test build on more exotic platforms would be appreciated -- I'm working on setting some up for me. Thank you. -- Regards, Mike --- ## #68439 — Michael Wallner — 2013-08-08T12:42:43+00:00 URL: https://externals.io/message/67923#68439 · In reply to: #68393 > > Alright, I rebased the 2Gupload branch against stdint: > https://github.com/m6w6/php-src/compare/2Guploads > https://github.com/m6w6/php-src/compare/stdint So, is everyone fine with it so far? -- Regards, Mike --- ## #68440 — Ralf Lang — 2013-08-08T12:44:38+00:00 URL: https://externals.io/message/67923#68440 · In reply to: #68439 > > > […] > > So, is everyone fine with it so far? > I'm fine with this. Juggling around with types to get a crossplatform 64bit type was not so easy for me. -- Ralf Lang Linux Consultant / Developer Tel.: +49-170-6381563 Mail: lang@b1-systems.de B1 Systems GmbH Osterfeldstraße 7 / 85088 Vohburg / http://www.b1-systems.de GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt,HRB 3537 --- ## #68441 — Kalle Sommer Nielsen — 2013-08-08T12:44:46+00:00 URL: https://externals.io/message/67923#68441 · In reply to: #68439 Hi Michael 2013/8/8 Michael Wallner : > > > […] > > So, is everyone fine with it so far? I think it is a long overdue that we have not have had this patch or similar committed, so I would say go a head and commit it to master. -- regards, Kalle Sommer Nielsen kalle@php.net --- ## #68450 — Michael Wallner — 2013-08-08T18:54:19+00:00 URL: https://externals.io/message/67923#68450 · In reply to: #68441 > Hi Michael > > 2013/8/8 Michael Wallner : > > […] > > I think it is a long overdue that we have not have had this patch or > similar committed, so I would say go a head and commit it to master. Thought so. I'll merge tomorrow. -- Regards, Mike