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

[David Holmes]

On 25/03/2014 4:42 PM, Dmitry Samersoff wrote:
> Poonam,
>
> Your proposal looks good for me. Thank you for doing it.

I concur. It is a good compromise to allow forward progress.

Thanks,
David

>
> On 2014-03-25 08:12, Poonam Bajaj wrote:
>> 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.
>
> -Dmitry
>
>
>
>>
>> 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
>>>>>>>>
>>>>>
>>>
>
>