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

Android API Level 30 issues #46

Open
mzanetti opened this issue Sep 9, 2021 · 39 comments
Open

Android API Level 30 issues #46

mzanetti opened this issue Sep 9, 2021 · 39 comments

Comments

@mzanetti
Copy link
Contributor

mzanetti commented Sep 9, 2021

With Android API level 30, registering the zoerconf browser fails with

W qtMainLoopThrea: type=1400 audit(0.0:8386): avc: denied { ioctl } for path="socket:[6380337]" dev="sockfs" ino=6380337 ioctlcmd=0x8927 scontext=u:r:untrusted_app:s0:c154,c256,c512,c768 tcontext=u:r:untrusted_app:s0:c154,c256,c512,c768 tclass=tcp_socket permissive=0 app=io.guh.nymeaapp

The failing call is in netlink.c, in avahi_netlink_new

    if (bind(fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) {

fails with "Permission denied".

Now that new apps can't be uploaded any more with an API level less than 30 (and existing apps only until end of November) this becomes a real problem. I'm still debugging how to get it to work again but so far couldn't find anything.

@jbagg
Copy link
Owner

jbagg commented Sep 9, 2021

Hi, Have you added android.permission.INTERNET in the manifest?

https://developer.android.com/reference/android/Manifest.permission#INTERNET

@mzanetti
Copy link
Contributor Author

mzanetti commented Sep 9, 2021

Yes, using QT += network does that implicitly. Anyhow, I've also tried adding it explicitly, same.

According to this report arvidn/libtorrent#6251 bind() is not available any more now. Just commenting this out for testing purposes makes the code pass, but then fails at avahi_netlink_send, send().

That said, there must be a solution as Qt <= 5.15.1 also has troubles with the QNetworkConfiguration class and API level 30, but it's working fine again with 5.15.2 but I suppose it requires to patch the avahi code.

@mzanetti
Copy link
Contributor Author

mzanetti commented Sep 9, 2021

AFAICT, this is all about the interface enumeration of the avahi_interface_monitor... And apparently, starting with a minimum API level of 24, getifaddrs() may be used instead. That would be one option I guess, not yet sure if it's the best, or even if that's all to it or if there will be more obstacles after this.

@mzanetti
Copy link
Contributor Author

mzanetti commented Sep 9, 2021

Another option, I suppose, would be to ditch avahi for and android case and instead provide a Java implementation using https://developer.android.com/training/connect-devices-wirelessly/nsd#java
Downside would be that users of the lib on android would need to make sure that the java files are compiled in their gradle file. Not sure if that's a viable option.

@jbagg
Copy link
Owner

jbagg commented Sep 9, 2021

I don't know of a way to asynchronously get updates to interfaces using getifaddrs the way netlink provides. Avahi needs this. I suspect if I / we modify avahi to use getifaddrs, it would have to have a timer that polls it once / second, which seams like a horrible thing to do. QZeroConf will probably have to use the java nsd as you suggest. I have started looking in the past at using nsd as it is not ideal to have more than one mdns daemon on a system. Java is not my strong suit, but I found some useful stuff this morning
https://developer.vuforia.com/forum/faq/android-how-can-i-call-java-methods-c

https://developer.android.com/ndk/samples/sample_hellojni

@mzanetti
Copy link
Contributor Author

mzanetti commented Sep 9, 2021

Yeah, I'm already using some Java code in my app and mostly got the hang of it on how to interact between Qt and the android JNI. I'll give it a go and report back.

@mzanetti
Copy link
Contributor Author

mzanetti commented Sep 10, 2021

Got it working! Here's a preview: mzanetti#2

There's still a lot cleanup to do. It's merely a prove of concept for now. So far it only supports browsing, no publishing, but the way it works would be the same for everything. Tested on a Pixel 3 with Android 11 and nymea:app.

Currently it's a PR against my fork because I have some other commits on top too so the diff would be polluted. Unless you stop me because you think there's something wrong, I would now go ahead, fill in the gaps and then propose a cleaned up pull request towards upstream.

Currently I just added the folder with the java file to my build.grade file and it works fine. Apart form making the actual code work, there's still the task to figure out a nice way of allowing user of the lib somehow to just "include qtzeroconf.gradle" or something like that without having to bother about the actual files to be built by the Java compiler.

@jbagg
Copy link
Owner

jbagg commented Sep 10, 2021

Looks like a good start. If I understand it right, if a program has multiple instances of QZeroConf, each instance of QZeroConf will get it's own nsdService, right? I should create a androidjni branch that you can make pull request against.

@mzanetti
Copy link
Contributor Author

Yes, every instance of QtZeroConf would create its own instance of the java class which in turn creates a new instance of the NsdManager. This is very much like the avahiclient implementation. NsdManager is really just a thin client to Androids system service.

We could create a single instance and share that across instances of QtZeroConf using some ref counting as the avahicore one does. This current way IMO simplifies the implementation and should also work fine if users instantiate multiple QtZeroConf objects across different threads. Such a shared mechanism would need to be mutexed and all (well, I guess the avahicore implementation doesn't really cope with that either so it's probably not a supported use case anyways). In any case, the benefits of a single shared NsdManager approach seem to be smaller than the downsides to me currently.

For the branches, I don't think you need to do anything, I'll just have to rebase my branch on your master instead of mine and propose a pull request against that. Unless you don't want to merge it to master when it's done but keep it only in an androidjni branch for a while.

@jbagg
Copy link
Owner

jbagg commented Sep 10, 2021

I created a branch called android-nds. This will allow us to test it well before merging to master. Have you used clang-format before?

@jbagg
Copy link
Owner

jbagg commented Sep 10, 2021

I agree, I think not sharing a single instance of NsdManager is the right way. Each QZeroConf instance should have it's own instance of NsdManager

@mzanetti
Copy link
Contributor Author

I created a branch called android-nds. This will allow us to test it well before merging to master. Have you used clang-format before?

Hmm, no. Is this about the code formatting?

@mzanetti
Copy link
Contributor Author

mzanetti commented Sep 10, 2021

While heads down in the code I ran into a couple of questions/issues:

  • I noticed that androids browser will resolve service types in the format _http._tcp.. Note the . at the end. The avahi implementation gives results as _http._tcp. I could of course trim the dot at the end, but while doing so, I noticed that the bonjour implementation gives services as ._http._tcp (No dot at the end but a dot at the start). Now I'm not sure, seems that this should be aligned through the lib, but obviously changing one or the other would mean sort of an API break... WDYT? Just align it to the avahi implementation to keep old behavior, or also align the bonjour one?

  • Calling stopPublishService() on avahi implementations would only cause publisherExists() to return false, however, on the bonjour implementation it would additionally emit serviceRemoved(). Again, I suppose I'll follow the avahi implementation and not emit serviceRemoved, right?

  • Name collision handling differs too: While the bonjour implementation does not ever seem to fail on name collisions (at least the error() is never emitted), the avahi implementations would bail out with error(serviceNameCollision). Interestingly enough, the android API behavior is to automatically resolve those collisions (by implicitly renaming) and returning the new name back. So I guess I could emit the error for name collision but still emitting servicePublished() and setting publisherExists to true. But not sure if that's weird for a user and I should just not emit the error at all?

@mzanetti
Copy link
Contributor Author

I've pushed the latest code to my branch. It is now feature complete but still not 100% cleaned up and the above questions need to be sorted out still.

I wanted to rebase it onto your master, but ran into conflicts wrt UBPorts port. Depending on whether the UBPorts edition will be accepted upstream or not, the conflicts would need to be resolved differently. See #48

@jbagg
Copy link
Owner

jbagg commented Sep 11, 2021

WDYT? Just align it to the avahi implementation to keep old behaviour, or also align the bonjour one?

I would just keep the old (inconsistent) behaviour. I wasn't aware of the difference and no one has raised it before. I guess a user doesn't really care about the domain when browsing, their primary interest is the service's IP address.

@jbagg
Copy link
Owner

jbagg commented Sep 11, 2021

Calling stopPublishService() on avahi implementations would only cause publisherExists() to return false, however, on the bonjour implementation it would additionally emit serviceRemoved(). Again, I suppose I'll follow the avahi implementation and not emit serviceRemoved, right?

I am a bit confused. stopServicePublish() will free the avahi group which will unpublish the service (avahicore.cpp). (maybe a comment should be added to explain this.) In boujour and also avahi implementations serviceRemoved() is tied to browsing, not publishing. serviceRemoved() should only fire if one was browsing.

@jbagg
Copy link
Owner

jbagg commented Sep 11, 2021

Name collision handling differs too:

I think a signal osChangedServiceName() would suffice for nsd services and don't emit an error. Someday in the other implementations we could implement similar renaming functionality.

@mzanetti
Copy link
Contributor Author

mzanetti commented Sep 11, 2021

I would just keep the old (inconsistent) behaviour. I wasn't aware of the difference and no one has raised it before. I guess a user doesn't really care about the domain when browsing, their primary interest is the service's IP address.

Done

I am a bit confused. stopServicePublish() will free the avahi group which will unpublish the service (avahicore.cpp). (maybe a comment should be added to explain this.) In boujour and also avahi implementations serviceRemoved() is tied to browsing, not publishing. serviceRemoved() should only fire if one was browsing.

Right... I was wondering why there's no feedback signal for "stopServicePublish()" and skimmed through the bonjour file. Ended up in cleanUp and saw the emit for serviceRemoved() but missed the "if (browser)" above.... So all good... No feedback for stopServicePublish() it is.

I think a signal osChangedServiceName() would suffice for nsd services and don't emit an error. Someday in the other implementations we could implement similar renaming functionality.

Added that.

@mzanetti
Copy link
Contributor Author

mzanetti commented Sep 11, 2021

Ok... did some more verifications wrt threading (java code runs in a different thread than C++ code) and made sure it's handled properly and thread-safe.

The last thing missing from my point of view would be a nicer way to inject the java file into the gradle build system of apps. Currently, the path to the QZeroConf/android folder needs to be added to java.srcDirs = [...] in the build.gradle in one way or another... Not that it's terribly bad, but probably not the gradle-way-of-things... For some reason I'm always struggling with gradle though and it takes me ages to figure out how to do things with it.

@jbagg
Copy link
Owner

jbagg commented Nov 4, 2021

I've managed to add your changes to the android-nds branch. I see what you mean by a nicer way to inject the java file into the gradle build system of apps now that I've been playing with it. I've created a soft link to QZeroConfNsdManager.java from my java.srcDirs. Maybe I can just make a note on the readme to do the same.

I also fixed two issues and haven't found a solution for a third. If another device unpublishes a service while the android device is sleeping, the android device will still fire a serviceAdded() for the service that has been removed (using Android 8.1 device). When the device sleeps I added some code to emit serviceRemoved() for all the discovered services. This is only part of the solution. I tried adding

discoveryListener = initializeDiscoveryListener(); in discoverServices() in the java hoping it would re-create the discoveryListener but no luck. Not sure how to flush or delete the whole lister or nsd object.

@mzanetti
Copy link
Contributor Author

mzanetti commented Nov 15, 2021

I've looked through your changes. Looks good. Indeed, I am mostly using the library to browser only, so I missed those publish related issues. Thanks for fixing them.

Can't say much about the third one atm. Does this happen if a services is published and removed again while the app is suspended? I guess in this case we should get onServiceFound as well as onServiceLost.. And the issue probably happens because the code starts the resolver in onServiceFound which will return after the onServiceLost.

Wouldn't the proper solution be to remove the resolving from the resolver queue in onServiceLost (and if the resolve is already pending, don't emit the onServiceFound in its callback)?

For the gradle thing: I am quite positive that there should be a way to provide a gradle "include" somehow. So a user of the lib would then somehow list that gradle include file in the dependencies {} part. IMO that would be the way to go, but again, haven't managed to get that going yet with the time I can spend for this. Hopefully some gradle guru stops by here :D

@dwebb72
Copy link

dwebb72 commented Feb 21, 2022

I've created a soft link to QZeroConfNsdManager.java from my java.srcDirs.

Can you expand on what this would look like? Trying to get my app update released but qtzeroconf not working with Android API level 30 is holding me up.

@jbagg
Copy link
Owner

jbagg commented Feb 21, 2022

If you the android-nds branch, most things work. There is one issue I don't know how to resolve. Basically we need to file a bug report with Google. After Android 6 (I've only tested Android 8) if another device adds or removes a service while the Android device is asleep, NsdManager can not detect the new or removed service. I've duplicated this issue using purely java code (but I'm not a java developer, so struggling along). It would be good if someone could try reproducing the issue on Android 9, 10 and 11. There are probably millions of devices this is broken on and will never be fixed. Maybe we need to use a 3rd party java discover library.

