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

[Poonam Bajaj]

Hi David, Dmitry,

I understand both the concerns:
- By using MAXPATHLEN which is defined as 4096 bytes, we would be 
allocating 4KB stack buffers where they are currently 2KB.
- By removing the path length check in these functions we open the risk 
of stack overflow errors by creating large buffers on the stack as the 
user input may be unreasonably large.

How about I just remove the path length check in os::open() for this bug 
where we are not creating any stack buffers based on the path input 
given by the user, and leave os::stat() as it is where a path buffer is 
created on the stack based on the user input. This bug only requires to 
remove the limitation while opening the files and the change in 
os::open() would fix that.

I will file another bug to take care of the inconsistent use of 
MAXPATHLEN and MAX_PATH in os_linux.cpp file.

Let me know your thoughts.

Thanks,
Poonam

On 3/24/2014 6:36 PM, Dmitry Samersoff wrote:
> 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
>>>>>>
>>>
>