1016744 - kvm_open()'s corefile argument should be /dev/null for non-root on DragonFly and FreeBSD

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Jan Beich]

Attachment: fix

To make kvm(3) retrieve data over sysctl(3) NetBSD and OpenBSD use KVM_NO_FILES flag while DragonFly and FreeBSD use /dev/null for corefile. Otherwise kvm(3) tries to query live kernel via /dev/mem and this would fail for non-root user. Accessing struct kinfo_proc members past failed kvm_open() would normally lead to a crash but it seems |class NamedProcessIterator| is unused on m-c.

upstream version uses sysctl(3) directly:
https://chromium.googlesource.com/chromium/src/base/+/HEAD/process/process_iterator_freebsd.cc
https://chromium.googlesource.com/chromium/src/base/+/HEAD/process/process_iterator_openbsd.cc

Comment 1

[Jan Beich]

Attachment: approximate test

$ cc test.c -std=c99 -lkvm
$ ./a.out
Segmentation fault (core dumped)

Comment 2

[Jan Beich]

Attachment: remove NamedProcessIterator

Just an idea. process_util_bsd.cc becomes mostly POSIX code. May need more header cleanup on Windows and Try run.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8429710 [details] [diff] [review]
fix

Hm, I'm not really a good reviewer for this.

Benjamin, would you like to look this over? Or know someone who would?

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8429725 [details] [diff] [review]
remove NamedProcessIterator

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

If we don't have any plans to use NamedProcessIterator, I don't see why this wouldn't work.  Let's see if it builds: https://tbpl.mozilla.org/?tree=Try&rev=94c23b388ecf

Comment 5

[Jan Beich]

Attachment: remove followup

Missed some -lkvm for BSDs in ipc/chromium/chromium-config.mozbuild. The patch should be squashed before landing.

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8429710 [details] [diff] [review]
fix

This is apparently dead code for us, and the later patch to delete the entire class passed try, so I think we should do that instead.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Remove NamedProcessIterator, squashed.

I've squashed the removal patches together, as requested in comment #5.  r=me, rs=bsmedberg via IRC.

Trying: https://tbpl.mozilla.org/?tree=Try&rev=0ce8872a7350 (but it passed before and the incremental change is NPOTB, so I don't expect it to fail).

Comment 8

[Jan Beich]

Comment on attachment 8434611 [details] [diff] [review]
Remove NamedProcessIterator, squashed.

Those red on OS X are probably an infra issue. Also, still builds fine on FreeBSD.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6bdcac89d0d

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e6bdcac89d0d