Bug 265291 - x11-wm/fvwm3- FvwmIconMan module crashes on FreeBSD 13 in virtual machine
Summary: x11-wm/fvwm3- FvwmIconMan module crashes on FreeBSD 13 in virtual machine
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Felix Palmen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-07-18 10:20 UTC by ax61
Modified: 2022-08-02 12:34 UTC (History)
3 users (show)

See Also:
zirias: maintainer-feedback+
zirias: merge-quarterly+


Attachments
"bt full" output from core dump via gdb; fvwm3 at commit f0ed978, c 20220526 (1.62 KB, text/plain)
2022-07-18 10:20 UTC, ax61
no flags Details
fvwm3 FvwmIconMan test patch (608 bytes, patch)
2022-07-18 23:29 UTC, Felix Palmen
zirias: maintainer-approval-
Details | Diff
0001-x11-wm-fvwm3-Avoid-crash-in-FvwmIconMan (1.72 KB, patch)
2022-07-21 09:57 UTC, Felix Palmen
zirias: maintainer-approval-
Details | Diff
0001-x11-wm-fvwm3-Avoid-crash-in-FvwmIconMan (2.69 KB, patch)
2022-07-21 11:11 UTC, Felix Palmen
zirias: maintainer-approval+
Details | Diff
0001-x11-wm-fvwm3-Avoid-crash-in-FvwmIconMan (2.70 KB, patch)
2022-07-23 07:13 UTC, Felix Palmen
zirias: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ax61 2022-07-18 10:20:31 UTC
Created attachment 235328 [details]
"bt full" output from core dump via gdb; fvwm3 at commit f0ed978, c 20220526

I have ...
- FreeBSD 13 (commit fb231965f9b, c 20211116) running inside VirtualBox 6.30 on Windows on ThinkPad X260 (Intel i5-6300U);

- FreeBSD 14 (commit c11e64ce513, c 20220624) runs directly on some ASUS motherboard (Intel i5-8400).


When running either of x11-wm/fvwm3 1.0.4 or fvwm3 compiled from Github repository (at commit f0ed978, c 20220526) on FreeBSD 13 virtual machine, FvwmIconMan module crashes with segmentation fault (backtrace of core dump is attached).

Mind that FvwmIconMan module of fvwm3 compiled at commit 4d5a778b49 c 20211017 does not crash on FreeBSD 13 (in VM).


OTOH there has been no crash of FvwmIconMan module on FreeBSD 14 (fvwm3 compiled at the above commit of f0ed*).


I had also filed the bug with the project ...

  FvwmIconMan crashes with segmentation fault some time after 20211017
  https://github.com/fvwmorg/fvwm3/issues/659
Comment 1 Felix Palmen freebsd_committer freebsd_triage 2022-07-18 23:29:53 UTC
Created attachment 235341 [details]
fvwm3 FvwmIconMan test patch

I'm using fvwm3-1.0.4 myself on 13.1-RELEASE, and I have FvwmIconMan as a part of my taskbar, and never had problems. I also believe it's unlikely that running in a VM could have any effect. What's quite likely is that configuration (especially of FvwmIconMan itself) matters, depending on it, you're hitting different code paths and just one of them might have a bug. Did you do a careful comparison between your two machines?

Technically, from your stacktrace, I assume what's immediately leading to the crash is an uninitialized field in the struct here:
https://github.com/fvwmorg/fvwm3/blob/1.0.4/modules/FvwmIconMan/x.c#L743

Could you please test the attached patch?

