bugmake - Bugs: bug #40226, Weird failure on Windows with...

 
 

bug #40226: Weird failure on Windows with OUTPUT_SYNC_TARGET

Submitted by:  None
Submitted on:  Wed 09 Oct 2013 10:27:54 PM UTC  
 
Severity: 3 - NormalItem Group: Bug
Status: FixedPrivacy: Public
Assigned to: Paul D. Smith <psmith>Open/Closed: Closed
Component Version: 4.0Operating System: None
Fixed Release: SCMTriage Status: None

Add a New Comment (Rich MarkupRich 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)

Sun 24 Nov 2013 09:12:11 AM UTC, comment #11:

I enhanced the option parsing in make to create a new type of option, that takes a single string instead of a list of strings. If multiple instances of that option are provided, each subsequent instance overwrites (instead of adds to) the previous one.

This new type is used for the jobserver fds internal option, the output-sync option, and the sync-mutex option.

This means we're not constructing "fake" stringlists for these options and we avoid the memory issue described here.

Paul D. Smith <psmith>
Project AdministratorIn charge of this item.
Mon 14 Oct 2013 05:29:17 PM UTC, comment #10:

Actually, this issue can also be reproduced with a x64 build done using MinGW (from http://mingw-w64.sourceforge.net/).

Christian Boos <cboos>
Sun 13 Oct 2013 06:57:56 PM UTC, comment #9:

Eli, it looks like the problem is triggered as soon
as you have some output generated during the
`read_all_makefiles ()` phase, as it causes a call
to `prepare_mutex_handle_string ()`.

And after reading the Makefiles, `main ()` calls
`decode_env_switches ("MAKEFLAGS")` which in turn
calls `decode_switches ()` and the added flag
`--sync-mutex=0x44` is now present.

So in my example at the top of comment #3, the trigger
was the `perror_with_name ()` call for the missing
no-Makefile file.

The problem indeed doesn't happen with simple
Makefiles where make doesn't generate any output
while reading the Makefiles.

Examples:

Of course, you have to be "lucky" and have that particular
memory layout I described in comment #3, i.e. the initial
xstrdup in `prepare_mutex_handle_string ()` must allocate
the "0x44" string just after the list itself. Otherwise
the memory overwrite that happens in `decode_switches ()`
is likely to remain asymptomatic. For example, when I launch
make from the debugger, the allocation pattern is different
and all seems fine, I could only figure this out by
attaching the debugger to the process...

Also, I just noticed that the problem is only reproducible
with 64-bits builds. I figure it might be difficult to
reproduce with other builds of make (msys, cygwin).

So the bug is likely to remain unnoticed most of the time,
which doesn't mean it's not there ;-)

Christian Boos <cboos>
Sun 13 Oct 2013 04:58:29 PM UTC, comment #8:

Are we going to release 4.01 soon? If not, is the problem serious enough to fix it in the present code? Or can I safely use the Windows binary of 4.0 unaltered? (As I wrote, the problem does not happen in my usage.)

Eli Zaretskii <eliz>
Project Member
Sun 13 Oct 2013 04:38:15 PM UTC, comment #7:

Christian: Thanks for the extended explanations.

For the record: I don't see this on my system, at least not with the simple recipe you provided. Moreover, the (second) call to `decode_switches`, after `prepare_mutex_handle_string`, never examines the --sync-mutex switch here. And that is actually what I would expect, since the --sync-mutex switch is prepared for sub-Make, which is never launched in this case.

Moreover, the same pattern of code is used for the --jobserver-fds switch, so if there is a problem, it should show up on Unix as well.

But I trust Paul will fix this anyway.

Thanks.

Eli Zaretskii <eliz>
Project Member
Sun 13 Oct 2013 04:31:45 PM UTC, comment #6:

I do understand what Christian is saying, but I'm going to rewrite it out of existence rather than fix it :-)

Paul D. Smith <psmith>
Project AdministratorIn charge of this item.
Sun 13 Oct 2013 04:16:21 PM UTC, comment #5:

Paul: If you already see and understand the problem, by all means take it. I didn't yet have time to even build the release ;-).

Thanks.

Eli Zaretskii <eliz>
Project Member
Sun 13 Oct 2013 03:52:18 PM UTC, comment #4:

Eli: unless you want to deal with this one, I'll take it. I noticed a few weeks ago that option parsing currently has no facility for a "single string"; we can only have a list of strings or filenames (e.g., for "-I"). But this doesn't make sense for some of our switches, such as -O or the internal jobserver-fds. So I think we need a new switch type which is just a single string where any subsequent option replaces the existing one. Then we won't need the stringlist here at all. I didn't want to get into this before the release though.