@dwebb72
Copy link

dwebb72 commented Feb 23, 2022

I do mean the android-nds branch. I think I figured the java.srcDirs thing out but I am having trouble building the android-nds branch. I get 'QGuiApplication' file not found
../QtZeroConf-android-nds/androidnsd.cpp:27:10: fatal error: 'QGuiApplication' file not found
#include

What version of QT are you using to build?

Dan

@jbagg
Copy link
Owner

jbagg commented Feb 23, 2022 via email

@dwebb72
Copy link

dwebb72 commented Feb 23, 2022

Adding that to the qtzeroconf.pro fixed that error. Now I get the following.
/armeabi-v7a/moc_qt-watch_p.cpp:75: error: error: undefined reference to 'AvahiWatch::gotIn()
/armeabi-v7a/moc_qt-watch_p.cpp:76: error: error: undefined reference to 'AvahiWatch::gotOut()'
/armeabi-v7a/moc_qt-watch_p.cpp:170: error: error: undefined reference to 'AvahiTimeout::timeout()'

@jbagg
Copy link
Owner

jbagg commented Feb 23, 2022 via email

@dwebb72
Copy link

dwebb72 commented Feb 23, 2022

Sorry, I don't think I was clear on this. This is the error I get when building my project using qtzeroconf as a dynamic library.

