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

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.

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