bugmake - Bugs: bug #33125, make fails building android build...

 
 

bug #33125: make fails building android build environment due to memory corruption

Submitted by:  Matthias Hopf <mshopf>
Submitted on:  Tue 19 Apr 2011 01:37:36 PM UTC  
 
Severity: 3 - NormalItem Group: Bug
Status: FixedPrivacy: Public
Assigned to: Paul D. Smith <psmith>Open/Closed: Closed
Component Version: 3.82Operating System: Any
Fixed Release: 4.0Triage Status: Verified

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Mon 02 May 2011 01:55:23 PM UTC, comment #8:

I added a regression test with my fix. It won't show any difference in behavior unless you run it in valgrind or similar though.

Paul D. Smith <psmith>
Project AdministratorIn charge of this item.
Mon 02 May 2011 01:14:46 PM UTC, comment #7:

I also create a test case for regression testing. It's pretty trivial, I'm attaching it here.

I'm just unsure in which script this should be added to. Presumably functions/sort. Please do as seems fit.

(file #23337)

Matthias Hopf <mshopf>
Mon 02 May 2011 12:43:28 PM UTC, comment #6:

I understand the problem. Fix committed. Thanks for your efforts Matthias!

Paul D. Smith <psmith>
Project AdministratorIn charge of this item.
Mon 02 May 2011 10:09:58 AM UTC, comment #5:

Find the captured string attached (excerpt of a debug output). If you analyze it in a hex dumper, you'll see a number of LF / space combinations.

Because the splitting function does not split at LFs, it creates a number of single LF tokens split by spaces (which were skipped together as whitespace in the counting routine).

(file #23336)

Matthias Hopf <mshopf>
Mon 02 May 2011 09:42:40 AM UTC, comment #4:

I understand your puzzlement - the more I think about it the less I personally understand...

I will capture the string - there is no easy test case, because the android build system isn't exactly trivial. It was only found due to corruption in free() at a much later point in time.

Matthias Hopf <mshopf>
Mon 02 May 2011 06:15:32 AM UTC, comment #3:

So I'm happy to make this change, because it does seem cleaner, but I must confess I don't understand how the original error is caused by it.

The code uses isspace() to count spaces the first time through the list. isspace() matches spaces and tabs, but also form feeds, carriage-returns, etc. The second loop skips ONLY spaces and tabs.

So it seems that the only possible problem here would be that the first loop counts TOO MANY words and allocates TOO MUCH memory: more than is necessary. I don't see any way for the current code to UNDER-allocate the space for the array, unless for some bizarre reason either space or TAB characters are not matched by isspace() which seems impossible.

Can you provide me with a repro test case? Even just the contents of the string that were passed to the $(sort ...) function would be helpful.

Paul D. Smith <psmith>
Project AdministratorIn charge of this item.
Tue 19 Apr 2011 03:26:22 PM UTC, comment #2:

Please use ONLY the revised patch; it doesn't change whitespace handling in tokenization, which introduced an incompatible semantic change.

The revised patch only fixes array counting as considered in the original report.

All test cases are working fine with this change.

(file #23275)

Matthias Hopf <mshopf>
Tue 19 Apr 2011 02:55:08 PM UTC, comment #1:

Just found that this apparently breaks test variables/define.
Analyzing.

Matthias Hopf <mshopf>
Tue 19 Apr 2011 01:37:36 PM UTC, original submission:

Building android build environment as described in

http://source.android.com/source/download.html

fails due to a glibc free() error. The bugs seems well known, the build system in fact tests for the make version and fails for versions != 3.81, and the wiki describes 3.82 to be non-working.

Testing with valgrind shows some memory corruption:

==13108== Invalid write of size 8
==13108== at 0x40BFAD: func_sort (function.c:1159)
==13108== by 0x40CE76: expand_builtin_function (function.c:2164)
==13108== by 0x40D1C0: handle_function (function.c:2273)
==13108== by 0x40692D: variable_expand_string (expand.c:263)
==13108== by 0x406E99: variable_expand (expand.c:429)
==13108== by 0x407004: variable_expand_for_file (expand.c:477)
==13108== by 0x4072B5: allocated_variable_expand_for_file (expand.c:574)
==13108== by 0x406FC4: expand_argument (expand.c:458)
==13108== by 0x40D07D: handle_function (function.c:2241)
==13108== by 0x40692D: variable_expand_string (expand.c:263)
==13108== by 0x406E99: variable_expand (expand.c:429)
==13108== by 0x407004: variable_expand_for_file (expand.c:477)

The string that is to be tokenized in func_sort() apparently contains non-space/tab whitespace.

Patch attached.

Though I have to remark that I find the use of two completely different approaches to
a) count required array entries and
b) fill the array entries
in function.c:func_sort() slightly disturbing. Without additional assert()s this just calls for trouble.

Question would be how to handle this correctly. I'd suggest the use of find_next_token() for counting as well. I can create an additional(!) patch for that if that seems fit.

Matthias Hopf <mshopf>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #23337:  multiline added by mshopf (358B - application/octet-stream - regression test script)
file #23336:  make.dump added by mshopf (2KiB - image/x-xwindowdump - debugging excerpt with the failing token string)
file #23275:  make-fix_whitespace_tokenization.diff added by mshopf (675B - text/x-patch - Revised patch that does NOT change whitespace handling, only array counting)
file #23274:  make-fix_whitespace_tokenization.diff added by mshopf (864B - text/x-patch - Patch to fix whitespace handling in tokenization.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by psmith (Posted a comment)
  • -unavailable- added by mshopf (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 02 May 2011 01:14:46 PM UTCmshopfAttached File-=>Added multiline, #23337
    Mon 02 May 2011 12:43:28 PM UTCpsmithStatusNone=>Fixed
      Assigned toNone=>psmith
      Open/ClosedOpen=>Closed
      Operating SystemNone=>Any
      Fixed ReleaseNone=>4.0
      Triage StatusNone=>Verified
    Mon 02 May 2011 10:09:58 AM UTCmshopfAttached File-=>Added make.dump, #23336
    Tue 19 Apr 2011 03:26:22 PM UTCmshopfAttached File-=>Added make-fix_whitespace_tokenization.diff, #23275
    Tue 19 Apr 2011 01:37:37 PM UTCmshopfAttached File-=>Added make-fix_whitespace_tokenization.diff, #23274

    Back to the top


    Powered by Savane 3.1-cleanup1