I am using QT Creator to build on Mac.

I did a "Clean" and then "Run qmake" and them "Build Project" from the Build menu.

I'm not sure how to do the following on Mac.
rm find . -name "*.o"
rm find . -name "moc*"

@mzanetti
Copy link
Contributor Author

mzanetti commented Mar 7, 2022

There is one issue I don't know how to resolve. Basically we need to file a bug report with Google. After Android 6 (I've only tested Android 8) if another device adds or removes a service while the Android device is asleep, NsdManager can not detect the new or removed service. I've duplicated this issue using purely java code (but I'm not a java developer, so struggling along). It would be good if someone could try reproducing the issue on Android 9, 10 and 11. There are probably millions of devices this is broken on and will never be fixed. Maybe we need to use a 3rd party java discover library.

Interestingly I've had this issue with the previous, avahi-core based implementation but I can't see this any more now with the Java based implementation (Pixel 5, Android 12). That said, if it's really an issue and I am just failing to reproduce on this particular device, I for one would rather detect the application state change and re-initialize the mdns discovery upon wakeup, rather than using another 3rd party library...

FWIW, using the Java implementation now for months and gotten zero problem reports.

@fatice8
Copy link

fatice8 commented May 22, 2023

I've managed to add your changes to the android-nds branch. I see what you mean by a nicer way to inject the java file into the gradle build system of apps now that I've been playing with it. I've created a soft link to QZeroConfNsdManager.java from my java.srcDirs. Maybe I can just make a note on the readme to do the same.

