Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macOS crash unknown sockaddr #3

Closed
ChristopherHX opened this issue Jun 27, 2020 · 17 comments
Closed

macOS crash unknown sockaddr #3

ChristopherHX opened this issue Jun 27, 2020 · 17 comments
Labels
bug Something isn't working

Comments

@ChristopherHX
Copy link
Member

I would suggest skip copying unknown socket types to bionic and avoid a crash,
or better log them in debug mode.
cannot test myself, reported issues.

*res = bionic::from_host_alloc(hres);

bionic::from_host(&hbuf.ifc_req[i].ifr_addr, &buf->req->addr);

@ChristopherHX ChristopherHX added bug Something isn't working enhancement New feature or request help wanted Extra attention is needed labels Jun 27, 2020
@MCMrARM
Copy link
Member

MCMrARM commented Jun 30, 2020

probably should only crash on shim errors in debug mode, and for release only log them

@MCMrARM
Copy link
Member

MCMrARM commented Jun 30, 2020

there's a good chance the second one got fixed though

@ChristopherHX
Copy link
Member Author

second one still crash with current libc-shim, unknown macOS version
minecraft-linux/mcpelauncher-manifest#341 (comment)
But now more somehow get a crash, most of them don't post a log.

@andy-mortimer
Copy link

Moving over from the referenced issue. I'm on 10.15.5. I've checked NG version 0.0.168 from https://github.com/ChristopherHX/osx-packaging-scripts/releases/tag/ng.dmg and confirmed that it does work, where the newer 0.0.175 does not. The last parts of the log probably don't help, but just in case it's useful:

linker: dlsym(handle=0x0("n/a"), sym_name="glPopGroupMarkerEXT", sym_ver="(null)", caller="/Users/andy-user/Library/Application Support/mcpelauncher/versions/1.16.0.2/lib/x86_64/libminecraftpe.so", caller_ns=@0x1081277f0) ...
linker: dlsym failed: invalid handle: 0x0
linker: ... dlsym failed: dlsym failed: invalid handle: 0x0
21:01:12 Warn  [Minecraft] NO LOG FILE! - [Device Lost] The graphics context was gained
21:01:12 Warn  [Minecraft] MinecraftGame::init && MinecraftGame::setSize!
21:01:13 Debug [XSAPI.Android] sign_in_impl: will NOT be showing UI
21:01:13 Info  [XboxInterop] InvokeMSA: requestCode=1 cid=6cf42dea7aadd41d
21:01:13 Trace [DaemonLauncher] Starting daemon: /Applications/Minecraft Bedrock Launcher.app/Contents/MacOS/./msa-daemon
21:01:18 Info  [Minecraft] NO LOG FILE! - IPv4 supported, port: 56662
21:01:18 Info  [Minecraft] NO LOG FILE! - IPv6 supported, port: 56663
21:01:18 Info  [Minecraft] NO LOG FILE! - IPv4 supported, port: 57562
21:01:18 Info  [Minecraft] NO LOG FILE! - IPv6 supported, port: 57563

... and then successfully shows the main menu. Logs from 0.0.175 at minecraft-linux/mcpelauncher-manifest#341 (comment) .

Let me know if there's anything else I can do to help test / narrow down the issue. I have XCode and am an experienced Windows developer but I'm fairly new to MacOS and its tools and libraries.

@ChristopherHX ChristopherHX changed the title macOS 10.10 - 10.13 crash unknown sockaddr macOS crash unknown sockaddr Jul 10, 2020
@ChristopherHX ChristopherHX removed the enhancement New feature or request label Jul 10, 2020
@ChristopherHX
Copy link
Member Author

ChristopherHX commented Jul 10, 2020

It would help if you could debug this line with gdb

default: throw std::runtime_error("Unknown socket family when converting sockaddr from host");

Then provide us the value of in->sa_family.

src https://github.com/minecraft-linux/mcpelauncher-manifest/tree/ng

git clone https://github.com/minecraft-linux/mcpelauncher-manifest --recursive -b ng
mkdir build
cd build
cmake .. -DCMAKE_BUILD_TYPE=Debug
make -j8 mcpelauncher-client

You could run it from a terminal

mcpelauncher-client/mcpelauncher-client -dg ~/Library/Application\ Support/mcpelauncher/versions/1.16.1.02

Debugging are more steps
I'm like you more a windows developer

@andy-mortimer
Copy link

It seems to be AF_LINK. I couldn't get gdb to work (got lost trying to get it codesigned) but printing the value to cerr gives 18, which is defined as AF_LINK in
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/sys/socket.h, and adding a case for AF_LINK does trigger:

