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

 
 

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

Submitter:  Matthias Hopf <mshopf>
Submitted:  Tue 19 Apr 2011 01:37:36 PM UTC
   
 
Severity:  3 - Normal Item Group:  Bug
Status:  Fixed Privacy:  Public
Assigned to:  psmith Open/Closed:  Closed
Component Version:  3.82 Operating System:  Any
Fixed Release:  4.0 Triage Status:  Verified
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group administrator
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>
Group administrator
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>
Group administrator
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 Files:
   
   
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
  • -email is unavailable- added by psmith (Posted a comment)
  • -email is unavailable- added by mshopf (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-05-02 mshopf Attached File- Added multiline, #23337
    2011-05-02 psmith StatusNone Fixed
        Assigned toNone psmith
        Open/ClosedOpen Closed
        Operating SystemNone Any
        Fixed ReleaseNone 4.0
        Triage StatusNone Verified
    2011-05-02 mshopf Attached File- Added make.dump, #23336
    2011-04-19 mshopf Attached File- Added make-fix_whitespace_tokenization.diff, #23275
    2011-04-19 mshopf Attached File- Added make-fix_whitespace_tokenization.diff, #23274

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code