I also fixed two issues and haven't found a solution for a third. If another device unpublishes a service while the android device is sleeping, the android device will still fire a serviceAdded() for the service that has been removed (using Android 8.1 device). When the device sleeps I added some code to emit serviceRemoved() for all the discovered services. This is only part of the solution. I tried adding

discoveryListener = initializeDiscoveryListener(); in discoverServices() in the java hoping it would re-create the discoveryListener but no luck. Not sure how to flush or delete the whole lister or nsd object.

I am able to use android-nds branch to setup my app to work in Android 13. However, I need to put the QZeroConfNsdManager.java into my main project, otherwise, nsdManager will not be able to locate the QZeroConfNsdManager class. May I know if there is a way to keeping the java file in the so library?

Thanks.

@jbagg
Copy link
Owner

jbagg commented May 22, 2023

Yes, putting QZeroConfNsdManager.java in your main project is the only way I know how it can be done.

@fatice8
Copy link

fatice8 commented May 22, 2023

Yes, putting QZeroConfNsdManager.java in your main project is the only way I know how it can be done.

Thanks for the reply.

However, the LGPL does not allow a program to take any code from the library. May I know if there is another way?

Thanks.

@jbagg
Copy link
Owner

jbagg commented May 22, 2023

I'm ok with changing the license on just QZeroConfNsdManager.java if Michael is. I've emailed him just in case he is not following this thread anymore.

@fatice8
Copy link

fatice8 commented May 22, 2023

I'm ok with changing the license on just QZeroConfNsdManager.java if Michael is. I've emailed him just in case he is not following this thread anymore.

Thanks for the update.

@jbagg
Copy link
Owner

jbagg commented May 22, 2023

QZeroConfNsdManager.java has been changed to a BSD license so include as necessary.

android-nds branch has been merged to master

@fatice8
Copy link

fatice8 commented May 23, 2023

QZeroConfNsdManager.java has been changed to a BSD license so include as necessary.

android-nds branch has been merged to master

Thanks for the update. I am now testing the merged code with client and core on different platforms.

@dominikfehr
Copy link

dominikfehr commented Jun 6, 2024

Problem seems to still exist on Android 15 (Pixel 7) for API levels 31, 33, 34

W qtMainLoopThrea: type=1400 audit(0.0:1636): avc: denied { ioctl } for path="socket:[215904]" dev="sockfs" ino=215904 ioctlcmd=0x8927 scontext=u:r:untrusted_app:s0:c204,c257,c512,c768 tcontext=u:r:untrusted_app:s0:c204,c257,c512,c768 tclass=tcp_socket permissive=0

Used latest master

@jbagg
Copy link
Owner

jbagg commented Jun 6, 2024

Do you experience zeroconf browsing failing in addition to this error?

@dominikfehr
Copy link

dominikfehr commented Jun 7, 2024

Do you experience zeroconf browsing failing in addition to this error?

Yes, the log flow is as follows:

  1. avc: denied { ioctl } warning appears
  2. QZeroConfNsdManager is created
  3. android.permission.ACCESS_NETWORK_STATE is confirmed to be allowed
  4. discoverServices() is called
  5. onDiscoveryStarted() is called
  6. silence

Test query is: zeroConfAdapter.startBrowser("_services._dns-sd._udp")

UPDATE:

No luck on Android 14 either with a Samsung Galaxy S21 FE. The SElinux error does not appear here tho.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants