RFR: JDK-8145099 Better error message when SA can't attach to a process

Fri Dec 11 11:32:30 UTC 2015

> On 11 dec. 2015, at 11:54, Dmitry Samersoff <dmitry.samersoff at oracle.com> wrote:
> 
> Staffan,
> 
>> This is all platform specific code - and all the platforms are
>> different. Of course, it would be good if all platforms reported good
>> error message always - and we can continue working on that.
> 
> OK.
> 
>> It should get filled in in the ptrace_attach() call.
> 
> if ph->pid == thr->lwp_id at ll. 409 the function return NULL but
> doesn't touch err_buf.

if ph->pid == thr->lwp_id the if-statement is skipped and we continue in the while-loop.

> 
> -Dmitry
> 
> On 2015-12-11 11:12, Staffan Larsen wrote:
>> 
>>> On 10 dec. 2015, at 23:20, Dmitry Samersoff
>>> <dmitry.samersoff at oracle.com <mailto:dmitry.samersoff at oracle.com>> wrote:
>>> 
>>> Staffan,
>>> 
>>> 1.
>>> 
>>> strerror_r comes in two versions - returning string (GNU) and returning
>>> int (XSI)
>>> 
>>> To make developers live more interesting GNU version doesn't guarantee
>>> to fill buf with appropriate string, so you can't just void return value.
>>> 
>>> Default version tends to be XSI now days and I'm not sure that build
>>> system add -D_GNU_SOURCE on all platforms.
>>> 
>>> So you have to add something like
>>> 
>>> #if _POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && ! _GNU_SOURCE
>>> use XSI strerror_r
>>> #else
>>> use GNU strerror_r
>>> #endif
>> 
>> We are getting the _GNU_SOURCE version when building. If that changes in
>> the makefiles somewhere so that we get the XSI version, then this code
>> should not compile - it should issue a warning when assigning an int to
>> char*.
>> 
>>> 
>>> Or just don't care of threads and use plain strerror - it widely used in
>>> hotspot and it doesn't crash on a race.
>>> 
>>> 2.
>>> 
>>> err_buff remains empty if the attach fails at ll.412 of ps_proc.c,
>>> but see below.
>> 
>> It should get filled in in the ptrace_attach() call.
>> 
>>> 
>>> 3. (* just an opinion *)
>>> 
>>> Extra parameters to ptrace_attach/PGrab linux makes in different from
>>> other platforms.
>> 
>> This is all platform specific code - and all the platforms are
>> different. Of course, it would be good if all platforms reported good
>> error message always - and we can continue working on that.
>> 
>> Thanks,
>> /Staffan
>> 
>>> 
>>> So I would prefer to have it unchanged - you can either mimic Solaris
>>> behavior and add Pgrab_error() function or just move all strerror staff
>>> to LinuxDebuggerLocal_attach0 - errno from ptrace_attach/Pgrab is
>>> available there.
>>> 
>>> (except calloc case, but if we can't allocate couple of bytes for struct
>>> ps_prochandle we most likely will not see the error anyway).
>>> 
>>> 
>>> -Dmitry
>>> 
>>> 
>>> On 2015-12-10 18:10, Staffan Larsen wrote:
>>>> Thanks!
>>>> 
>>>>> On 10 dec. 2015, at 15:56, Thomas Stüfe <thomas.stuefe at gmail.com
>>>>> <mailto:thomas.stuefe at gmail.com>
>>>>> <mailto:thomas.stuefe at gmail.com>> wrote:
>>>>> 
>>>>> Hi Staffan,
>>>>> 
>>>>> this looks fine now, thanks!
>>>>> 
>>>>> ..Thomas
>>>>> 
>>>>> On Thu, Dec 10, 2015 at 3:42 PM, Staffan Larsen
>>>>> <staffan.larsen at oracle.com
>>>>> <mailto:staffan.larsen at oracle.com> <mailto:staffan.larsen at oracle.com>>
>>>>> wrote:
>>>>> 
>>>>>   Hi Thomas, 
>>>>> 
>>>>>>   On 10 dec. 2015, at 15:27, Thomas Stüfe <thomas.stuefe at gmail.com
>>>>>> <mailto:thomas.stuefe at gmail.com>
>>>>>>   <mailto:thomas.stuefe at gmail.com>> wrote:
>>>>>> 
>>>>>>   Hi Staffan,
>>>>>> 
>>>>>>   thanks!
>>>>>> 
>>>>>>   It also just occurred to me that strerror is not considered
>>>>>>   threadsafe. Maybe strerror_r() would a better option (depending
>>>>>>   on the version you use you would have to check the return value
>>>>>>   for NULL, though).
>>>>> 
>>>>>   Updated here: http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.03/
>>>>> 
>>>>>   Thanks,
>>>>>   /Staffan
>>>>> 
>>>>>> 
>>>>>>   sorry for this piecemeal review.
>>>>>> 
>>>>>>   Kind Regards, Thomas
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>>   On Thu, Dec 10, 2015 at 3:22 PM, Staffan Larsen
>>>>>>   <staffan.larsen at oracle.com
>>>>>> <mailto:staffan.larsen at oracle.com> <mailto:staffan.larsen at oracle.com>>
>>>>>> wrote:
>>>>>> 
>>>>>>       Hi Thommas,
>>>>>> 
>>>>>>>       On 10 dec. 2015, at 14:49, Thomas Stüfe
>>>>>>>       <thomas.stuefe at gmail.com
>>>>>>> <mailto:thomas.stuefe at gmail.com> <mailto:thomas.stuefe at gmail.com>>
>>>>>>>       wrote:
>>>>>>> 
>>>>>>>       Hi Stefan,
>>>>>>> 
>>>>>>>       Disclaimer: not a "R"eviewer.
>>>>>>> 
>>>>>>>       http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.01/agent/src/os/linux/LinuxDebuggerLocal.c.udiff.html
>>>>>>> 
>>>>>>>       I am not sure why you pass sizeof(err_buf) - 1 instead of
>>>>>>>       sizeof(err_buf) to Pgrab and sizeof(msg) - 1 instead of
>>>>>>>       sizeof(msg) to snprintf? snprintf will always zero terminate
>>>>>>>       in case of truncation, at least on posix platforms.
>>>>>> 
>>>>>>       Good point. I was just being overly cautious without thinking.
>>>>>> 
>>>>>>       Updated
>>>>>>       webrev: http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.02/
>>>>>> 
>>>>>>       Thanks,
>>>>>>       /Staffan
>>>>>> 
>>>>>>> 
>>>>>>>       Otherwise this looks good.
>>>>>>> 
>>>>>>>       Kind Regards, Thomas
>>>>>>> 
>>>>>>> 
>>>>>>>       On Thu, Dec 10, 2015 at 2:19 PM, Staffan Larsen
>>>>>>>       <staffan.larsen at oracle.com <mailto:staffan.larsen at oracle.com>
>>>>>>>       <mailto:staffan.larsen at oracle.com>> wrote:
>>>>>>> 
>>>>>>>           Please review this patch to add a better error message
>>>>>>>           to SA when it fails to attach to a process on linux.
>>>>>>>           Currently the error says "Can't attach to the process”.
>>>>>>>           After this change the message will look something like:
>>>>>>>           "Can't attach to the process: ptrace(PTRACE_ATTACH, ..)
>>>>>>>           failed for 28417: Operation not permitted”
>>>>>>> 
>>>>>>>           webrev:
>>>>>>> http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.01/
>>>>>>>           bug: https://bugs.openjdk.java.net/browse/JDK-8145099
>>>>>>> 
>>>>>>>           Thanks,
>>>>>>>           /Staffan
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>>> 
>>>>> 
>>>>> 
>>>> 
>>> 
>>> 
>>> -- 
>>> Dmitry Samersoff
>>> Oracle Java development team, Saint Petersburg, Russia
>>> * I would love to change the world, but they won't give me the sources.
>> 
> 
> 
> -- 
> Dmitry Samersoff
> Oracle Java development team, Saint Petersburg, Russia
> * I would love to change the world, but they won't give me the sources.