Closed Bug 1016744 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: IPC, defect)

All
FreeBSD
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jbeich, Assigned: jbeich)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch fix (obsolete) — Splinter Review
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
Attachment #8429710 - Flags: review?(bent.mozilla)
Attached file approximate test (obsolete) —
$ cc test.c -std=c99 -lkvm
$ ./a.out
Segmentation fault (core dumped)
Attachment #8429711 - Attachment mime type: text/x-csrc → text/plain
Attached patch remove NamedProcessIterator (obsolete) — Splinter Review
Just an idea. process_util_bsd.cc becomes mostly POSIX code. May need more header cleanup on Windows and Try run.
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?
Attachment #8429710 - Flags: review?(bent.mozilla) → review?(benjamin)
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
Attached patch remove followup (obsolete) — Splinter Review
Missed some -lkvm for BSDs in ipc/chromium/chromium-config.mozbuild. The patch should be squashed before landing.
Attachment #8429710 - Flags: review?(benjamin) → review?(jld)
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.
Attachment #8429710 - Attachment is obsolete: true
Attachment #8429710 - Flags: review?(jld)
Attachment #8429711 - Attachment is obsolete: true
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).
Attachment #8429725 - Attachment is obsolete: true
Attachment #8431236 - Attachment is obsolete: true
Attachment #8434611 - Flags: review+
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.
Attachment #8434611 - Flags: feedback+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e6bdcac89d0d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Duplicate of this bug: 1024641
You need to log in before you can comment on or make changes to this bug.