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

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. 

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