[PATCH] objtool: Fix memory leak in decode_instructions()

[PATCH] objtool: Fix memory leak in decode_instructions()

[Kamalesh Babulal]

free the allocated insn before returning, when an error occurs
before adding insn to file->insn_list.

Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
---
 tools/objtool/check.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index a0c518ecf085..c0e26ad1fa7e 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -267,12 +267,13 @@ static int decode_instructions(struct objtool_file *file)
 						      &insn->immediate,
 						      &insn->stack_op);
 			if (ret)
-				return ret;
+				goto err;
 
 			if (!insn->type || insn->type > INSN_LAST) {
 				WARN_FUNC("invalid instruction type %d",
 					  insn->sec, insn->offset, insn->type);
-				return -1;
+				ret = -1;
+				goto err;
 			}
 
 			hash_add(file->insn_hash, &insn->hash, insn->offset);
@@ -296,6 +297,10 @@ static int decode_instructions(struct objtool_file *file)
 	}
 
 	return 0;
+
+err:
+	free(insn);
+	return ret;
 }
 
 /*
-- 
2.11.0

Re: [PATCH] objtool: Fix memory leak in decode_instructions()

[Josh Poimboeuf]

On Thu, Oct 12, 2017 at 02:32:14PM +0530, Kamalesh Babulal wrote:
> free the allocated insn before returning, when an error occurs
> before adding insn to file->insn_list.
> 
> Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

Any chance you're working on porting objtool to ppc64le? :-)

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: [PATCH] objtool: Fix memory leak in decode_instructions()

[Kamalesh Babulal]

On Thursday 12 October 2017 09:40 PM, Josh Poimboeuf wrote:
> On Thu, Oct 12, 2017 at 02:32:14PM +0530, Kamalesh Babulal wrote:
>> free the allocated insn before returning, when an error occurs
>> before adding insn to file->insn_list.
>>
>> Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
>
> Any chance you're working on porting objtool to ppc64le? :-)
>
> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
>

Thanks for the review. I have started working on it :)

-- 
cheers,
Kamalesh.

Re: [PATCH] objtool: Fix memory leak in decode_instructions()

[Josh Poimboeuf]

On Fri, Oct 13, 2017 at 10:14:36AM +0530, Kamalesh Babulal wrote:
> On Thursday 12 October 2017 09:40 PM, Josh Poimboeuf wrote:
> > On Thu, Oct 12, 2017 at 02:32:14PM +0530, Kamalesh Babulal wrote:
> > > free the allocated insn before returning, when an error occurs
> > > before adding insn to file->insn_list.
> > > 
> > > Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
> > 
> > Any chance you're working on porting objtool to ppc64le? :-)
> > 
> > Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > 
> 
> Thanks for the review. I have started working on it :)

Good!  Let me know if you have any questions.

I originally wrote objtool with arch-independence in mind, though with
the new "objtool 2.0" rewrite, it unfortunately became more
x86-specific.

I was hoping to work on making it more arch-independent, and then start
porting it to other arches, but it may be a few months before I have the
time to do so.  So any work you want to there would be great.

-- 
Josh

Re: [PATCH] objtool: Fix memory leak in decode_instructions()

[Kamalesh Babulal]

On Friday 13 October 2017 10:36 AM, Josh Poimboeuf wrote:
> On Fri, Oct 13, 2017 at 10:14:36AM +0530, Kamalesh Babulal wrote:
>> On Thursday 12 October 2017 09:40 PM, Josh Poimboeuf wrote:
>>> On Thu, Oct 12, 2017 at 02:32:14PM +0530, Kamalesh Babulal wrote:
>>>> free the allocated insn before returning, when an error occurs
>>>> before adding insn to file->insn_list.
>>>>
>>>> Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
>>>
>>> Any chance you're working on porting objtool to ppc64le? :-)
>>>
>>> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
>>>
>>
>> Thanks for the review. I have started working on it :)
>
> Good!  Let me know if you have any questions.

Thank you, I am sure I will have lots of them.

>
> I originally wrote objtool with arch-independence in mind, though with
> the new "objtool 2.0" rewrite, it unfortunately became more
> x86-specific.
>
> I was hoping to work on making it more arch-independent, and then start
> porting it to other arches, but it may be a few months before I have the
> time to do so.  So any work you want to there would be great.
>

Sure, will keep that in mind to abstract arch-independent code in to 
common files and push arch-dependent code into arch/ directory for both 
ppc64le/x86.

-- 
cheers,
Kamalesh.

Re: [PATCH] objtool: Fix memory leak in decode_instructions()

[Josh Poimboeuf]

On Fri, Oct 13, 2017 at 11:20:58AM +0530, Kamalesh Babulal wrote:
> On Friday 13 October 2017 10:36 AM, Josh Poimboeuf wrote:
> > On Fri, Oct 13, 2017 at 10:14:36AM +0530, Kamalesh Babulal wrote:
> > > On Thursday 12 October 2017 09:40 PM, Josh Poimboeuf wrote:
> > > > On Thu, Oct 12, 2017 at 02:32:14PM +0530, Kamalesh Babulal wrote:
> > > > > free the allocated insn before returning, when an error occurs
> > > > > before adding insn to file->insn_list.
> > > > > 
> > > > > Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
> > > > 
> > > > Any chance you're working on porting objtool to ppc64le? :-)
> > > > 
> > > > Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > > 
> > > 
> > > Thanks for the review. I have started working on it :)
> > 
> > Good!  Let me know if you have any questions.
> 
> Thank you, I am sure I will have lots of them.
> 
> > 
> > I originally wrote objtool with arch-independence in mind, though with
> > the new "objtool 2.0" rewrite, it unfortunately became more
> > x86-specific.
> > 
> > I was hoping to work on making it more arch-independent, and then start
> > porting it to other arches, but it may be a few months before I have the
> > time to do so.  So any work you want to there would be great.
> > 
> 
> Sure, will keep that in mind to abstract arch-independent code in to common
> files and push arch-dependent code into arch/ directory for both
> ppc64le/x86.

Kamalesh, since you might be working on this soon, here's a bit of a
brain dump in case it helps.

I have a vague idea for eventually improving objtool, which might make
arch-independence easier.  I wonder if we could use a GCC plugin to add
annotations in special sections, which are then read by objtool to help
it understand what's going on in the code.

I suspect the plugins would need to be arch-specific.  But then maybe
that would allow objtool itself to be completely arch-independent.
Eventually, if we had similar clang plugins, objtool could become
compiler-independent as well.

Unfortunately I haven't had a chance to think about it any more deeply
than that.  Just something to think about.

Or maybe it makes sense to port objtool to ppc first, before trying to
abstract the arch-specific parts into plugins.  I dunno.

Also, another idea that has been suggested, is to use DWARF data as
input to objtool.  I have my doubts about that idea, but if it worked,
that might be another way to help objtool achieve arch-independence.

For a previous discussion of these ideas, see:

  https://lkml.kernel.org/r/20170831044209.4hodx2dasad66yab@treble

-- 
Josh

Re: [PATCH] objtool: Fix memory leak in decode_instructions()

[Josh Poimboeuf]

On Thu, Oct 19, 2017 at 09:31:37AM -0500, Josh Poimboeuf wrote:
> On Fri, Oct 13, 2017 at 11:20:58AM +0530, Kamalesh Babulal wrote:
> > On Friday 13 October 2017 10:36 AM, Josh Poimboeuf wrote:
> > > On Fri, Oct 13, 2017 at 10:14:36AM +0530, Kamalesh Babulal wrote:
> > > > On Thursday 12 October 2017 09:40 PM, Josh Poimboeuf wrote:
> > > > > On Thu, Oct 12, 2017 at 02:32:14PM +0530, Kamalesh Babulal wrote:
> > > > > > free the allocated insn before returning, when an error occurs
> > > > > > before adding insn to file->insn_list.
> > > > > > 
> > > > > > Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
> > > > > 
> > > > > Any chance you're working on porting objtool to ppc64le? :-)
> > > > > 
> > > > > Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > > > > 
> > > > 
> > > > Thanks for the review. I have started working on it :)
> > > 
> > > Good!  Let me know if you have any questions.
> > 
> > Thank you, I am sure I will have lots of them.
> > 
> > > 
> > > I originally wrote objtool with arch-independence in mind, though with
> > > the new "objtool 2.0" rewrite, it unfortunately became more
> > > x86-specific.
> > > 
> > > I was hoping to work on making it more arch-independent, and then start
> > > porting it to other arches, but it may be a few months before I have the
> > > time to do so.  So any work you want to there would be great.
> > > 
> > 
> > Sure, will keep that in mind to abstract arch-independent code in to common
> > files and push arch-dependent code into arch/ directory for both
> > ppc64le/x86.
> 
> Kamalesh, since you might be working on this soon, here's a bit of a
> brain dump in case it helps.
> 
> I have a vague idea for eventually improving objtool, which might make
> arch-independence easier.  I wonder if we could use a GCC plugin to add
> annotations in special sections, which are then read by objtool to help
> it understand what's going on in the code.
> 
> I suspect the plugins would need to be arch-specific.  But then maybe
> that would allow objtool itself to be completely arch-independent.
> Eventually, if we had similar clang plugins, objtool could become
> compiler-independent as well.
> 
> Unfortunately I haven't had a chance to think about it any more deeply
> than that.  Just something to think about.
> 
> Or maybe it makes sense to port objtool to ppc first, before trying to
> abstract the arch-specific parts into plugins.  I dunno.
> 
> Also, another idea that has been suggested, is to use DWARF data as
> input to objtool.  I have my doubts about that idea, but if it worked,
> that might be another way to help objtool achieve arch-independence.
> 
> For a previous discussion of these ideas, see:
> 
>   https://lkml.kernel.org/r/20170831044209.4hodx2dasad66yab@treble

Also, any chance you'll be in Prague for the conferences next week?  If
so, we could discuss it more in person.

-- 
Josh

Re: [PATCH] objtool: Fix memory leak in decode_instructions()

[Kamalesh Babulal]

On Thursday 19 October 2017 08:02 PM, Josh Poimboeuf wrote:
> On Thu, Oct 19, 2017 at 09:31:37AM -0500, Josh Poimboeuf wrote:
>> On Fri, Oct 13, 2017 at 11:20:58AM +0530, Kamalesh Babulal wrote:
>>> On Friday 13 October 2017 10:36 AM, Josh Poimboeuf wrote:
>>>> On Fri, Oct 13, 2017 at 10:14:36AM +0530, Kamalesh Babulal wrote:
>>>>> On Thursday 12 October 2017 09:40 PM, Josh Poimboeuf wrote:
>>>>>> On Thu, Oct 12, 2017 at 02:32:14PM +0530, Kamalesh Babulal wrote:
>>>>>>> free the allocated insn before returning, when an error occurs
>>>>>>> before adding insn to file->insn_list.
>>>>>>>
>>>>>>> Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
>>>>>>
>>>>>> Any chance you're working on porting objtool to ppc64le? :-)
>>>>>>
>>>>>> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
>>>>>>
>>>>>
>>>>> Thanks for the review. I have started working on it :)
>>>>
>>>> Good!  Let me know if you have any questions.
>>>
>>> Thank you, I am sure I will have lots of them.
>>>
>>>>
>>>> I originally wrote objtool with arch-independence in mind, though with
>>>> the new "objtool 2.0" rewrite, it unfortunately became more
>>>> x86-specific.
>>>>
>>>> I was hoping to work on making it more arch-independent, and then start
>>>> porting it to other arches, but it may be a few months before I have the
>>>> time to do so.  So any work you want to there would be great.
>>>>
>>>
>>> Sure, will keep that in mind to abstract arch-independent code in to common
>>> files and push arch-dependent code into arch/ directory for both
>>> ppc64le/x86.
>>
>> Kamalesh, since you might be working on this soon, here's a bit of a
>> brain dump in case it helps.
>>
>> I have a vague idea for eventually improving objtool, which might make
>> arch-independence easier.  I wonder if we could use a GCC plugin to add
>> annotations in special sections, which are then read by objtool to help
>> it understand what's going on in the code.
>>
>> I suspect the plugins would need to be arch-specific.  But then maybe
>> that would allow objtool itself to be completely arch-independent.
>> Eventually, if we had similar clang plugins, objtool could become
>> compiler-independent as well.
>>
>> Unfortunately I haven't had a chance to think about it any more deeply
>> than that.  Just something to think about.
>>
>> Or maybe it makes sense to port objtool to ppc first, before trying to
>> abstract the arch-specific parts into plugins.  I dunno.
>>

Thanks for sharing the thoughts on making objtool arch-independent.
I have not followed the discussions closely on GCC plugin to add
annotations/DWARF data. I will read through the ideas, before
commenting on them.


>> Also, another idea that has been suggested, is to use DWARF data as
>> input to objtool.  I have my doubts about that idea, but if it worked,
>> that might be another way to help objtool achieve arch-independence.
>>
>> For a previous discussion of these ideas, see:
>>
>>   https://lkml.kernel.org/r/20170831044209.4hodx2dasad66yab@treble
>
> Also, any chance you'll be in Prague for the conferences next week?  If
> so, we could discuss it more in person.
>

I will not be attending the conference.

-- 
cheers,
Kamalesh.

Re: [PATCH] objtool: Fix memory leak in decode_instructions()

[Josh Poimboeuf]

Hi Ingo,

I'm not sure if you saw the below ACK.  In general, for objtool patches
which don't have you on CC, is it ok if I just add you to CC along with
the ACK?  Or would you prefer I resend the patch to you with my SOB?

Or, would you rather I start queueing patches and doing pull requests
weekly (or so)?

Thanks!

On Thu, Oct 12, 2017 at 11:10:16AM -0500, Josh Poimboeuf wrote:
> On Thu, Oct 12, 2017 at 02:32:14PM +0530, Kamalesh Babulal wrote:
> > free the allocated insn before returning, when an error occurs
> > before adding insn to file->insn_list.
> > 
> > Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
> 
> Any chance you're working on porting objtool to ppc64le? :-)
> 
> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: [PATCH] objtool: Fix memory leak in decode_instructions()

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> Hi Ingo,
> 
> I'm not sure if you saw the below ACK.  In general, for objtool patches
> which don't have you on CC, is it ok if I just add you to CC along with
> the ACK?  Or would you prefer I resend the patch to you with my SOB?

Yeah, that would be nice - that way I get your SOB which is better.

Thanks,

	Ingo

Re: [PATCH] objtool: Fix memory leak in decode_instructions()

[Josh Poimboeuf]

On Thu, Oct 19, 2017 at 05:21:06PM +0200, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > Hi Ingo,
> > 
> > I'm not sure if you saw the below ACK.  In general, for objtool patches
> > which don't have you on CC, is it ok if I just add you to CC along with
> > the ACK?  Or would you prefer I resend the patch to you with my SOB?
> 
> Yeah, that would be nice - that way I get your SOB which is better.

Sounds good.  And for patches which you *are* on CC, do you have a
preference (ACK or resend with SOB)?

-- 
Josh

Re: [PATCH] objtool: Fix memory leak in decode_instructions()

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Thu, Oct 19, 2017 at 05:21:06PM +0200, Ingo Molnar wrote:
> > 
> > * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > 
> > > Hi Ingo,
> > > 
> > > I'm not sure if you saw the below ACK.  In general, for objtool patches
> > > which don't have you on CC, is it ok if I just add you to CC along with
> > > the ACK?  Or would you prefer I resend the patch to you with my SOB?
> > 
> > Yeah, that would be nice - that way I get your SOB which is better.
> 
> Sounds good.  And for patches which you *are* on CC, do you have a
> preference (ACK or resend with SOB)?

In general resend with SOB would be more robust for me, but as the 
objtool/x86-debuginfo maintainer it's your call really, I can certainly do both.

Thanks,

	Ingo

[PATCH] objtool: Fix memory leak in decode_instructions()

[Josh Poimboeuf]

From: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

When an error occurs before adding an allocated insn to the list, free
it before returning.

Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/objtool/check.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 83f370fa00c2..9b341584eb1b 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -267,12 +267,13 @@ static int decode_instructions(struct objtool_file *file)
 						      &insn->immediate,
 						      &insn->stack_op);
 			if (ret)
-				return ret;
+				goto err;
 
 			if (!insn->type || insn->type > INSN_LAST) {
 				WARN_FUNC("invalid instruction type %d",
 					  insn->sec, insn->offset, insn->type);
-				return -1;
+				ret = -1;
+				goto err;
 			}
 
 			hash_add(file->insn_hash, &insn->hash, insn->offset);
@@ -296,6 +297,10 @@ static int decode_instructions(struct objtool_file *file)
 	}
 
 	return 0;
+
+err:
+	free(insn);
+	return ret;
 }
 
 /*
-- 
2.13.6