Details

Reviewers

lwhsu
lichray_gmail.com
ygy

Commits

rP559591: chinese/fcitx-rime: Upgrade to use RimeData-based schemas

Summary

brise has been deprecated since 2019.
This revision replaces the usage of chinese/brise in fcitx-rime withRimeData-based schemas.

RimeData-based schemas live in ${LOCALBASE}/share/rime-data.

https://bugs.freebsd.org/251893

Diff Detail

Repository

rP FreeBSD ports repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

khng created this revision.Dec 16 2020, 11:17 AM

Herald added a subscriber: mat. · View Herald TranscriptDec 16 2020, 11:17 AM

khng requested review of this revision.Dec 16 2020, 11:17 AM

Harbormaster completed remote builds in B35434: Diff 80750.Dec 16 2020, 11:17 AM

Deprecate chinese/brise in favor of chinese/rime-data

Updating D27630: Upgrade fcitx-rime to use rime-data.

Add chinese/rime-cantonese.

Harbormaster completed remote builds in B35435: Diff 80751.Dec 16 2020, 11:22 AM

I can add chinese/rime-cantonese and chinese/rime-data soon, but please file a ticket in bugzilla to ask approval from the maintainer of chinese/fcitx-rime regarding the changes to it. BTW, we need to bump its PORTREVISION.

In short, let's do this in two steps, adding the dependency first, then modify chinese/fcitx-rime to use them.

Oh, and we probably want to mark chinese/brise as DEPRECATED first, but if there is no ports depend on it, of course we can do it faster. I suggest making this as the third step.

khng retitled this revision from Upgrade fcitx-rime to use rime-data. Add chinese/rime-cantonese. to Upgrade fcitx-rime to use rime-data.Add chinese/rime-cantonese..Dec 16 2020, 11:30 AM

khng added a reviewer: lichray_gmail.com.

lwhsu added inline comments.Dec 16 2020, 11:33 AM

