Review Request JDK-7119643: It is not possible to read files with a path longer than 2048 characters

[Dmitry Samersoff]

David,

> The issue here is to remove the internal limitation. We can't just
> make the replacement you suggest because we will suddenly be
> allocating 4KB stack buffers where they were previously 2KB. That is
> bound to break something.

I share your concern but what we have today[1] is not acceptable. We
have to make decision and either go with our own constant MAX_PATH (2*k)
with clear justification why we don't follow rest of the linux or
use one of system constants: MAXPATHLEN or PAH_MAX.

But impact of this cleanup have to be carefully evaluated and probably
out of scope of this CR.

*As for proposed changes*:

I'm against of dynamic allocation in stack based on user input - it can
lead to stack overflow in some cases and cause intermittent failures
that very hard to debug.

So we should either change this value to MAXPATHLEN and hope that
testing catch all problems or postpone the changes to complete and
careful cleanup.

[1]

jaja:vm#grep -n PATH_MAX os_linux.cpp

2619:  char buf[PATH_MAX+1];

jaja:vm#grep -n MAXPATHLEN os_linux.cpp

379:        char buf[MAXPATHLEN];
2310:static char saved_jvm_path[MAXPATHLEN] = {0};
2315:  if (buflen < MAXPATHLEN) {
2326:  char dli_fname[MAXPATHLEN];
5977:    char buf[MAXPATHLEN];
5978:    char libmawtpath[MAXPATHLEN];

jaja:vm#grep -n MAX_PATH os_linux.cpp

110:#define MAX_PATH    (2 * K)
5025:  char pathbuf[MAX_PATH];
5026:  if (strlen(path) > MAX_PATH - 1) {
5052:  char buf[sizeof(struct dirent) + MAX_PATH];
5075:  if (strlen(path) > MAX_PATH - 1) {
5393:  char filename[MAX_PATH];
5395:    jio_snprintf(filename, MAX_PATH, PauseAtStartupFile);
5397:    jio_snprintf(filename, MAX_PATH, "./vm.paused.%d",
current_process_id());

-Dmitry

On 2014-03-24 16:17, David Holmes wrote:
> Dmitry,
> 
> On 24/03/2014 10:13 PM, Dmitry Samersoff wrote:
>> Poonam,
>>
>> As far as I understand, MAX_PATH is here for a while but later we start
>> using MAXPATHLEN.
>>
>> I would recommend to replace all usage of MAX_PATH and PATH_MAX
>> to MAXPATHLEN
> 
> The issue here is to remove the internal limitation. We can't just make
> the replacement you suggest because we will suddenly be allocating 4KB
> stack buffers where they were previously 2KB. That is bound to break
> something.
> 
> David
> 
>> if you fill that these changes is go out of scope of JDK-7119643, please
>> replace it inside os:stat() only to solve immediate needs and file a
>> separate CR to have it cleaned up.
>>
>> -Dmitry
>>
>>
>> On 2014-03-24 06:55, Poonam Bajaj wrote:
>>> Hi Dmitry,
>>>
>>> On 3/24/2014 12:49 AM, Dmitry Samersoff wrote:
>>>> Poonam,
>>>>
>>>> 1.
>>>>
>>>> MAXPATHLEN == PATH_MAX and it's a system constant that comes from
>>>> sys/param.h and linux/limits.h so current behavior of hotspot is
>>>> absolutely correct.
>>>>
>>>> Moreover, we uses MAXPATHLEN in many places in os_*.cpp files, so this
>>>> patch makes hotspot behavior inconsistent.
>>>>
>>>> Typical value of PATH_MAX is 4096 so it's unclean for me why we have
>>>> problems with 2048 bytes file.
>>>>
>>>> We probably should check our reference build platform.
>>> The purpose of this change is to revert back to the same behavior as it
>>> was before the  fix for 6348631 where we didn't have any path limitation
>>> in os::open() and os::stat() functions.
>>>
>>> But yes, you are right. We have the following in sys/param.h and
>>> linux/limits.h for the maximum allowed path length on linux platforms:
>>> #define PATH_MAX        4096    /* # chars in a path name including
>>> nul */
>>> #define MAXPATHLEN      PATH_MAX
>>>
>>> and MAXPATHLEN is also used in os_linux.cpp. So, if we have to insert
>>> the path limitation in os::open() and os::stat(), we should be using
>>> MAXPATHLEN at all the places in os_linux.cpp. Why was MAX_PATH defined
>>> and to a different value than MAXPATHLEN which is inconsistent with the
>>> linux definition?
>>>
>>>> 2.
>>>>
>>>> Test could be a pure java.
>>> Ok, will do.
>>>
>>> Thanks,
>>> Poonam
>>>>
>>>> -Dmitry
>>>>
>>>> On 2014-03-21 16:52, Poonam Bajaj wrote:
>>>>> Hi,
>>>>>
>>>>> Review request for:
>>>>> JDK-7119643<https://bugs.openjdk.java.net/browse/JDK-7119643>: It is
>>>>> not possible to read files with a path longer than 2048 characters
>>>>>
>>>>> Problem and Fix: This bug is a regression from 6u29 and exists only on
>>>>> Linux platform. With the fix for 6348631, a limit of MAX_PATH (2K) got
>>>>> introduced on the file path length in the os::open() and os::stat()
>>>>> functions in os_linux.cpp. These changes remove that limit. These
>>>>> changes also add a regression test case to check that the files having
>>>>> path longer than 2048 are read without any error.
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~poonam/7119643/webrev.00/
>>>>>
>>>>> This fix needs to go into 9, 8u, 7u and 6u.
>>>>>
>>>>> Thanks,
>>>>> Poonam
>>>>>
>>>>
>>
>>


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the source code.