Messages in this thread | | | Date | Wed, 29 Apr 2015 01:35:14 +0100 | From | Al Viro <> | Subject | Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy" |
| |
On Tue, Apr 28, 2015 at 02:48:45PM -0700, Linus Torvalds wrote:
> I suspect you could take that lib/strncpy_from_user.c and massage it > reasonably trivially to be a good function. > > That said, I can't think of a single strncpy (or strlcpy) in kernel > space that is actually worth even optimizing for. They just don't tend > to be in the critical path. So correctness is likely *much* more > important than worrying about performance.
Indeed. As it is, I suspect that strlcpy() use should be uniformly discouraged; if nothing else, snprintf() gives the same semantics, is less likely to cause confusion regardling the expected return value and none of those paths are performance-critical.
strncpy() has another use, though, and it can't be replaced by strlcpy() - see the commits that had started this thread. IMO they (and anything else of the same nature) really need to be reverted; using strlcpy() on something that isn't guaranteed to be NUL-terminated is a serious bug.
And catching all such places is going to be quite a work - there are too many strlcpy() callers out there.
Frankly, looking through call sites in fs... * two callers in fs/9p - strlcpy() + sscanf(), both should've been plain sscanf() (and the second should've been "HARDLINKCOUNT%u" instead of "%13s %u" + comparison of string with "HARDLINKCOUNT" - sscanf() is quite capable of matching explicit string literals) * affs one: match_strdup + strlcpy + kfree. Should just use match_strlcpy instead (BTW, despite the name, it does *not* use strclpy() internally). * afs: might be correct. * two in befs: both broken. * binfmt_misc: fishy; load_misc_binary() finds an object under rwlock, copies one of its fields (->interpreter), drops rwlock and proceeds to do various blocking operations (including open_exec()). Using another field of the same object (->interp_flags) all along. If it gets freed and reused, well... let's hope we won't fuck up too badly. IMO we'd be better off if we added a refcount to that sucker and used it to control the freeing. * btrfs: undefined behaviour - potentially overlapping source and destination. * another btrfs one: char b[BDEVNAME_SIZE]; strlcpy(s->s_id, bdevname(bdev, b), sizeof(s->s_id)); complete garbage; might as well do bdevname(bdev, s->s_id) and be done with that.
... and so on; this stuff is misused more often than not.
| |