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

 
 

bug #40226: Weird failure on Windows with OUTPUT_SYNC_TARGET

Submitter:  None
Submitted:  Wed 09 Oct 2013 10:27:54 PM UTC
   
 
Severity:  3 - Normal Item Group:  Bug
Status:  Fixed Privacy:  Public
Assigned to:  psmith Open/Closed:  Closed
Component Version:  4.0 Operating System:  None
Fixed Release:  4.1 Triage Status:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group administrator
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/).


c:\Workspace\src\git\make>gnumake.exe -f Makefile.bug -Otarget
Hello
gnumake.exe: *** internal error: multiple --sync-mutex options.  Stop.


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:


$ cat Makefile.bug
$(info Hello)
all:
        @echo All done


$ WinDebug/make.exe -f Makefile.bug -Otarget
Hello
make: *** internal error: multiple --sync-mutex options.  Stop.

$ cat Makefile.ok
all:
        $(info Hello)
        @echo All done
$ WinDebug/make.exe -f Makefile.ok -Otarget
Hello
All done


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>
Group 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>
Group 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>
Group administrator
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>
Group 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>
Group administrator
Sun 13 Oct 2013 03:35:15 PM UTC, comment #3: 

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


$ /C/Workspace/src/git/make/WinDebug/make -Otarget -f no-Makefile
make: no-Makefile: No such file or directory
make: *** internal error: multiple --sync-mutex options.  Stop.


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


   +--------+
 0 |    x--------.
   +--------+    |
 1 |        |    |
   +--------+    |
 2 | "0xf4" |<---'
   +--------+


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.


   +--------+
 0 |    x--------.
   +--------+    |
 1 |    x-----------.
   +--------+    |  |
 2 |      0 |<---'  |
   +--------+       v
                  "0xf4" (somewhere else in memory)


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


  for (pp=output_sync_option->list; *pp; ++pp)


Some others iterate up to idx:


      for (idx = 1; idx < sync_mutex->idx; idx++)


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


      sync_mutex->list = xmalloc (sizeof (char *));
      sync_mutex->list[0] = xstrdup (hdl_string);
      sync_mutex->idx = 1;
      sync_mutex->max = 1;


(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 Files:
   
   
Comment:
   

Attached Files
file #29345:  0001-Fix-initialization-of-sync_mutex-and-jobserver_fds-o.patch added by cboos (2KiB - 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
  • -email is unavailable- added by psmith (Posted a comment)
  • -email is unavailable- added by eliz (Posted a comment)
  • -email is unavailable- added by cboos (Updated the item)
  • -email is unavailable- added by cboos
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2013-11-24 psmith StatusNone Fixed
        Assigned toNone psmith
        Open/ClosedOpen Closed
        Fixed ReleaseNone 4.1
    2013-10-10 cboos Attached File- Added 0001-Fix-initialization-of-sync_mutex-and-jobserver_fds-o.patch, #29345
    2013-10-09 cboos Carbon-Copy- Added cboos
    2013-10-09 None Attached File- Added 0001-Fix-test-detecting-when-a-stringlist-must-grow-in-de.patch, #29339

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code