Changeset View
Standalone View
sys/kern/kern_descrip.c
Show First 20 Lines • Show All 1,487 Lines • ▼ Show 20 Lines | |||||
* Copy filecaps structure allocating memory for ioctls array if needed. | * Copy filecaps structure allocating memory for ioctls array if needed. | ||||
* | * | ||||
* The last parameter indicates whether the fdtable is locked. If it is not and | * The last parameter indicates whether the fdtable is locked. If it is not and | ||||
* ioctls are encountered, copying fails and the caller must lock the table. | * ioctls are encountered, copying fails and the caller must lock the table. | ||||
* | * | ||||
* Note that if the table was not locked, the caller has to check the relevant | * Note that if the table was not locked, the caller has to check the relevant | ||||
* sequence counter to determine whether the operation was successful. | * sequence counter to determine whether the operation was successful. | ||||
*/ | */ | ||||
int | bool | ||||
filecaps_copy(const struct filecaps *src, struct filecaps *dst, bool locked) | filecaps_copy(const struct filecaps *src, struct filecaps *dst, bool locked) | ||||
{ | { | ||||
size_t size; | size_t size; | ||||
if (src->fc_ioctls != NULL && !locked) | |||||
return (false); | |||||
*dst = *src; | *dst = *src; | ||||
mjg: I think this avoidably extends the branchfest. Instead we can retain *dst = *src and just NULL… | |||||
jhbAuthorUnsubmitted Not Done Inline ActionsI considered that (adding the NULL), but it would mean triggering an assertion later in filecaps_validate() (OTOH, we probably shouldn't be using the returned filecaps anyway in the 'false' case). That said, I do think there is a tradeoff between robustness / clarity and performance. I'm not sure losing one extra branch is worth leaving the filecaps in a weird state. We could perhaps just do filecaps_init() before returning false and clear the entire thing. Seems clearer to avoid changing it at all though. jhb: I considered that (adding the NULL), but it would mean triggering an assertion later in… | |||||
mjgUnsubmitted Not Done Inline ActionsIt's not going to get into weird state. If the is held, we always get the stable copy by definition as nobody can modify caps. If the lock is not held, the caller is supposed to verify the sequence counter which fget_cap is doing. If the sequence counter check says it's fine, the whole object was read correctly (otherwise retries). mjg: It's not going to get into weird state. If the is held, we always get the stable copy by… | |||||
jhbAuthorUnsubmitted Not Done Inline ActionsIf the caller isn't buggy. In this case the caller was and the inconsistent state was used. If we set it to NULL it would just be a different inconsistent state if a future caller is buggy and returns it. jhb: If the caller isn't buggy. In this case the caller was and the inconsistent state was used. | |||||
if (src->fc_ioctls == NULL) | if (src->fc_ioctls == NULL) | ||||
return (0); | return (true); | ||||
if (!locked) | |||||
return (1); | |||||
KASSERT(src->fc_nioctls > 0, | KASSERT(src->fc_nioctls > 0, | ||||
("fc_ioctls != NULL, but fc_nioctls=%hd", src->fc_nioctls)); | ("fc_ioctls != NULL, but fc_nioctls=%hd", src->fc_nioctls)); | ||||
size = sizeof(src->fc_ioctls[0]) * src->fc_nioctls; | size = sizeof(src->fc_ioctls[0]) * src->fc_nioctls; | ||||
dst->fc_ioctls = malloc(size, M_FILECAPS, M_WAITOK); | dst->fc_ioctls = malloc(size, M_FILECAPS, M_WAITOK); | ||||
bcopy(src->fc_ioctls, dst->fc_ioctls, size); | bcopy(src->fc_ioctls, dst->fc_ioctls, size); | ||||
return (0); | return (true); | ||||
} | } | ||||
static u_long * | static u_long * | ||||
filecaps_copy_prep(const struct filecaps *src) | filecaps_copy_prep(const struct filecaps *src) | ||||
{ | { | ||||
u_long *ioctls; | u_long *ioctls; | ||||
size_t size; | size_t size; | ||||
▲ Show 20 Lines • Show All 2,736 Lines • Show Last 20 Lines |
I think this avoidably extends the branchfest. Instead we can retain *dst = *src and just NULL out the dst->fc_ioctls pointer. Later on copying is needed it is being overwritten anyway.
No opinion about rętval changes.
Still, weird the regular capsicum tests did not run into this.