Support abstract socket address

RESOLVED FIXED in 4.20

Status

enhancement
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

other
4.20
Unspecified
Android
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

a year ago
This is required for Remote debugging using Android USB connection.
Assignee

Updated

a year ago
Blocks: 1462019
Assignee

Updated

a year ago
Assignee: nobody → m_kato
Assignee

Updated

a year ago
Summary: Support abstract socket → Support abstract socket address
Assignee

Comment 2

a year ago
Comment on attachment 8983668 [details] [diff] [review]
Support abstract socket address on Linux and Android

adb cannot open UNIX domain socket using path name on Android O that is target sdk >= 26 .  Android O can still use abstract socket address and Chrome for Android uses it.

Actually, NSPR doesn't support abstract socket address, so I need this support.
Attachment #8983668 - Flags: review?(mh+mozilla)
Assignee

Comment 3

a year ago
Comment on attachment 8983668 [details] [diff] [review]
Support abstract socket address on Linux and Android

sorry, I need small clean up, so this is canceled
Attachment #8983668 - Flags: review?(mh+mozilla)
Assignee

Comment 4

a year ago
Attachment #8983668 - Attachment is obsolete: true
Assignee

Comment 5

a year ago
Attachment #8983688 - Attachment is obsolete: true
Assignee

Comment 6

a year ago
Comment on attachment 8983690 [details] [diff] [review]
Support abstract socket address on Linux and Android

adb cannot open UNIX domain socket using path name on Android O that is target sdk >= 26 .  Android O can still use abstract socket address and Chrome for Android uses it.

Actually, NSPR doesn't support abstract socket address, so I need this support.
Attachment #8983690 - Flags: review?(mh+mozilla)
Comment on attachment 8983690 [details] [diff] [review]
Support abstract socket address on Linux and Android

Review of attachment 8983690 [details] [diff] [review]:
-----------------------------------------------------------------

::: pr/src/misc/prnetdb.c
@@ +1374,5 @@
> +#if defined(LINUX)
> +        if (addr->local.path[0] == 0)
> +            /* abstract socket address is supported on Linux only */
> +            addrsize = strlen(addr->local.path + 1) +
> +                       offsetof(struct sockaddr_un, sun_path) + 1;

This seems unnecessary. The non-abstract socket address case doesn't care about the strlen of the path, and just uses the size of the whole thing. Why should the abstract socket address case care?

::: pr/tests/abstract.c
@@ +56,5 @@
> +  PRFileDesc* socket;
> +  PRFileDesc* acceptSocket;
> +  PRThread* thread;
> +  PRNetAddr addr;
> +  PRUint8 buf[1] = { 'A' };

maybe make this a little larger? (not that it matters much)

@@ +60,5 @@
> +  PRUint8 buf[1] = { 'A' };
> +
> +  addr.local.family = PR_AF_LOCAL;
> +  addr.local.path[0] = '\0';
> +  strcpy(addr.local.path + 1, abstractSocketName);

you can set abstractSocketName to "\0testsocket" and just memcpy(addr.local.path, abstractSocketName, sizeof(abstractSocketName))
Attachment #8983690 - Flags: review?(mh+mozilla)
Assignee

Comment 8

a year ago
(In reply to Mike Hommey [:glandium] from comment #7)
> Comment on attachment 8983690 [details] [diff] [review]
> Support abstract socket address on Linux and Android
> 
> Review of attachment 8983690 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: pr/src/misc/prnetdb.c
> @@ +1374,5 @@
> > +#if defined(LINUX)
> > +        if (addr->local.path[0] == 0)
> > +            /* abstract socket address is supported on Linux only */
> > +            addrsize = strlen(addr->local.path + 1) +
> > +                       offsetof(struct sockaddr_un, sun_path) + 1;
> 
> This seems unnecessary. The non-abstract socket address case doesn't care
> about the strlen of the path, and just uses the size of the whole thing. Why
> should the abstract socket address case care?

name (sun_path) of abstract socket address don't check null-terminate, so it is necessary to calculate size for name.
If this code is nothing, socket name has a lot of garbage character, then PR_Connect on client will be failed.
(In reply to Makoto Kato [:m_kato] from comment #8)
> name (sun_path) of abstract socket address don't check null-terminate

Where did you get that from? The kernel source surely shows it does, and that has been true since at least the first version in the kernel repo history, 13 years ago.

https://github.com/torvalds/linux/blob/master/net/unix/af_unix.c#L240-L242
Assignee

Comment 10

a year ago
(In reply to Mike Hommey [:glandium] from comment #9)
> (In reply to Makoto Kato [:m_kato] from comment #8)
> > name (sun_path) of abstract socket address don't check null-terminate
> 
> Where did you get that from? The kernel source surely shows it does, and
> that has been true since at least the first version in the kernel repo
> history, 13 years ago.
> 
> https://github.com/torvalds/linux/blob/master/net/unix/af_unix.c#L240-L242

sun_path[0] is \0 on abstract socket address.
err, facepalm, I read the condition backwards. Then you shouldn't let strlen possibly go past the end of the buffer if the abstract socket address is not null terminated within the size of the buffer.
Assignee

Updated

11 months ago
Attachment #8985663 - Flags: review?(mh+mozilla)
Comment on attachment 8985663 [details] [diff] [review]
Support abstract socket address on Linux and Android

Review of attachment 8985663 [details] [diff] [review]:
-----------------------------------------------------------------

::: pr/src/misc/prnetdb.c
@@ +1374,5 @@
> +#if defined(LINUX)
> +        if (addr->local.path[0] == 0)
> +            /* abstract socket address is supported on Linux only */
> +            addrsize = strnlen(addr->local.path + 1,
> +                               sizeof(addr->local.path) - 1) +

you don't need to -1 here.
Attachment #8985663 - Flags: review?(mh+mozilla) → review+
Assignee

Comment 15

11 months ago
I have no permission for nspr repository.
Keywords: checkin-needed
Mike, please land this. Sheriffs don't have write access to NSPR.
Flags: needinfo?(mh+mozilla)
https://hg.mozilla.org/projects/nspr/rev/607196c7ef66142c333717ee8a819c13587285d0
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Flags: needinfo?(mh+mozilla)
Resolution: --- → FIXED
Assignee

Updated

11 months ago
Keywords: checkin-needed
Assignee

Comment 18

10 months ago
Kai, although I would like to uplift this fix to mozilla-central (for Gecko's remote debugger support), is there any process to uplift?

Some fix such as bug 1358466 is landed to mozilla-central directly, but most landing is rtm version or beta version of nspr only.
Flags: needinfo?(kaie)

Comment 19

10 months ago
Yes, there is a process. Because of Linux distribution, which may use NSPR and NSS as separate system libraries, it's necessary that for all changes that Firefox requires, that we publish new releases of those libraries.

I assume you target Firefox 63. We can plan for a new NSPR 4.20 release that can be released in time for FF63.

I'll uplift an NSPR 4.20 beta release to m-c today.
Flags: needinfo?(kaie)
FYI, uplifting is tracked in bug 1477680

Updated

9 months ago
Target Milestone: --- → 4.20

Updated

9 months ago
Depends on: 1487579
You need to log in before you can comment on or make changes to this bug.