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

Tue Mar 25 06:42:40 UTC 2014

Poonam,

Your proposal looks good for me. Thank you for doing it.

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
>>>>>>>
>>>>
>>

-- 
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.