Re: [PATCH] Proposed fix for oprofile JIT bad event spec error

On 8/13/19 5:30 PM, will schmidt wrote:
> On Tue, 2019-08-13 at 16:36 -0400, William Cohen wrote:
>> On 7/26/19 4:38 PM, will schmidt wrote:
>>> Hi Oprofile-devs,
>>> Attached is a fix for a problem we have seen recently when
>>> trying to profile against java/JIT workloads.
>>
>> Hi,
>>
>> Sorry for the delay reviewing the patch.  I do have a question about
>> the code below.
>>
>>
>>>     
>>> The error floated to the user at opreport time is:
>>>     
>>>   opreport error: parse_event_spec(): bad event specification:
>>> 165624.0x
>>>     
>>> Where the 165624 value will match the PIDs of the processes being
>>> profiled.
>>>     
>>> This ends up being an issue with how we are building the string for
>>> the file
>>> that contains the ELF blob for the java/JIT code
>>> (i.e.  12345.jo).  We were
>>> not truncating our 'path' string such that when we strncpy'd it
>>> into our
>>> path-and-file variable, we were getting part of an address range
>>> (12345.0x)
>>> instead of the desired .jo suffix.
>>>     
>>> The code has been updated to correct that.  Sniff testing shows we
>>> are
>>> now successfully processing the .jo contents, and no longer seeing
>>> the bad
>>> event spec error.
>>>     
>>> Tested locally, this appears to resolve the issue.
>>>     
>>>     agents/jvmti/libjvmti_oprofile.c:
>>>      - update strncpy call to avoid gcc warning on strncpy
>>> parameters.
>>>     opjitconv/opjitconv.c:
>>>      - update logic around calculation of elf_file_size, and rework
>>> snprintf
>>>      calls so we properly truncate the input string.
>>>     
>>>      Signed-off-by: Will Schmidt  <[email protected]>
>>>
>>> ---
>>>
>>>
>>> diff --git a/agents/jvmti/libjvmti_oprofile.c
>>> b/agents/jvmti/libjvmti_oprofile.c
>>> index 40f9979..b518eb1 100644
>>> --- a/agents/jvmti/libjvmti_oprofile.c
>>> +++ b/agents/jvmti/libjvmti_oprofile.c
>>> @@ -174,11 +174,11 @@ static void JNICALL
>>> cb_compiled_method_load(jvmtiEnv * jvmti,
>>>  
>>>  	{
>>>  	int cnt = strlen(method_name) + strlen(class_signature) +
>>>  		strlen(method_signature) + 2;
>>>  	char buf[cnt];
>>> -	strncpy(buf, class_signature, cnt - 1);
>>> +	strncpy(buf, class_signature, sizeof(buf) - 1);
>>>  	strncat(buf, method_name, cnt - strlen(buf) - 1);
>>>  	strncat(buf, method_signature, cnt - strlen(buf) - 1);
>>>  	if (op_write_native_code(agent_hdl, buf,
>>>  				 (uint64_t)(uintptr_t) code_addr,
>>>  				 code_addr, code_size)) {
>>> diff --git a/opjitconv/opjitconv.c b/opjitconv/opjitconv.c
>>> index c55d8b1..5b46142 100644
>>> --- a/opjitconv/opjitconv.c
>>> +++ b/opjitconv/opjitconv.c
>>> @@ -311,10 +311,11 @@ static int process_jit_dumpfile(char const *
>>> dmp_pathname,
>>>  	int jofd;
>>>  	struct stat file_stat;
>>>  	time_t dumpfile_modtime;
>>>  	struct op_jitdump_info dmp_info;
>>>  	char * elf_file = NULL;
>>> +	char * tempstring = NULL;
>>>  	char * proc_id = NULL;
>>>  	char const * anon_dir;
>>>  	char const * dumpfilename = rindex(dmp_pathname, '/');
>>>  	/* temporary copy of dump file created for conversion step */
>>>  	char * tmp_dumpfile;
>>> @@ -388,15 +389,16 @@ chk_proc_id:
>>>  			rc = OP_JIT_CONV_FAIL;
>>>  			goto free_res2;
>>>  		}
>>>  		result_dir_length = ++anon_path_seg - anon_dir;
>>>  		/* create final ELF file name */
>>> -		elf_file_size = result_dir_length
>>> +		elf_file_size = result_dir_length + strlen("/")
>>>  			+ strlen(proc_id) + strlen(".jo") + 1;
>>>  		elf_file = xmalloc(elf_file_size);
>>> -		snprintf(elf_file, elf_file_size, "%s%s.jo",
>>> -			 anon_dir, proc_id);
>>> +		tempstring = xmalloc(elf_file_size);
>>> +		snprintf(tempstring, result_dir_length, "%s",anon_dir);
>>> +		snprintf(elf_file, elf_file_size,"%s/%s.jo",tempstring,proc_id);
>>
>> Why not directly use anon_dir in the snprintf above rather than
>> having another variable tempstring, allocating space for it, and
>> doing a snprintf on tempstring then immediately using the result of
>> for snprintf into elf_file?  Looks like result_dir_length might need
>> to be adjected for this change.
> 
> I think I had coded that up at first.  Don't immediately recall if I
> got a compiler warning, or just the spider sense awareness, but per a
> double-check of the manpage, tesults are undefined if we were to have
> anon_dir as both source and destination on the snprintf.
> None of the existing strings seemed like good choices for re-use, so I
> went with a new one.
> Tempstring could probably be better named something like
> full_path_to_elf_object .   variable naming isn't my strength. :-)
> I'm not strongly tied to this if there is another approach you have in
> mind. 

Hi,

String handling is not the strong suite of C/C++.  Having to parse through the string handling is a pain as one needs to be careful about the index values to the char arrays.  I am comfortable with the patch now and have merged it in.  Thanks.  Is there a testcase that could be put into oprofile-testsuite?

-Will Cohen
> 
> Thanks
> -Will (Schmidt)
> 
> 
> 
> 
>>
>> -Will Cohen
>>
>>>  		/* create temporary ELF file name */
>>>  		tmp_elffile_size = strlen(tmp_conv_dir) + 1
>>>  			+ strlen(proc_id) + strlen(".jo") + 1;
>>>  		tmp_elffile = xmalloc(tmp_elffile_size);
>>>  		snprintf(tmp_elffile, tmp_elffile_size, "%s/%s.jo",
>>>
>>>
>>>
>>> _______________________________________________
>>> oprofile-list mailing list
>>> [email protected]
>>> https://lists.sourceforge.net/lists/listinfo/oprofile-list
>>>
>>
>>
>