jbagg / QtZeroConf

Qt wrapper class for ZeroConf libraries across various platforms.
Other
69 stars 51 forks source link

Android API Level 30 issues #46

Open mzanetti opened 2 years ago

mzanetti commented 2 years ago

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 commented 2 years ago

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

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

mzanetti commented 2 years ago

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

According to this report https://github.com/arvidn/libtorrent/issues/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Got it working! Here's a preview: https://github.com/mzanetti/QtZeroConf/pull/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

mzanetti commented 2 years ago

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 https://github.com/jbagg/QtZeroConf/pull/48

jbagg commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Qt = 5.15.2

Do you have

QT+= gui widgets

in your .pro file?  I've seen this error before (not in QtZeroConf) if you don't.

This link QZeroConfNsdManager.java -> ../../../QZeroConfNsdManager.java

is in QtZeroConf/example/android/src/

Jon

On 2022-02-23 3:57 p.m., dwebb72 wrote:

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

— Reply to this email directly, view it on GitHub https://github.com/jbagg/QtZeroConf/issues/46#issuecomment-1049209613, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADFEL4ZIL7R77NNQZAWVJALU4VC5FANCNFSM5DW7J5AA. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you commented.Message ID: @.***>

dwebb72 commented 2 years ago

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 commented 2 years ago

After changing the .pro, I assume you did a

make clean

rm `find . -name "*.o"`

rm `find . -name "moc*"`

qmake

?

On 2022-02-23 4:38 p.m., dwebb72 wrote:

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()'

— Reply to this email directly, view it on GitHub https://github.com/jbagg/QtZeroConf/issues/46#issuecomment-1049239658, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADFEL47EPAJCHQIHB7GPAH3U4VHW7ANCNFSM5DW7J5AA. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you commented.Message ID: @.***>

dwebb72 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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

fatice8 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

android-nds branch has been merged to master

fatice8 commented 1 year ago

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 commented 2 months ago

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 commented 2 months ago

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

dominikfehr commented 2 months ago

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.