igmp: Avoid leaving dangling pointers in the state-change queue

[markj]

Summary

When igmp_v3_merge_state_changes() is iterating over state-change
packets, there is a case where it'll drop packets from the queue but
fail to remove them. Fix that.

Reported by: Yuxiang Yang, Yizhou Zhao, Xuewei Feng, Qi Li, and Ke Xu from Tsinghua University using GLM5.1 from Z.ai

[markj]

sys/netinet/igmp.c

3325

I don't quite understand this check--is the intent to check mbufq_full(scq)? It rather looks like it, especially given that the mbufq_enqueue() call below in the !domerge case is not checked.

3330

Why exactly do we drop the packet only in the !docopy case?

[bms]

Yeah. Don't have much to offer here, I'm afraid, ironic being the instigator, but just so much is going on at the moment, and I'm literally RAMMED every DAY. The mental map of this code is long gone from the forebrain, but probably lingers somewhere, but I'm not about to pop Piracetam or Choline Inositol to find out this week. (Those are nootropics; the latter are nutraceuticals.)

It might be worth doing a contrast study with xnu to see if AAPL picked it up. I think there's a snowball's chance in hell of them submitting patches to US any time soon, though.

"Remember, my dear, I can smell a lie like a fart in a car." -- Mark Hunter, as played by Christian Slater, "Pump Up The Volume" (1990)

P.S. Kind of ironic it was Tsinghua folk who found this. Technically I was in an R&D race with them at one point which turned out to be a juicy nothingburger.

[bms]

I concur that commit-id 855239e56 of xnu/bsd/netinet/igmp.c from https://github.com/apple-oss-distributions/xnu appears to have a similar fix, but they explicitly split the conditional further. The "fog of war" of xnu imports in "git log" prevents further instrumentation beyond the obvious "git blame" "where the bodies are buried" action. (Hey, I had a git sparse-checkout lying around in my primary FreeBSD VM dev instance. Go ME! But that was also part of the ongoing xnu dissection, "Take what is useful, destroy the rest." Developer Hegemony is currently looking like a bit of a pipe dream, unless people start taking what Dijkstra said in 1975 about "complexity generators" seriously.)
Gotta go, I'm out of actual food, so I need to go to the shop, respond to my other GSoC student meaningfully, and tune in to "Auntie" (Prof.) Emily Bender at 2000BST on Twitch.
UPDATE: OK, that was not as cool as I thought it would be, and I've eaten too many crisps and cheap sushi. She was mostly poking fun at people's huge AGENTS.md files on GitHub.

[glebius]

In the commit text, in the phrase it'll drop packets from the queue but fail to remove them, I'd suggest to change drop to free or m_freem.

sys/netinet/igmp.c

3334

Why did you change from using mt as pointer to m0?

[bms]

In D56947#1304518, @glebius wrote:

In the commit text, in the phrase it'll drop packets from the queue but fail to remove them, I'd suggest to change drop to free or m_freem.

Apple were using "n" just to be gauche. https://github.com/apple-oss-distributions/xnu/blob/main/bsd/netinet/igmp.c#L3892

[markj]

sys/netinet/igmp.c

3334

Just because mt is overloaded as the last pointer in scq, whereas m0 is already used as a temporary variable elsewhere in the loop.