diff --git a/src/network.cpp b/src/network.cpp
index e00207f..0ea906c 100644
--- a/src/network.cpp
+++ b/src/network.cpp
@@ -103,7 +103,11 @@ void bionic::from_host(const ::sockaddr *in, bionic::sockaddr *out) {
             memcpy(((bionic::sockaddr_in6 *) out)->addr, ((::sockaddr_in6*) in)->sin6_addr.s6_addr, 16);
             ((bionic::sockaddr_in6 *) out)->scope = ((::sockaddr_in6*) in)->sin6_scope_id;
             break;
-        default: throw std::runtime_error("Unknown socket family when converting sockaddr from host");
+       case AF_LINK:
+           throw std::runtime_error("AF_LINK socket family when converting sockaddr from host");
+        default: 
+           //std::cerr << "af_family " << (int)in->sa_family << std::endl;
+           throw std::runtime_error("Unknown socket family when converting sockaddr from host");
     }
 }
 

@andy-mortimer
Copy link

This crash appears to be triggered by 01d8ebb . If I go back to ab54671, it goes away.

Looking at the commit and doing a little googling, it looks like this is indeed fixing things compared to the original implementation, so presumably this is one of those problem-hides-another-problem cases and reverting this commit probably isn't the fix! I will see if I can find any more, but I'm a bit in the dark as to what this code is all about ... any clues welcome

@MCMrARM
Copy link
Member

MCMrARM commented Aug 14, 2020

Proper fix is to add AF_LINK to the translation layer, no?

@andy-mortimer
Copy link

Yes, I think so. I will try it when I get a chance. I'm still learning my way around, so the little time I have for this doesn't progress me very fast!

@andy-mortimer
Copy link

It wasn't that easy! PR #4 updated to ignore them, let me know what you think.

@ChristopherHX
Copy link
Member Author

I think found a problem, the loop is wrong for macOS
https://gist.github.com/OrangeTide/909204#file-showif-c-L60
sizeof ifreq is not the way to go on macOS

@MCMrARM
Copy link
Member

MCMrARM commented Aug 18, 2020

Seems the correct way to do it is using _SIZEOF_ADDR_IFREQ if available. See: https://opensource.apple.com/source/postfix/postfix-174/postfix/src/util/inet_addr_local.c.auto.html for an example

#if defined(_SIZEOF_ADDR_IFREQ)
#define NEXT_INTERFACE(ifr)	((struct ifreq *) \
	((char *) ifr + _SIZEOF_ADDR_IFREQ(*ifr)))
#define IFREQ_SIZE(ifr)	_SIZEOF_ADDR_IFREQ(*ifr)
#elif defined(HAS_SA_LEN)
#define NEXT_INTERFACE(ifr)	((struct ifreq *) \
	((char *) ifr + sizeof(ifr->ifr_name) + ifr->ifr_addr.sa_len))
#define IFREQ_SIZE(ifr)	(sizeof(ifr->ifr_name) + ifr->ifr_addr.sa_len)
#else
#define NEXT_INTERFACE(ifr)	(ifr + 1)
#define IFREQ_SIZE(ifr)	sizeof(ifr[0])
#endif

That's unexpected to be honest thanks for investigating it @ChristopherHX .

ChristopherHX added a commit that referenced this issue Sep 2, 2020
Fix only filling the first ifreq entry of bionic's array
Addresses parts of #3
@ChristopherHX
Copy link
Member Author

Calling this method manually make it reproduceable for me, will add tests to this launcher soon.

My dump of inet families of the ioctl
AF_LINK=18
AF_INET6=30
AF_INET=2
Should be fixed soon in ng, ignoring AF_LINK to match linux

ifr_name: 'lo0'
sa_family: 18
ifr_name: 'lo0'
sa_family: 2
ifr_name: 'lo0'
sa_family: 30
ifr_name: 'lo0'
sa_family: 30
ifr_name: 'gif0'
sa_family: 18
ifr_name: 'stf0'
sa_family: 18
ifr_name: 'XHC20'
sa_family: 18
ifr_name: 'EHC29'
sa_family: 18

@ChristopherHX
Copy link
Member Author

We might need to increase the storage for the host ifconf on macOS, because the list is so much longer than on Linux

@ChristopherHX
Copy link
Member Author

ChristopherHX commented Sep 4, 2020

SOCKET_CGIFNETMASK
Needs changes on macOS
ifreq has indeed not the size for storing the to_host addressfamily

oh got this part wrong only accept ipv4 for compatibility.

SIOCGIFCONF also have to return only AF_INET and skip ipv6 etc.for binary compatibility to linux apps

I will close this as fixed with the next release, no errors should happen after latest commits from today.

@ChristopherHX ChristopherHX removed the help wanted Extra attention is needed label Sep 4, 2020
@ChristopherHX ChristopherHX reopened this Sep 10, 2020
@ChristopherHX
Copy link
Member Author

Can't keep up.
I might fixed this bug, but this spawened a new one with cxa_guard...

@ChristopherHX
Copy link
Member Author

Has nothing to do with this issue, it is syscall()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants