Prevent console logging on iOS 11.3+ within WebSocket #18948

maicki · 2018-04-19T14:10:41Z

Fixes Xcode console output for web socket connections. Apple uses OSLog for logging within libnetworking starting 11.3+. The old way we hook into logging to prevent it will not work anymore.

Let's hook into __nwlog_pack that is exclusively used by libnetworking and prevent the logging in there.

pull-bot · 2018-04-19T14:12:41Z

	Warnings
⚠️	📋 Test Plan - This PR appears to be missing a Test Plan.
⚠️	📋 Release Notes - This PR appears to be missing Release Notes.

Generated by 🚫 dangerJS

lhecker · 2018-04-23T12:39:24Z

@maicki @hramos I tested this PR and can confirm that this solves #10027 in my case. 🙂
With this __nwlog_pack hook in place the nwlog_legacy_v hook could be removed though, right?

lhecker · 2018-04-23T12:44:22Z

Libraries/WebSocket/RCTReconnectingWebSocket.m

@@ -63,6 +84,9 @@ + (void)load
    rebind_symbols((struct rebinding[1]){
      {"nwlog_legacy_v", my_nwlog_legacy_v, (void *)&orig_nwlog_legacy_v}
    }, 1);
+    rebind_symbols((struct rebinding[1]){
+      {"__nwlog_pack", my__nwlog_pack, (void *)&orig__nwlog_pack}},
+                   1);


I believe you need to fix the indentation here.

lhecker · 2018-04-23T12:47:56Z

Libraries/WebSocket/RCTReconnectingWebSocket.m

@@ -19,6 +19,27 @@

 #if RCT_DEV // Only supported in dev mode

+// From https://github.com/apple/swift/blob/ad40c770bfe372f879b530443a3d94761fe258a6/stdlib/public/SDK/os/os_log.m


It might be necessary to add a __IPHONE_OS_VERSION_MAX_ALLOWED >= 100300 check here since the entire API around this (e.g. _os_log_pack_size etc.) has only been added in iOS 10.3, but React supports up to iOS 8.

lhecker · 2018-04-23T12:54:53Z

Libraries/WebSocket/RCTReconnectingWebSocket.m

+static void my__nwlog_pack(os_log_pack_t pack, os_log_type_t logType)
+{
+  if (logType == OS_LOG_TYPE_ERROR) {
+    if (strstr(pack->olp_format, "%{public}s %s Connection has no connected handler") == NULL) {


Since you're comparing the entire string from start to end anyways I suggest changing this check to:

strcmp(pack->olp_format, "%{public}s %s Connection has no connected handler") != 0

I change it to just compare to Connection has no connected handler, the same as in my_nwlog_legacy_v

lhecker · 2018-04-23T12:55:00Z

Libraries/WebSocket/RCTReconnectingWebSocket.m

@@ -46,7 +67,7 @@ static void my_os_log_error_impl(void *dso, os_log_t log, os_log_type_t type, co
  }
 }

-#endif /* __IPHONE_11_0 */
+#endif /* RCT_DEV */


This change is incorrect. This endif belongs to its previous __IPHONE_11_0 check

Correct, changed back.

lhecker

Here's my modified version with all required changes I can think of: https://gist.github.com/lhecker/19592f092b2f0611f264733051a168c1
(No attribution needed.)

maicki · 2018-04-23T14:19:01Z

@lhecker Just addressed the comments, thanks for reviewing.

I would suggest we keep the nwlog_legacy_v hook to have at least for the near term some backwards compatibility.

lhecker · 2018-04-23T14:32:49Z

Libraries/WebSocket/RCTReconnectingWebSocket.m

-      {"__nwlog_pack", my__nwlog_pack, (void *)&orig__nwlog_pack}},
-                   1);
+      {"__nwlog_pack", my__nwlog_pack, (void *)&orig__nwlog_pack}
+


Sadly there's still a leftover newline here. 😕

lhecker · 2018-04-23T14:34:12Z

@maicki Yeah you're right about the nwlog_legacy_v hook.
P.S.: Sorry that I didn't bundle those 4 reviews into one before. I forgot about this "Start Review" functionality Github provides.

vovkasm · 2018-04-27T08:59:58Z

Oh no... please stop insert hard to debug code here... instead, please remove fishhook at all. See wip patch #17617

facebook-github-bot

@mmmulani has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mmmulani · 2018-05-09T21:26:48Z

thanks for this!

overall this change looks good to me, the only thing I might fix up is combining the if's to a single one.

I'm also a fan of this approach as it's only in dev mode.

vovkasm · 2018-05-10T05:38:01Z

It introduces forward incompatibilty, what if os_log_pack_s change in new version of ios?
Moreover it will be security issue... field olp_format compared with strstr, but if it change, what address of memory will be supplied to strstr?
It increases complexity and future support burden...
It hides users errors, not only RN's

One can say "it's safe because it only in dev mode", but I will say:

Each new dev mode deviation from production increases the need to run/test/debug production mode builds, so developer will create/enable additional level of debug itself... then he will forgot to disable some of them...
Dev mode can be too easy be enabled in production software... I need to forgot about one single switch...

mmmulani · 2018-05-10T14:41:56Z

we want dev mode to be a good experience for people though. Of course we're deviating from prod because we want to make it easy to develop. But I don't think this will cause much unexpected changes for people when comparing debug to prod.

Dev mode can be too easy be enabled in production software... I need to forgot about one single switch...

Again, I don't think this is a good reason for us to not make the development experience good.

Regarding security, the attack vector would be another native module that you import. At which point, there's not much hope from defending against it.

mmmulani · 2018-05-10T14:42:45Z

separately this got reverted for a build error, I'll try to reland

Summary: Fixes Xcode console output for web socket connections. Apple uses OSLog for logging within libnetworking starting 11.3+. The old way we hook into logging to prevent it will not work anymore. Let's hook into `__nwlog_pack` that is exclusively used by libnetworking and prevent the logging in there. Closes facebook#18948 Reviewed By: fkgozali Differential Revision: D7940969 Pulled By: mmmulani fbshipit-source-id: a61beea34377044bfad7e3c446b2ec1138d6d1f5

Prevent console logging on iOS 11.3+ from WebSocket

d055f28

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 19, 2018

react-native-bot added Missing Test Plan This PR appears to be missing a test plan. Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. Platform: iOS iOS applications. labels Apr 19, 2018

lhecker reviewed Apr 23, 2018

View reviewed changes

lhecker suggested changes Apr 23, 2018

View reviewed changes

Address comments

03e3b18

lhecker approved these changes Apr 23, 2018

View reviewed changes

facebook-github-bot reviewed May 9, 2018

View reviewed changes

facebook-github-bot closed this in d11fdcf May 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent console logging on iOS 11.3+ within WebSocket #18948

Prevent console logging on iOS 11.3+ within WebSocket #18948

maicki commented Apr 19, 2018 •

edited

Loading

pull-bot commented Apr 19, 2018 •

edited

Loading

lhecker commented Apr 23, 2018

lhecker Apr 23, 2018 •

edited

Loading

maicki Apr 23, 2018

lhecker Apr 23, 2018

maicki Apr 23, 2018

lhecker Apr 23, 2018

maicki Apr 23, 2018

lhecker Apr 23, 2018

maicki Apr 23, 2018

lhecker left a comment

maicki commented Apr 23, 2018

lhecker Apr 23, 2018

maicki Apr 23, 2018

lhecker commented Apr 23, 2018

vovkasm commented Apr 27, 2018

facebook-github-bot left a comment

mmmulani commented May 9, 2018

vovkasm commented May 10, 2018

mmmulani commented May 10, 2018

mmmulani commented May 10, 2018

		@@ -19,6 +19,27 @@

		#if RCT_DEV // Only supported in dev mode

		// From https://github.com/apple/swift/blob/ad40c770bfe372f879b530443a3d94761fe258a6/stdlib/public/SDK/os/os_log.m

Prevent console logging on iOS 11.3+ within WebSocket #18948

Prevent console logging on iOS 11.3+ within WebSocket #18948

Conversation

maicki commented Apr 19, 2018 • edited Loading

pull-bot commented Apr 19, 2018 • edited Loading

lhecker commented Apr 23, 2018

lhecker Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker left a comment

Choose a reason for hiding this comment

maicki commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker commented Apr 23, 2018

vovkasm commented Apr 27, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

mmmulani commented May 9, 2018

vovkasm commented May 10, 2018

mmmulani commented May 10, 2018

mmmulani commented May 10, 2018

maicki commented Apr 19, 2018 •

edited

Loading

pull-bot commented Apr 19, 2018 •

edited

Loading

lhecker Apr 23, 2018 •

edited

Loading