Convert the last users of copystr that were not using the length
argument to strlcpy.
Details
Details
- Reviewers
mhorne
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 71837 Build 68720: arc lint + arc unit
Event Timeline
| sys/dev/vmm/vmm_dev.c | ||
|---|---|---|
| 274 | Strlcpy returns the number of characters that would be copied if dst is large enought. Not an error | |
Comment Actions
Hi, thanks for taking this on, it would be nice to see this conversion completed.
Most important: the order of 'src' and 'dest' is reversed between copystr() and strlcpy(). You have not accounted for this.
Second: there are more instances of copystr() in the tree needing conversion that do not yet appear in this patch.
| sys/dev/vmm/vmm_dev.c | ||
|---|---|---|
| 274 | Clever, but not quite right. In this case, error is propagated upwards, and eventually returned to userspace. We need to take care to set a real error code, not a boolean value. You can see that copystr() is defined as a macro (in sys/systm.h) as a wrapper for strlcpy(). So in each of these instances the goal is likely to de-inline that macro, returning ENAMETOOLONG wherever it does now. Similar advice applies below. | |
Comment Actions
Yeez, that was a bad shot from the hip. Sorry about that.
Give me some time and I do the complete thing with those cases that do use the length return argument, too,. Just not late at night.
Comment Actions
It's worse. copystr and strlcpy also have a different
idea of what string length means. Taken into account now.
Comment Actions
Good catch. Strings are very hard...
| sys/dev/vmm/vmm_dev.c | ||
|---|---|---|
| 301–303 | ||
| sys/kern/imgact_shell.c | ||
| 238–239 | We can drop this one safely, IMO. The flow of the function is such that we assume error is already initialized. | |
| sys/kern/kern_exec.c | ||
| 1580–1595 ↗ | (On Diff #174719) | |
| 1610–1612 ↗ | (On Diff #174719) | Hmmm, I would say this is one of the motivating examples where the usefulness of copystr() becomes clearer. Still, let us proceed. |
| 1621–1635 ↗ | (On Diff #174719) | |
| sys/kern/vfs_lookup.c | ||
| 468–475 ↗ | (On Diff #174719) | For your consideration: what do you think of this pattern? It appears to be frequent enough with uses of strlcpy() within the kernel. However, the explicitness of your current version regarding the NUL terminator is useful, IMO. |
| sys/sys/systm.h | ||
| 292–302 ↗ | (On Diff #174719) | The actual removal and version bump should be split into a separate commit. If you like I can handle this part when the whole review is ready. |
| sys/ufs/ufs/ufs_extattr.c | ||
| 282 ↗ | (On Diff #174719) | I believe if you remove this, the increment added above is not needed. |
| 287 ↗ | (On Diff #174719) | By inspection, this function is where cnp.cn_namelen is consumed. |