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

On Wed, 2019-08-14 at 14:07 -0400, William Cohen wrote:
> 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?

Thanks for the review and the commit.   :-)

I'll investigate adding a test case to the testsuite.  (in the queue,
won't be right away). 

Thanks,
-Will (Schmidt)


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