Paul D. Smith <psmith>
Project AdministratorIn charge of this item.
Sun 13 Oct 2013 03:35:15 PM UTC, comment #3:

Well, the symptom can even be reproduced without a Makefile...

The reason for this "multiple --sync-mutex options" problem
can be traced to the memory handling bug I presented earlier.
I'll try to explain better what I saw while I was debugging
this issue.

The initial allocation of the sync_mutex stringlist which
happens in `prepare_mutex_handle_string ()` (main.c) leads
to the following memory layout at the address pointed to
by `sync_mutex->list`:

i.e. the blocks returned by the xmalloc and xstrdup
calls happen to be contiguous... So far, so good.

For reasons I didn't bother to check, the program then
enters `decode_switches ()` and one of these
switches is now "--sync-mutex 0xf4" (for example).
When handling this parameter in the `case string:`,
the code first tries to check if the stringlist
is not already allocated (it is), or if the list needs
to be expanded (that check is wrong). Here we have
idx == max == 1 for that list, so the expansion check
will wrongly decide all is fine and proceed with the
update code, which will make slot 1 point to "0xf4"
and null-terminate the list in the slot 2.

Now we have the slot 0 pointing to an empty value
(`(char*)0) and slot 1 pointing to "0xf4", and this
is what `decode_output_sync_flags ()` detects and
reports as an error.

Hope my previous comments now make better sense ;-)

(please ignore the first patch file #29339 which was
wrong; I believe the second patch file #29345 is correct,
as well as the alternative solution presented in comment #2
of changing the test operator from == to >=, in the expansion
check).

Christian Boos <cboos>
Sat 12 Oct 2013 03:17:41 PM UTC, comment #2:

Could you please provide a minimal Makefile and a test case to reproduce the problem you saw? I'm sorry, but I have a difficulty understanding your description of the bug you found in the code; hopefully a test case will help me see the light.

Thanks!

Eli Zaretskii <eliz>
Project Member
Thu 10 Oct 2013 10:15:17 AM UTC, comment #1:

Second thought... I see now where the bug is coming from.

Some use cases for the stringlist imply it's handled as a null-terminated array:

Some others iterate up to idx:

For the second example, the list is initialized like this:

(copy/paste from the jobserver_fds code?)

The problem is that decode_switches() assumes the first use case, but when sync_mutex ends up there, the fact that idx == max prevents the list to expand as the (sl->idx == sl->max - 1)
condition is never met.

So how to fix this best? For consistency, perhaps always ensure
such a "list" null-terminated, and fix the initialization.
That would also be in accordance with the struct stringlist
comments. But there's also a simpler fix which would be to
change the expansion check from (sl->idx == sl->max - 1) to
(sl->idx >= sl->max - 1) so that it would also work for the
case when they're both set to 1.

Both solutions tested with success.

(file #29345)

Christian Boos <cboos>
Wed 09 Oct 2013 10:27:54 PM UTC, original submission:

On Windows, with a MSVC9 x64 build, I observed the following behavior:

$ /C/Workspace/src/git/make/WinDebug/make.exe -Otarget
...
make: *** internal error: multiple --sync-mutex options. Stop.

I could trace this to an invalid manipulation of the
sync_mutex stringlist in main.c, see attached patch which
fixes the issue. I also tested that patch on Linux where
I could run the test suite with success (couldn't manage
to do that on Windows, unfortunately).

Anonymous

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #29345:  0001-Fix-initialization-of-sync_mutex-and-jobserver_fds-o.patch added by cboos (2kB - application/octet-stream - more correct fix, please ignore the previous patch file #29339)

 

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 eliz (Posted a comment)
  • -unavailable- added by cboos (Updated the item)
  • -unavailable- added by cboos
  •  

    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 7 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 24 Nov 2013 09:12:11 AM UTCpsmithStatusNone=>Fixed
      Assigned toNone=>psmith
      Open/ClosedOpen=>Closed
      Fixed ReleaseNone=>SCM
    Thu 10 Oct 2013 10:15:17 AM UTCcboosAttached File-=>Added 0001-Fix-initialization-of-sync_mutex-and-jobserver_fds-o.patch, #29345
    Wed 09 Oct 2013 11:43:18 PM UTCcboosCarbon-Copy-=>Added cboos
    Wed 09 Oct 2013 10:27:54 PM UTCNoneAttached File-=>Added 0001-Fix-test-detecting-when-a-stringlist-must-grow-in-de.patch, #29339

    Back to the top


    Powered by Savane 3.1-cleanup