chinese/fcitx-rime/Makefile
6 ↗	(On Diff #80751)	PORTREVISION bump needed.

lwhsu added inline comments.Dec 16 2020, 11:38 AM

chinese/rime-data/pkg-descr
3 ↗	(On Diff #80751)	It seems we retrieve data from multiple rime repositories, why do we use rime-bopomofo here? BTW, does it make more sense to create fine-grained ports like data-bopomofo and so on?

In D27630#617395, @lwhsu wrote:

Oh, and we probably want to mark chinese/brise as DEPRECATED first, but if there is no ports depend on it, of course we can do it faster. I suggest making this as the third step.

And since there is a maintainer, we also want to inform them, or wait a little longer after the maintainer timeout. Yes, this needs a ticket in bugzilla.

Bump chinese/fcitx-rime PORTREVISION
Fix chinese/rime-data pkg-descr

Harbormaster completed remote builds in B35436: Diff 80752.Dec 16 2020, 12:02 PM

khng marked 2 inline comments as done.Dec 16 2020, 12:02 PM

khng added inline comments.Dec 16 2020, 12:04 PM

chinese/rime-data/pkg-descr
3 ↗	(On Diff #80751)	My original attempt was to create a fine-grained ports such as rime-bopomofo. However the attempt doesn't work well so far as including all these fine-grained data files in a rime-data metaport seems to confuse the infrastructure on dependency resolving. Generally to make sure the user experience isn't affected a basic set of schemas has to be included.

Revert "Deprecate chinese/brise in favor of chinese/rime-data"

Harbormaster completed remote builds in B35438: Diff 80765.Dec 16 2020, 3:32 PM

diizzy added a subscriber: diizzy.Dec 16 2020, 4:48 PM

diizzy added inline comments.

chinese/fcitx-rime/Makefile
5 ↗	(On Diff #80765)	Should be distversion https://www.freebsd.org/doc/en/books/porters-handbook/book.html --> Table 5.2. Package Naming Examples
8 ↗	(On Diff #80765)	Redirects to https
19 ↗	(On Diff #80765)	Looks like a copy paste issue, should be BUILD_DEPENDS= bash:shells/bash \

Fix chinese/fcitx-rime MASTER_SITES
Address diizzy's comments on chinese/fcitx-rime

Harbormaster completed remote builds in B35441: Diff 80772.Dec 16 2020, 5:44 PM

More ports are split from chinese/rime-data

New port: chinese/rime-prelude
New port: chinese/rime-essay
New port: chinese/rime-cantonese
New port: chinese/rime-terra-pinyin
New port: chinese/rime-luna-pinyin
New port: chinese/rime-bopomofo
New port: chinese/rime-cangjie
New port: chinese/rime-stroke
New port: chinese/rime-data
Update fcitx-rime to use RimeData instead of brise
chinese/fcitx-rime depends on chinese/rime-data
chinese/fcitx-rime: Address diizzy's comments on Makefile

Harbormaster completed remote builds in B35444: Diff 80777.Dec 16 2020, 6:41 PM

khng marked 3 inline comments as done.Dec 16 2020, 6:42 PM

Rebased to D27652:

Update fcitx-rime to use RimeData instead of brise
chinese/fcitx-rime depends on chinese/rime-data
chinese/fcitx-rime: Address diizzy's comments on Makefile

khng added a parent revision: D27652: New port: chinese/rime-data.Dec 17 2020, 7:48 AM

Harbormaster completed remote builds in B35468: Diff 80813.Dec 17 2020, 7:48 AM

Fix Sponsored by line

Harbormaster completed remote builds in B35471: Diff 80816.Dec 17 2020, 7:56 AM

Rebased

Harbormaster completed remote builds in B35473: Diff 80818.Dec 17 2020, 8:08 AM

khng retitled this revision from Upgrade fcitx-rime to use rime-data.Add chinese/rime-cantonese. to Upgrade chinese/fcitx-rime to use chinese/rime-data..Dec 17 2020, 9:29 AM

khng edited the summary of this revision. (Show Details)

LGTM, wait for maintainer.

This revision is now accepted and ready to land.Dec 17 2020, 9:31 AM

Rebased

This revision now requires review to proceed.Dec 17 2020, 10:17 AM

Harbormaster completed remote builds in B35475: Diff 80821.Dec 17 2020, 10:17 AM

lwhsu edited the summary of this revision. (Show Details)Dec 17 2020, 11:59 AM

Rebased.

chinese/rime-*: Use CP instead of RLN for data files from localbase
Update chinese/fcitx-rime to use RimeData instead

Harbormaster completed remote builds in B35595: Diff 81034.Dec 22 2020, 6:57 AM

Rebased.

Harbormaster completed remote builds in B35597: Diff 81036.Dec 22 2020, 7:05 AM

khng edited parent revisions, added: D27716: chinese/rime-*: Use CP instead of RLN for data files from localbase; removed: D27652: New port: chinese/rime-data.Dec 22 2020, 7:06 AM

Rebased

Harbormaster completed remote builds in B35599: Diff 81038.Dec 22 2020, 7:43 AM

Rebased.

Harbormaster completed remote builds in B35601: Diff 81040.Dec 22 2020, 7:47 AM

Rebased.

Update fcitx-rime to use RimeData instead of brise
chinese/fcitx-rime depends on chinese/rime-data
chinese/fcitx-rime: Address diizzy's comments on Makefile
chinese/fcitx-rime: Depends on individual essential schema ports

Harbormaster completed remote builds in B35646: Diff 81092.Dec 23 2020, 4:05 PM

khng retitled this revision from Upgrade chinese/fcitx-rime to use chinese/rime-data. to Upgrade chinese/fcitx-rime to use %%LOCALBASE%%/share/rime-data schemas..Dec 23 2020, 5:02 PM

khng edited the summary of this revision. (Show Details)

khng retitled this revision from Upgrade chinese/fcitx-rime to use %%LOCALBASE%%/share/rime-data schemas. to Upgrade chinese/fcitx-rime to use RimeData-based schemas..Dec 23 2020, 5:20 PM

khng edited the summary of this revision. (Show Details)

LGTM.

Since the maintainer has been reset and diff updated, we can get this committed soon. ;)

This revision is now accepted and ready to land.Dec 28 2020, 10:42 AM

Closed by commit rP559591: chinese/fcitx-rime: Upgrade to use RimeData-based schemas (authored by ygy). · Explain WhyDec 29 2020, 4:02 PM

This revision was automatically updated to reflect the committed changes.

ygy added a commit: rP559591: chinese/fcitx-rime: Upgrade to use RimeData-based schemas.

Diff 81304

View Options

head/chinese/fcitx-rime/Makefile

Show First 20 Lines • Show All 97 Lines • ▼ Show 20 Lines
#endif		#endif

int old_mlock = 0;		int old_mlock = 0;
SYSCTL_INT(_vm, OID_AUTO, old_mlock, CTLFLAG_RWTUN, &old_mlock, 0,		SYSCTL_INT(_vm, OID_AUTO, old_mlock, CTLFLAG_RWTUN, &old_mlock, 0,
"Do not apply RLIMIT_MEMLOCK on mlockall");		"Do not apply RLIMIT_MEMLOCK on mlockall");
static int mincore_mapped = 1;		static int mincore_mapped = 1;
SYSCTL_INT(_vm, OID_AUTO, mincore_mapped, CTLFLAG_RWTUN, &mincore_mapped, 0,		SYSCTL_INT(_vm, OID_AUTO, mincore_mapped, CTLFLAG_RWTUN, &mincore_mapped, 0,
"mincore reports mappings, not residency");		"mincore reports mappings, not residency");
		static int imply_prot_max = 0;
		SYSCTL_INT(_vm, OID_AUTO, imply_prot_max, CTLFLAG_RWTUN, &imply_prot_max, 0,
		emasteUnsubmitted Done Inline Actions `s/mincore_mapped/imply_prot_max/` emaste: `s/mincore_mapped/imply_prot_max/`
		"Imply maximum page permissions in mmap() when none are specified.");
		markjUnsubmitted Done Inline Actions sysctl description strings usually don't contain a period. markj: sysctl description strings usually don't contain a period.

#ifdef MAP_32BIT		#ifdef MAP_32BIT
#define MAP_32BIT_MAX_ADDR ((vm_offset_t)1 << 31)		#define MAP_32BIT_MAX_ADDR ((vm_offset_t)1 << 31)
#endif		#endif

#ifndef _SYS_SYSPROTO_H_		#ifndef _SYS_SYSPROTO_H_
struct sbrk_args {		struct sbrk_args {
int incr;		int incr;
▲ Show 20 Lines • Show All 68 Lines • ▼ Show 20 Lines
kern_mmap(struct thread *td, uintptr_t addr0, size_t size, int prot, int flags,		kern_mmap(struct thread *td, uintptr_t addr0, size_t size, int prot, int flags,
int fd, off_t pos)		int fd, off_t pos)
{		{
struct vmspace *vms;		struct vmspace *vms;
struct file *fp;		struct file *fp;
vm_offset_t addr;		vm_offset_t addr;
vm_size_t pageoff;		vm_size_t pageoff;
vm_prot_t cap_maxprot;		vm_prot_t cap_maxprot;
int align, error;		int align, error, max_prot;
cap_rights_t rights;		cap_rights_t rights;

		if ((prot & ~(_PROT_ALL \| PROT_MAX(_PROT_ALL))) != 0)
		return (EINVAL);
		emasteUnsubmitted Done Inline Actions This has the effect of masking invalid flags (e.g. `PROT_READ \| PROT_WRITE \| 0x100000` or `0xffff` in tests/sys/vm/mmap_test.c). emaste: This has the effect of masking invalid flags (e.g. `PROT_READ \| PROT_WRITE \| 0x100000` or…
		max_prot = PROT_MAX_EXTRACT(prot);
		prot = PROT_EXTRACT(prot);
		markjUnsubmitted Done Inline Actions Don't we also need to verify that prot is a subset of max_prot? markj: Don't we also need to verify that prot is a subset of max_prot?
		/*
		* Always honor PROT_MAX if set. If not, default to all
		* permissions unless we're implying maximum permissions.
		*
		* XXX: should be tunable per process and ABI.
		*/
		if (max_prot == 0)
		max_prot = (imply_prot_max && prot != PROT_NONE) ?
		prot : _PROT_ALL;
		kibUnsubmitted Done Inline Actions I would write this as ` max_prot = imply_prot_max ? prot : _PROT_ALL ` kib: I would write this as ` max_prot = imply_prot_max ? prot : _PROT_ALL `

vms = td->td_proc->p_vmspace;		vms = td->td_proc->p_vmspace;
fp = NULL;		fp = NULL;
AUDIT_ARG_FD(fd);		AUDIT_ARG_FD(fd);
addr = addr0;		addr = addr0;

/*		/*
* Ignore old flags that used to be defined but did not do anything.		* Ignore old flags that used to be defined but did not do anything.
*/		*/
▲ Show 20 Lines • Show All 126 Lines • ▼ Show 20 Lines	error = vm_mmap_object(&vms->vm_map, &addr, size, VM_PROT_NONE,
VM_PROT_NONE, flags, NULL, pos, FALSE, td);		VM_PROT_NONE, flags, NULL, pos, FALSE, td);
} else if ((flags & MAP_ANON) != 0) {		} else if ((flags & MAP_ANON) != 0) {
/*		/*
* Mapping blank space is trivial.		* Mapping blank space is trivial.
*		*
* This relies on VM_PROT_* matching PROT_*.		* This relies on VM_PROT_* matching PROT_*.
*/		*/
error = vm_mmap_object(&vms->vm_map, &addr, size, prot,		error = vm_mmap_object(&vms->vm_map, &addr, size, prot,
VM_PROT_ALL, flags, NULL, pos, FALSE, td);		max_prot, flags, NULL, pos, FALSE, td);
} else {		} else {
/*		/*
* Mapping file, get fp for validation and don't let the		* Mapping file, get fp for validation and don't let the
* descriptor disappear on us if we block. Check capability		* descriptor disappear on us if we block. Check capability
* rights, but also return the maximum rights to be combined		* rights, but also return the maximum rights to be combined
* with maxprot later.		* with maxprot later.
*/		*/
cap_rights_init(&rights, CAP_MMAP);		cap_rights_init(&rights, CAP_MMAP);
if (prot & PROT_READ)		if (prot & PROT_READ)
cap_rights_set(&rights, CAP_MMAP_R);		cap_rights_set(&rights, CAP_MMAP_R);
if ((flags & MAP_SHARED) != 0) {		if ((flags & MAP_SHARED) != 0) {
if (prot & PROT_WRITE)		if (prot & PROT_WRITE)
cap_rights_set(&rights, CAP_MMAP_W);		cap_rights_set(&rights, CAP_MMAP_W);
}		}
if (prot & PROT_EXEC)		if (prot & PROT_EXEC)
cap_rights_set(&rights, CAP_MMAP_X);		cap_rights_set(&rights, CAP_MMAP_X);
error = fget_mmap(td, fd, &rights, &cap_maxprot, &fp);		error = fget_mmap(td, fd, &rights, &cap_maxprot, &fp);
if (error != 0)		if (error != 0)
goto done;		goto done;
if ((flags & (MAP_SHARED \| MAP_PRIVATE)) == 0 &&		if ((flags & (MAP_SHARED \| MAP_PRIVATE)) == 0 &&
		markjUnsubmitted Not Done Inline Actions Shouldn't it be `(max_prot & cap_maxprot) == max_prot`? Otherwise userland can trigger this assertion by requesting a `PROT_MAX(PROT_READ) \| PROT_READ` mapping using a descriptor with read and write rights. markj: Shouldn't it be `(max_prot & cap_maxprot) == max_prot`? Otherwise userland can trigger this…
		brooksAuthorUnsubmitted Done Inline Actions My belief was that this can't happen because of the way fget_mmap is implemented, but rereading, it looks like that's only true when CAPABILITIES is defined (in which case cap_maxprot == prot so this is safe because prot must be a subset of max_prot per the check above). In the non-CAPABILITIES case cap_maxprot is VM_PROT_ALL so this will fail. Dropping the assertion may be the best fix. brooks: My belief was that this can't happen because of the way fget_mmap is implemented, but rereading…
		markjUnsubmitted Done Inline Actions Sorry, I don't see why that assertion worked even with CAPABILITIES defined. fget_mmap() returns the maximum protections permitted by capabilities on the descriptor. markj: Sorry, I don't see why that assertion worked even with CAPABILITIES defined. fget_mmap()…
		brooksAuthorUnsubmitted Done Inline Actions On further reading, you're correct. brooks: On further reading, you're correct.
td->td_proc->p_osrel >= P_OSREL_MAP_FSTRICT) {		td->td_proc->p_osrel >= P_OSREL_MAP_FSTRICT) {
error = EINVAL;		error = EINVAL;
goto done;		goto done;
}		}

/* This relies on VM_PROT_* matching PROT_. /		/* This relies on VM_PROT_* matching PROT_. /
error = fo_mmap(fp, &vms->vm_map, &addr, size, prot,		error = fo_mmap(fp, &vms->vm_map, &addr, size, prot,
cap_maxprot, flags, pos, td);		max_prot & cap_maxprot, flags, pos, td);
		kibUnsubmitted Not Done Inline Actions Should we deny the request if `(max_prot & cap_maxprot) != max_prot`, so that user knows that the call was not really satisfied ? kib: Should we deny the request if `(max_prot & cap_maxprot) != max_prot`, so that user knows that…
		brooksAuthorUnsubmitted Done Inline Actions I'm leaning toward not. I think we'd only want to error when we'd explicitly passed a PROT_MAX(FOO) expression so we'd have to add state for that since we don't want to fail when max_prot defaults to _PROT_ALL or when it's implied. We'd also just add yet another inexplicable EINVAL for a rare case. brooks: I'm leaning toward not. I think we'd only want to error when we'd explicitly passed a PROT_MAX…
		brooksAuthorUnsubmitted Done Inline Actions I've added a KASSERT of this because I'm convinced that with the `(prot & max_prot) == prot` check above that this will always be safe (cap_maxprot ends up set to something derived directly from prot). brooks: I've added a KASSERT of this because I'm convinced that with the `(prot & max_prot) == prot`…
}		}

if (error == 0)		if (error == 0)
td->td_retval[0] = (register_t) (addr + pageoff);		td->td_retval[0] = (register_t) (addr + pageoff);
done:		done:
if (fp)		if (fp)
fdrop(fp, td);		fdrop(fp, td);

▲ Show 20 Lines • Show All 214 Lines • ▼ Show 20 Lines	sys_mprotect(struct thread td, struct mprotect_args uap)
return (kern_mprotect(td, (uintptr_t)uap->addr, uap->len, uap->prot));		return (kern_mprotect(td, (uintptr_t)uap->addr, uap->len, uap->prot));
}		}

int		int
kern_mprotect(struct thread *td, uintptr_t addr0, size_t size, int prot)		kern_mprotect(struct thread *td, uintptr_t addr0, size_t size, int prot)
{		{
vm_offset_t addr;		vm_offset_t addr;
vm_size_t pageoff;		vm_size_t pageoff;
		int vm_error, max_prot;

addr = addr0;		addr = addr0;
		if ((prot & ~(_PROT_ALL \| PROT_MAX(_PROT_ALL))) != 0)
		return (EINVAL);
		brooksAuthorUnsubmitted Done Inline Actions This check is interesting. Linux documents this behavior (EINVAL for invalid protections), but POSIX seems to endorse our silent stripping. I'm inclined to think Linux is more correct here. brooks: This check is interesting. Linux documents this behavior (EINVAL for invalid protections), but…
		markjUnsubmitted Done Inline Actions We should probably document this error in the man page too. markj: We should probably document this error in the man page too.
		max_prot = PROT_MAX_EXTRACT(prot);
prot = (prot & VM_PROT_ALL);		prot = (prot & VM_PROT_ALL);
		alcUnsubmitted Done Inline Actions While not strictly necessary for correctness, I would advocate changing this to prot = PROT_EXTRACT(prot); alc: While not strictly necessary for correctness, I would advocate changing this to ``` prot =…
pageoff = (addr & PAGE_MASK);		pageoff = (addr & PAGE_MASK);
addr -= pageoff;		addr -= pageoff;
size += pageoff;		size += pageoff;
size = (vm_size_t) round_page(size);		size = (vm_size_t) round_page(size);
#ifdef COMPAT_FREEBSD32		#ifdef COMPAT_FREEBSD32
if (SV_PROC_FLAG(td->td_proc, SV_ILP32)) {		if (SV_PROC_FLAG(td->td_proc, SV_ILP32)) {
if (((addr + size) & 0xffffffff) < addr)		if (((addr + size) & 0xffffffff) < addr)
return (EINVAL);		return (EINVAL);
} else		} else
#endif		#endif
if (addr + size < addr)		if (addr + size < addr)
return (EINVAL);		return (EINVAL);

switch (vm_map_protect(&td->td_proc->p_vmspace->vm_map, addr,		if (max_prot != 0) {
addr + size, prot, FALSE)) {		if ((max_prot & prot) != prot)
		return (EINVAL);
		vm_error = vm_map_protect(&td->td_proc->p_vmspace->vm_map,
		kibUnsubmitted Not Done Inline Actions I do not understand this. Suppose you have a readonly shared mapping of the file, which was created using O_RDONLY file descriptor. Isn't this addition allows to add VM_PROT_WRITE to the corresponding vm_map_entry max_prot ? Then on the next step user can mprotect(2) the mapping to modify the file. kib: I do not understand this. Suppose you have a readonly shared mapping of the file, which was…
		brooksAuthorUnsubmitted Done Inline Actions I think this does the right thing, but I had to read the implementation a couple times to convince myself. In `vm_map_protect`, when `set_max` is `true`, the new `max_prot` is checked against existing entries `max_protection` in the first loop over the region: if ((new_prot & current->max_protection) != new_prot) { vm_map_unlock(map); return (KERN_PROTECTION_FAILURE); } This rejects attempts to upgrade `max_prot`. brooks: I think this does the right thing, but I had to read the implementation a couple times to…
		addr, addr + size, max_prot, TRUE);
		if (vm_error != KERN_SUCCESS)
		goto error_out;
		markjUnsubmitted Done Inline Actions You can avoid a goto by initializing `vm_error` to `KERN_SUCCESS` and skipping the final vm_map_protect() if `vm_error != KERN_SUCCESS`. markj: You can avoid a goto by initializing `vm_error` to `KERN_SUCCESS` and skipping the final…
		}
		vm_error = vm_map_protect(&td->td_proc->p_vmspace->vm_map, addr,
		addr + size, prot, FALSE);

		error_out:
		switch (vm_error) {
case KERN_SUCCESS:		case KERN_SUCCESS:
return (0);		return (0);
case KERN_PROTECTION_FAILURE:		case KERN_PROTECTION_FAILURE:
return (EACCES);		return (EACCES);
case KERN_RESOURCE_SHORTAGE:		case KERN_RESOURCE_SHORTAGE:
return (ENOMEM);		return (ENOMEM);
}		}
return (EINVAL);		return (EINVAL);
▲ Show 20 Lines • Show All 977 Lines • Show Last 20 Lines

View Options

head/chinese/fcitx-rime/distinfo

Show First 20 Lines • Show All 97 Lines • ▼ Show 20 Lines
#endif		#endif

int old_mlock = 0;		int old_mlock = 0;
SYSCTL_INT(_vm, OID_AUTO, old_mlock, CTLFLAG_RWTUN, &old_mlock, 0,		SYSCTL_INT(_vm, OID_AUTO, old_mlock, CTLFLAG_RWTUN, &old_mlock, 0,
"Do not apply RLIMIT_MEMLOCK on mlockall");		"Do not apply RLIMIT_MEMLOCK on mlockall");
static int mincore_mapped = 1;		static int mincore_mapped = 1;
SYSCTL_INT(_vm, OID_AUTO, mincore_mapped, CTLFLAG_RWTUN, &mincore_mapped, 0,		SYSCTL_INT(_vm, OID_AUTO, mincore_mapped, CTLFLAG_RWTUN, &mincore_mapped, 0,
"mincore reports mappings, not residency");		"mincore reports mappings, not residency");
		static int imply_prot_max = 0;
		SYSCTL_INT(_vm, OID_AUTO, imply_prot_max, CTLFLAG_RWTUN, &imply_prot_max, 0,
		emasteUnsubmitted Done Inline Actions `s/mincore_mapped/imply_prot_max/` emaste: `s/mincore_mapped/imply_prot_max/`
		"Imply maximum page permissions in mmap() when none are specified.");
		markjUnsubmitted Done Inline Actions sysctl description strings usually don't contain a period. markj: sysctl description strings usually don't contain a period.

#ifdef MAP_32BIT		#ifdef MAP_32BIT
#define MAP_32BIT_MAX_ADDR ((vm_offset_t)1 << 31)		#define MAP_32BIT_MAX_ADDR ((vm_offset_t)1 << 31)
#endif		#endif

#ifndef _SYS_SYSPROTO_H_		#ifndef _SYS_SYSPROTO_H_
struct sbrk_args {		struct sbrk_args {
int incr;		int incr;
▲ Show 20 Lines • Show All 68 Lines • ▼ Show 20 Lines
kern_mmap(struct thread *td, uintptr_t addr0, size_t size, int prot, int flags,		kern_mmap(struct thread *td, uintptr_t addr0, size_t size, int prot, int flags,
int fd, off_t pos)		int fd, off_t pos)
{		{
struct vmspace *vms;		struct vmspace *vms;
struct file *fp;		struct file *fp;
vm_offset_t addr;		vm_offset_t addr;
vm_size_t pageoff;		vm_size_t pageoff;
vm_prot_t cap_maxprot;		vm_prot_t cap_maxprot;
int align, error;		int align, error, max_prot;
cap_rights_t rights;		cap_rights_t rights;

		if ((prot & ~(_PROT_ALL \| PROT_MAX(_PROT_ALL))) != 0)
		return (EINVAL);
		emasteUnsubmitted Done Inline Actions This has the effect of masking invalid flags (e.g. `PROT_READ \| PROT_WRITE \| 0x100000` or `0xffff` in tests/sys/vm/mmap_test.c). emaste: This has the effect of masking invalid flags (e.g. `PROT_READ \| PROT_WRITE \| 0x100000` or…
		max_prot = PROT_MAX_EXTRACT(prot);
		prot = PROT_EXTRACT(prot);
		markjUnsubmitted Done Inline Actions Don't we also need to verify that prot is a subset of max_prot? markj: Don't we also need to verify that prot is a subset of max_prot?
		/*
		* Always honor PROT_MAX if set. If not, default to all
		* permissions unless we're implying maximum permissions.
		*
		* XXX: should be tunable per process and ABI.
		*/
		if (max_prot == 0)
		max_prot = (imply_prot_max && prot != PROT_NONE) ?
		prot : _PROT_ALL;
		kibUnsubmitted Done Inline Actions I would write this as ` max_prot = imply_prot_max ? prot : _PROT_ALL ` kib: I would write this as ` max_prot = imply_prot_max ? prot : _PROT_ALL `

vms = td->td_proc->p_vmspace;		vms = td->td_proc->p_vmspace;
fp = NULL;		fp = NULL;
AUDIT_ARG_FD(fd);		AUDIT_ARG_FD(fd);
addr = addr0;		addr = addr0;

/*		/*
* Ignore old flags that used to be defined but did not do anything.		* Ignore old flags that used to be defined but did not do anything.
*/		*/
▲ Show 20 Lines • Show All 126 Lines • ▼ Show 20 Lines	error = vm_mmap_object(&vms->vm_map, &addr, size, VM_PROT_NONE,
VM_PROT_NONE, flags, NULL, pos, FALSE, td);		VM_PROT_NONE, flags, NULL, pos, FALSE, td);
} else if ((flags & MAP_ANON) != 0) {		} else if ((flags & MAP_ANON) != 0) {
/*		/*
* Mapping blank space is trivial.		* Mapping blank space is trivial.
*		*
* This relies on VM_PROT_* matching PROT_*.		* This relies on VM_PROT_* matching PROT_*.
*/		*/
error = vm_mmap_object(&vms->vm_map, &addr, size, prot,		error = vm_mmap_object(&vms->vm_map, &addr, size, prot,
VM_PROT_ALL, flags, NULL, pos, FALSE, td);		max_prot, flags, NULL, pos, FALSE, td);
} else {		} else {
/*		/*
* Mapping file, get fp for validation and don't let the		* Mapping file, get fp for validation and don't let the
* descriptor disappear on us if we block. Check capability		* descriptor disappear on us if we block. Check capability
* rights, but also return the maximum rights to be combined		* rights, but also return the maximum rights to be combined
* with maxprot later.		* with maxprot later.
*/		*/
cap_rights_init(&rights, CAP_MMAP);		cap_rights_init(&rights, CAP_MMAP);
if (prot & PROT_READ)		if (prot & PROT_READ)
cap_rights_set(&rights, CAP_MMAP_R);		cap_rights_set(&rights, CAP_MMAP_R);
if ((flags & MAP_SHARED) != 0) {		if ((flags & MAP_SHARED) != 0) {
if (prot & PROT_WRITE)		if (prot & PROT_WRITE)
cap_rights_set(&rights, CAP_MMAP_W);		cap_rights_set(&rights, CAP_MMAP_W);
}		}
if (prot & PROT_EXEC)		if (prot & PROT_EXEC)
cap_rights_set(&rights, CAP_MMAP_X);		cap_rights_set(&rights, CAP_MMAP_X);
error = fget_mmap(td, fd, &rights, &cap_maxprot, &fp);		error = fget_mmap(td, fd, &rights, &cap_maxprot, &fp);
if (error != 0)		if (error != 0)
goto done;		goto done;
if ((flags & (MAP_SHARED \| MAP_PRIVATE)) == 0 &&		if ((flags & (MAP_SHARED \| MAP_PRIVATE)) == 0 &&
		markjUnsubmitted Not Done Inline Actions Shouldn't it be `(max_prot & cap_maxprot) == max_prot`? Otherwise userland can trigger this assertion by requesting a `PROT_MAX(PROT_READ) \| PROT_READ` mapping using a descriptor with read and write rights. markj: Shouldn't it be `(max_prot & cap_maxprot) == max_prot`? Otherwise userland can trigger this…
		brooksAuthorUnsubmitted Done Inline Actions My belief was that this can't happen because of the way fget_mmap is implemented, but rereading, it looks like that's only true when CAPABILITIES is defined (in which case cap_maxprot == prot so this is safe because prot must be a subset of max_prot per the check above). In the non-CAPABILITIES case cap_maxprot is VM_PROT_ALL so this will fail. Dropping the assertion may be the best fix. brooks: My belief was that this can't happen because of the way fget_mmap is implemented, but rereading…
		markjUnsubmitted Done Inline Actions Sorry, I don't see why that assertion worked even with CAPABILITIES defined. fget_mmap() returns the maximum protections permitted by capabilities on the descriptor. markj: Sorry, I don't see why that assertion worked even with CAPABILITIES defined. fget_mmap()…
		brooksAuthorUnsubmitted Done Inline Actions On further reading, you're correct. brooks: On further reading, you're correct.
td->td_proc->p_osrel >= P_OSREL_MAP_FSTRICT) {		td->td_proc->p_osrel >= P_OSREL_MAP_FSTRICT) {
error = EINVAL;		error = EINVAL;
goto done;		goto done;
}		}

/* This relies on VM_PROT_* matching PROT_. /		/* This relies on VM_PROT_* matching PROT_. /
error = fo_mmap(fp, &vms->vm_map, &addr, size, prot,		error = fo_mmap(fp, &vms->vm_map, &addr, size, prot,
cap_maxprot, flags, pos, td);		max_prot & cap_maxprot, flags, pos, td);
		kibUnsubmitted Not Done Inline Actions Should we deny the request if `(max_prot & cap_maxprot) != max_prot`, so that user knows that the call was not really satisfied ? kib: Should we deny the request if `(max_prot & cap_maxprot) != max_prot`, so that user knows that…
		brooksAuthorUnsubmitted Done Inline Actions I'm leaning toward not. I think we'd only want to error when we'd explicitly passed a PROT_MAX(FOO) expression so we'd have to add state for that since we don't want to fail when max_prot defaults to _PROT_ALL or when it's implied. We'd also just add yet another inexplicable EINVAL for a rare case. brooks: I'm leaning toward not. I think we'd only want to error when we'd explicitly passed a PROT_MAX…
		brooksAuthorUnsubmitted Done Inline Actions I've added a KASSERT of this because I'm convinced that with the `(prot & max_prot) == prot` check above that this will always be safe (cap_maxprot ends up set to something derived directly from prot). brooks: I've added a KASSERT of this because I'm convinced that with the `(prot & max_prot) == prot`…
}		}

if (error == 0)		if (error == 0)
td->td_retval[0] = (register_t) (addr + pageoff);		td->td_retval[0] = (register_t) (addr + pageoff);
done:		done:
if (fp)		if (fp)
fdrop(fp, td);		fdrop(fp, td);

▲ Show 20 Lines • Show All 214 Lines • ▼ Show 20 Lines	sys_mprotect(struct thread td, struct mprotect_args uap)
return (kern_mprotect(td, (uintptr_t)uap->addr, uap->len, uap->prot));		return (kern_mprotect(td, (uintptr_t)uap->addr, uap->len, uap->prot));
}		}

int		int
kern_mprotect(struct thread *td, uintptr_t addr0, size_t size, int prot)		kern_mprotect(struct thread *td, uintptr_t addr0, size_t size, int prot)
{		{
vm_offset_t addr;		vm_offset_t addr;
vm_size_t pageoff;		vm_size_t pageoff;
		int vm_error, max_prot;

addr = addr0;		addr = addr0;
		if ((prot & ~(_PROT_ALL \| PROT_MAX(_PROT_ALL))) != 0)
		return (EINVAL);
		brooksAuthorUnsubmitted Done Inline Actions This check is interesting. Linux documents this behavior (EINVAL for invalid protections), but POSIX seems to endorse our silent stripping. I'm inclined to think Linux is more correct here. brooks: This check is interesting. Linux documents this behavior (EINVAL for invalid protections), but…
		markjUnsubmitted Done Inline Actions We should probably document this error in the man page too. markj: We should probably document this error in the man page too.
		max_prot = PROT_MAX_EXTRACT(prot);
prot = (prot & VM_PROT_ALL);		prot = (prot & VM_PROT_ALL);
		alcUnsubmitted Done Inline Actions While not strictly necessary for correctness, I would advocate changing this to prot = PROT_EXTRACT(prot); alc: While not strictly necessary for correctness, I would advocate changing this to ``` prot =…
pageoff = (addr & PAGE_MASK);		pageoff = (addr & PAGE_MASK);
addr -= pageoff;		addr -= pageoff;
size += pageoff;		size += pageoff;
size = (vm_size_t) round_page(size);		size = (vm_size_t) round_page(size);
#ifdef COMPAT_FREEBSD32		#ifdef COMPAT_FREEBSD32
if (SV_PROC_FLAG(td->td_proc, SV_ILP32)) {		if (SV_PROC_FLAG(td->td_proc, SV_ILP32)) {
if (((addr + size) & 0xffffffff) < addr)		if (((addr + size) & 0xffffffff) < addr)
return (EINVAL);		return (EINVAL);
} else		} else
#endif		#endif
if (addr + size < addr)		if (addr + size < addr)
return (EINVAL);		return (EINVAL);

switch (vm_map_protect(&td->td_proc->p_vmspace->vm_map, addr,		if (max_prot != 0) {
addr + size, prot, FALSE)) {		if ((max_prot & prot) != prot)
		return (EINVAL);
		vm_error = vm_map_protect(&td->td_proc->p_vmspace->vm_map,
		kibUnsubmitted Not Done Inline Actions I do not understand this. Suppose you have a readonly shared mapping of the file, which was created using O_RDONLY file descriptor. Isn't this addition allows to add VM_PROT_WRITE to the corresponding vm_map_entry max_prot ? Then on the next step user can mprotect(2) the mapping to modify the file. kib: I do not understand this. Suppose you have a readonly shared mapping of the file, which was…
		brooksAuthorUnsubmitted Done Inline Actions I think this does the right thing, but I had to read the implementation a couple times to convince myself. In `vm_map_protect`, when `set_max` is `true`, the new `max_prot` is checked against existing entries `max_protection` in the first loop over the region: if ((new_prot & current->max_protection) != new_prot) { vm_map_unlock(map); return (KERN_PROTECTION_FAILURE); } This rejects attempts to upgrade `max_prot`. brooks: I think this does the right thing, but I had to read the implementation a couple times to…
		addr, addr + size, max_prot, TRUE);
		if (vm_error != KERN_SUCCESS)
		goto error_out;
		markjUnsubmitted Done Inline Actions You can avoid a goto by initializing `vm_error` to `KERN_SUCCESS` and skipping the final vm_map_protect() if `vm_error != KERN_SUCCESS`. markj: You can avoid a goto by initializing `vm_error` to `KERN_SUCCESS` and skipping the final…
		}
		vm_error = vm_map_protect(&td->td_proc->p_vmspace->vm_map, addr,
		addr + size, prot, FALSE);

		error_out:
		switch (vm_error) {
case KERN_SUCCESS:		case KERN_SUCCESS:
return (0);		return (0);
case KERN_PROTECTION_FAILURE:		case KERN_PROTECTION_FAILURE:
return (EACCES);		return (EACCES);
case KERN_RESOURCE_SHORTAGE:		case KERN_RESOURCE_SHORTAGE:
return (ENOMEM);		return (ENOMEM);
}		}
return (EINVAL);		return (EINVAL);
▲ Show 20 Lines • Show All 977 Lines • Show Last 20 Lines

Upgrade chinese/fcitx-rime to use RimeData-based schemas.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Updating D27630: Upgrade fcitx-rime to use rime-data.

Revision Contents
Changeset List

Diff 81304

head/chinese/fcitx-rime/Makefile

head/chinese/fcitx-rime/distinfo

Upgrade chinese/fcitx-rime to use RimeData-based schemas.ClosedPublicActions

Details

Diff Detail

Event Timeline

Updating D27630: Upgrade fcitx-rime to use rime-data.

Revision ContentsChangeset List

Diff 81304

head/chinese/fcitx-rime/Makefile

head/chinese/fcitx-rime/distinfo

Upgrade chinese/fcitx-rime to use RimeData-based schemas.
ClosedPublic
Actions

Revision Contents
Changeset List