[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: Review Request JDK-7119643: It is not possible to read files with a path longer than 2048 charac
From:       Poonam Bajaj <poonam.bajaj () oracle ! com>
Date:       2014-03-25 11:15:10
Message-ID: 5331626E.8020304 () oracle ! com
[Download RAW message or body]

Ok, thanks. I will try to convert the test to a pure java test case and 
will re-submit the review request.

Thanks,
Poonam

On 3/25/2014 3:36 PM, David Holmes wrote:
> 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
>>>>>>>>>
>>>>>>
>>>>
>>
>>
[prev in list] [next in list] [prev in thread] [next in thread]