It just initializes the offending field to NULL, probably not what that code should do, but if it avoids the crash, you can pass it upstream as a hint for them to find the *correct* solution...
Comment 2 ax61 2022-07-19 01:58:34 UTC
(In reply to Felix Palmen from comment #1)

I will test the patch later in few hours.

FvwmIconMan crashes entirely with the default configuration of fvwm3 port or  built from the Git repository.

As for the comparison between the 2 machines, what exactly should I check for? I had listed the general set up for background & non-crash on one; did not compare as hardware- & OS-wise the 2 machines are disparate.

I could check the behaviour on FreeBSD 14 in new virtual machine on the laptop. I am not inclined to test with FreeBSD 13 on the ASUS machine (mid tower/"desktop") due to ZFS root pool not being recognized on boot when I tried to switch from -CURRENT TO -STABLE earlier, had to reinstall.
Comment 3 Cy Schubert freebsd_committer freebsd_triage 2022-07-19 02:45:29 UTC
(In reply to Felix Palmen from comment #1)
I will set maintainer feedback back to "?". Set it to "+" if you want it committed or "-" for not.

You can commit a patch, including any Makefile adjustments, to a branch of your git repo. Then run git format-patch -1. Attach the patch created by git-format-patch to this PR and I will import your patch, using git-am. Your name/email address will become the author of the patch in the git repo.
Comment 4 Felix Palmen freebsd_committer freebsd_triage 2022-07-19 06:42:02 UTC
(In reply to anubhav.x61+freebsd from comment #2)
If you're sure it is *not* the configuration, it might not be *too* worthwhile testing a lot of other things. If the crash is caused by what I assume it is, it's a case of "undefined behavior", and you can have a really hard time figuring out what circumstances actually make it crash. So, I'd only try to look further if the attached patch does not avoid the crash.

If it does, we could probably add it as a temporary workaround to the port until there's a real fix in upstream's repo.
Comment 5 Felix Palmen freebsd_committer freebsd_triage 2022-07-19 06:45:42 UTC
(In reply to Cy Schubert from comment #3)
There's no need to request feedback from me again now, I monitor this PR. As for committing, aren't you confusing the -feedback flag with the -approval flag on individual attachments? Of course I'll set this to + when I think something should be committed. And sure, I always use git-am format for patches to be committed since we're on git, it's really nice and comfortable :)
Comment 6 Cy Schubert freebsd_committer freebsd_triage 2022-07-19 12:21:02 UTC
I will commit the patch then.
Comment 7 Felix Palmen freebsd_committer freebsd_triage 2022-07-19 12:27:04 UTC
A "committable" version is already prepared, just waiting for feedback because I can't reproduce the crash myself...
Comment 8 commit-hook freebsd_committer freebsd_triage 2022-07-19 12:44:02 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=1d7c23d087f79fa348a421cc4129e613f7086402

commit 1d7c23d087f79fa348a421cc4129e613f7086402
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2022-07-19 12:34:07 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2022-07-19 12:43:15 +0000

    x11/fvwm3: Fix FvwmIconMan module segfault

    Initialize the offending field in the fscreen_scr_arg struct to NULL,
    fixing the segfault.

    PR:             265291
    Submitted by:   Felix Palmen <felix@palmen-it.de> (Maintainer)
    Reported by:    anubhav.x61+freebsd@gmail.com
    Approved by:    Felix Palmen <felix@palmen-it.de> (Maintainer)

 x11-wm/fvwm3/Makefile                                  |  2 +-
 x11-wm/fvwm3/files/patch-modules_FvwmIconMan_x.c (new) | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)
Comment 9 Cy Schubert freebsd_committer freebsd_triage 2022-07-19 12:44:39 UTC
(In reply to Felix Palmen from comment #7)
I've committed this one already because feedback is "+". If you did not want this to be committed, the + should have been removed. This is the way it works.
Comment 10 Felix Palmen freebsd_committer freebsd_triage 2022-07-19 12:46:42 UTC
Please read https://wiki.freebsd.org/Bugzilla/DosAndDonts, especially:

> DON'T use maintainer-feedback flag to declare approval of a patch.
> (DO use the maintainer-approval flag)

No, this was not approved for commit, please reopen.
Comment 11 Cy Schubert freebsd_committer freebsd_triage 2022-07-19 13:15:57 UTC
The maintainer approved the commit. The patch has been committed.

I can reopen this if you want though. I'm not sure why.
Comment 12 Cy Schubert freebsd_committer freebsd_triage 2022-07-19 13:25:37 UTC
The maintainer feedback flag is there for maintainer approval of the patch. When the maintainer changes the flag from ? to + the maintainer approves. If the flag is changed from ? to - the maintainer disapproves.

The email I get when you change the ? to + is,

Subject: maintainer-feedback granted: [Bug 265291] x11-wm/fvwm3- FvwmIconMan
 module crashes on FreeBSD 13 in virtual machine

You have granted the approval to commit. This is how it works. I had reset approval thinking you may have misunderstood what the flag meant but you had approved the patch, again, meaning you really meant that the patch be committed. 

At this point you can fine tune the patch here or open a new PR to have the patch removed or modified in some way.
Comment 13 Felix Palmen freebsd_committer freebsd_triage 2022-07-19 13:27:46 UTC
This is completely wrong. "feedback" has a *very* different meaning than "approval".

I did NOT approve this patch. I never had any such issue in the past, as people typically understand "maintainer-approval" is the flag to give approval for commit.
Comment 14 commit-hook freebsd_committer freebsd_triage 2022-07-19 13:35:10 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=c112b84fd8076a19f30f0705a3c139ef360b101e

commit c112b84fd8076a19f30f0705a3c139ef360b101e
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2022-07-19 13:31:06 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2022-07-19 13:34:06 +0000

    Revert "x11/fvwm3: Fix FvwmIconMan module segfault"

    The maintainer misunderstood the meaning of the maintainer-feedback
    flag in bugzilla.

    PR:             265291

    This reverts commit 1d7c23d087f79fa348a421cc4129e613f7086402.

 x11-wm/fvwm3/Makefile                                   |  2 +-
 x11-wm/fvwm3/files/patch-modules_FvwmIconMan_x.c (gone) | 10 ----------
 2 files changed, 1 insertion(+), 11 deletions(-)
Comment 15 Gleb Popov freebsd_committer freebsd_triage 2022-07-19 13:45:41 UTC
(In reply to Cy Schubert from comment #12)

> The maintainer feedback flag is there for maintainer approval of the patch. When the maintainer changes the flag from ? to + the maintainer approves. If the flag is changed from ? to - the maintainer disapproves.

Where did you get that? The info bubble that appears on hover over "maintainer feedback" says:

* Set (+) when you provide feedback. Avoids "maintainer timeout" (MAINTAINER)
Comment 16 Cy Schubert freebsd_committer freebsd_triage 2022-07-19 13:54:24 UTC
(In reply to Gleb Popov from comment #15)
I don't see any popup with FF.

Anyhow, any time I've submitted patches and asked for explicit approval, because I've missed that the maintainer had already set the flag to +, and have been told that it meant it was approved.

Seems different people interpret this flag differently rendering it virtually meaningless.
Comment 17 Felix Palmen freebsd_committer freebsd_triage 2022-07-19 14:03:24 UTC
Comment on attachment 235341 [details]
fvwm3 FvwmIconMan test patch

The documentation is IMHO unambiguous, but this doesn't seem to stop "different interpretations".

But let's stop this please, I'm just waiting for feedback from anubhav.x61 whether this workaround has the desired effect or not. If it has, I have prepared a commit. Otherwise, we'll have to investigate further, or wait for upstream...

Setting approval explicitly to '-' now, I should probably adopt this as best practice for any "test patches".
Comment 18 ax61 2022-07-21 09:39:00 UTC
(In reply to Felix Palmen from comment #1)

Thank you, dear maintainer! I had manually edited the file (FvwmIconMan/x.c) to make the sole change of addition of the line in the Git repository checkout at commit f0ed978, c 20220526. That allowed FvwmIconMan to work!

I could not test the port as was stuck with stage directory path being mangled during installation (could be some setting that is tickling it; IIRC "pkg" (1.18.3 here) recently became aware of "flavor") ...

...
===>   Registering installation for fvwm3-1.0.4_5
pkg-static: Unable to access file /build/ports/src-ports/ports/x11-wm/fvwm3/work/stage/usr/local/share/py310-fvwm3/ConfigFvwmBacker:No such file or directory
pkg-static: Unable to access file /build/ports/src-ports/ports/x11-wm/fvwm3/work/stage/usr/local/share/py310-fvwm3/ConfigFvwmButtons:No such file or directory
pkg-static: Unable to access file /build/ports/src-ports/ports/x11-wm/fvwm3/work/stage/usr/local/share/py310-fvwm3/ConfigFvwmDefaults:No such file or directory
...

... "p310-" is there because I have set DEFAULT_VERSIONS for Python to 3.10 in /etc/make.conf. If I comment out that line, then "py39-" is inserted instead. Do note that the paths without "py310-" (or, "py39-") do exist.
Comment 19 Felix Palmen freebsd_committer freebsd_triage 2022-07-21 09:57:13 UTC
Created attachment 235402 [details]
0001-x11-wm-fvwm3-Avoid-crash-in-FvwmIconMan

(In reply to anubhav.x61+freebsd from comment #18)
Ok thanks, I think it's good enough to know adding this initialization indeed avoids the crash.

Therefore we should add it as a workaround until upstream provides a "correct" fix.

Also commented on the upstream issue, hoping it will help them.
Comment 20 Felix Palmen freebsd_committer freebsd_triage 2022-07-21 10:34:51 UTC
Comment on attachment 235402 [details]
0001-x11-wm-fvwm3-Avoid-crash-in-FvwmIconMan

Sorry, not good enough for 1.0.4 ... it also misses code checking for NULL that's present in newer git revisions. I'll rework this!
Comment 21 Felix Palmen freebsd_committer freebsd_triage 2022-07-21 11:11:12 UTC
Created attachment 235403 [details]
0001-x11-wm-fvwm3-Avoid-crash-in-FvwmIconMan

Ok, but now, added some changes backported from main to correctly handle a NULL value in this field.

* tested it still works for me (FvwmIconMan runs fine both "swallowed" in
  FvwmButtons and launched manually from FvwmPrompt)
* successful poudriere testport run on 13.1-amd64
Comment 22 Felix Palmen freebsd_committer freebsd_triage 2022-07-23 07:13:01 UTC
Created attachment 235437 [details]
0001-x11-wm-fvwm3-Avoid-crash-in-FvwmIconMan

Only change is the commit message to reflect the request for MFH. As it's a bugfix indeed, this makes sense, thanks cy@ for the suggestion!
Comment 23 commit-hook freebsd_committer freebsd_triage 2022-07-28 12:58:02 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=1b4b3e9ce1e7255519f84324f3a7a4c1d7cd3f33

commit 1b4b3e9ce1e7255519f84324f3a7a4c1d7cd3f33
Author:     Felix Palmen <felix@palmen-it.de>
AuthorDate: 2022-07-18 23:20:36 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2022-07-28 12:56:00 +0000

    x11-wm/fvwm3: Avoid crash in FvwmIconMan

    Add a patch working around the crash by initializing some struct member
    to NULL that's otherwise used uninitialized and backporting some code
    from upstream's main branch to correctly handle that case.

    PR:             265291
    MFH:            2022Q3

 x11-wm/fvwm3/Makefile                   |  2 +-
 x11-wm/fvwm3/files/patch-pr265291 (new) | 51 +++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)
Comment 24 commit-hook freebsd_committer freebsd_triage 2022-07-28 16:22:39 UTC
A commit in branch 2022Q3 references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=1a37d438f0b40c8a3dd5f098b7b8aac6ad378b1b

commit 1a37d438f0b40c8a3dd5f098b7b8aac6ad378b1b
Author:     Felix Palmen <felix@palmen-it.de>
AuthorDate: 2022-07-18 23:20:36 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2022-07-28 16:21:20 +0000

    x11-wm/fvwm3: Avoid crash in FvwmIconMan

    Add a patch working around the crash by initializing some struct member
    to NULL that's otherwise used uninitialized and backporting some code
    from upstream's main branch to correctly handle that case.

    PR:             265291

    (cherry picked from commit 1b4b3e9ce1e7255519f84324f3a7a4c1d7cd3f33)

 x11-wm/fvwm3/Makefile                   |  2 +-
 x11-wm/fvwm3/files/patch-pr265291 (new) | 51 +++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)