Message ID | 20230606182410.3976487-1-azeemshaikh38@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v2] uml: Replace strlcpy with strscpy | expand |
----- Ursprüngliche Mail ----- > Von: "Azeem Shaikh" <azeemshaikh38@gmail.com> > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > No return values were used, so direct replacement is safe. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > [2] https://github.com/KSPP/linux/issues/89 > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> > Reported-by: kernel test robot <lkp@intel.com> > Closes: > https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/ Are you sure Reported-by and Closes make sense? AFAIK the report was only on your first patch and nothing against upstream. So stating this in the updated patch is in vain. Other than that, Acked-by: Richard Weinberger <richard@nod.at> Thanks, //richard
On Tue, Jun 6, 2023 at 4:51 PM Richard Weinberger <richard@nod.at> wrote: > > ----- Ursprüngliche Mail ----- > > Von: "Azeem Shaikh" <azeemshaikh38@gmail.com> > > strlcpy() reads the entire source buffer first. > > This read may exceed the destination size limit. > > This is both inefficient and can lead to linear read > > overflows if a source string is not NUL-terminated [1]. > > In an effort to remove strlcpy() completely [2], replace > > strlcpy() here with strscpy(). > > No return values were used, so direct replacement is safe. > > > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > > [2] https://github.com/KSPP/linux/issues/89 > > > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: > > https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/ > > Are you sure Reported-by and Closes make sense? > AFAIK the report was only on your first patch and nothing against upstream. > So stating this in the updated patch is in vain. I left the metadata in only for the sake of posterity. If it's not helpful, I'm ok with removing it. > Other than that, > Acked-by: Richard Weinberger <richard@nod.at> Thanks!
On Tue, Jun 06, 2023 at 05:08:27PM -0400, Azeem Shaikh wrote: > On Tue, Jun 6, 2023 at 4:51 PM Richard Weinberger <richard@nod.at> wrote: > > > > ----- Ursprüngliche Mail ----- > > > Von: "Azeem Shaikh" <azeemshaikh38@gmail.com> > > > strlcpy() reads the entire source buffer first. > > > This read may exceed the destination size limit. > > > This is both inefficient and can lead to linear read > > > overflows if a source string is not NUL-terminated [1]. > > > In an effort to remove strlcpy() completely [2], replace > > > strlcpy() here with strscpy(). > > > No return values were used, so direct replacement is safe. > > > > > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > > > [2] https://github.com/KSPP/linux/issues/89 > > > > > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> > > > Reported-by: kernel test robot <lkp@intel.com> > > > Closes: > > > https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/ > > > > Are you sure Reported-by and Closes make sense? > > AFAIK the report was only on your first patch and nothing against upstream. > > So stating this in the updated patch is in vain. > > I left the metadata in only for the sake of posterity. If it's not > helpful, I'm ok with removing it. > IMO using Reported-by in cases like this is harmful, as it makes commits seem like bug fixes when they are not. - Eric
On Tue, 2023-06-06 at 21:23 -0700, Eric Biggers wrote: > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > Closes: > > > > https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/ > > > > > > Are you sure Reported-by and Closes make sense? > > > AFAIK the report was only on your first patch and nothing against upstream. > > > So stating this in the updated patch is in vain. > > > > I left the metadata in only for the sake of posterity. If it's not > > helpful, I'm ok with removing it. > > > > IMO using Reported-by in cases like this is harmful, as it makes commits seem > like bug fixes when they are not. I've yet to see anyone disagree with that, and yet, the robot actively asks for such tags to be included in patch resubmissions. On the one hand, I can understand their desire to be recognised for their efforts. At this point then we might suggest that we introduce a different tag, say "Improved-by:" or "Issues-found-by:" or something. On the other hand, I don't feel like we should give a robot more recognition than we give _people_ reviewing, and we currently really only recognise them by a Reviewed-by tag. Then again, that doesn't work with the robot - it is pretty much looking at each patch and only comments on a small fraction. We also really don't want it to comment on each and every patch ... So it seems we should ask the robot maintainers to just stop suggesting those tags? johannes
----- Ursprüngliche Mail ----- > Von: "Johannes Berg" <johannes@sipsolutions.net> > On Tue, 2023-06-06 at 21:23 -0700, Eric Biggers wrote: >> >> > > > Reported-by: kernel test robot <lkp@intel.com> >> > > > Closes: >> > > > https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/ >> > > >> > > Are you sure Reported-by and Closes make sense? >> > > AFAIK the report was only on your first patch and nothing against upstream. >> > > So stating this in the updated patch is in vain. >> > >> > I left the metadata in only for the sake of posterity. If it's not >> > helpful, I'm ok with removing it. >> > >> >> IMO using Reported-by in cases like this is harmful, as it makes commits seem >> like bug fixes when they are not. > > I've yet to see anyone disagree with that, and yet, the robot actively > asks for such tags to be included in patch resubmissions. > > On the one hand, I can understand their desire to be recognised for > their efforts. At this point then we might suggest that we introduce a > different tag, say "Improved-by:" or "Issues-found-by:" or something. Robots don't have feelings. No need to worry. ;-) > On the other hand, I don't feel like we should give a robot more > recognition than we give _people_ reviewing, and we currently really > only recognise them by a Reviewed-by tag. Then again, that doesn't work > with the robot - it is pretty much looking at each patch and only > comments on a small fraction. We also really don't want it to comment on > each and every patch ... > > > So it seems we should ask the robot maintainers to just stop suggesting > those tags? Agreed. Thanks, //richard
On Wed, Jun 07, 2023 at 10:34:55AM +0200, Richard Weinberger wrote: > ----- Ursprüngliche Mail ----- > > Von: "Johannes Berg" <johannes@sipsolutions.net> > > On Tue, 2023-06-06 at 21:23 -0700, Eric Biggers wrote: > >> > >> > > > Reported-by: kernel test robot <lkp@intel.com> > >> > > > Closes: > >> > > > https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/ > >> > > > >> > > Are you sure Reported-by and Closes make sense? > >> > > AFAIK the report was only on your first patch and nothing against upstream. > >> > > So stating this in the updated patch is in vain. > >> > > >> > I left the metadata in only for the sake of posterity. If it's not > >> > helpful, I'm ok with removing it. > >> > > >> > >> IMO using Reported-by in cases like this is harmful, as it makes commits seem > >> like bug fixes when they are not. > > > > I've yet to see anyone disagree with that, and yet, the robot actively > > asks for such tags to be included in patch resubmissions. > > > > On the one hand, I can understand their desire to be recognised for > > their efforts. At this point then we might suggest that we introduce a > > different tag, say "Improved-by:" or "Issues-found-by:" or something. > > Robots don't have feelings. No need to worry. ;-) > > > On the other hand, I don't feel like we should give a robot more > > recognition than we give _people_ reviewing, and we currently really > > only recognise them by a Reviewed-by tag. Then again, that doesn't work > > with the robot - it is pretty much looking at each patch and only > > comments on a small fraction. We also really don't want it to comment on > > each and every patch ... > > > > > > So it seems we should ask the robot maintainers to just stop suggesting > > those tags? > > Agreed. Thanks all for the feedback. We will carefully consider how to present the suggestion clearly. For now, because the bot covers both upstream and developer repos, there can be various situations, such as the bug is found in upstream. So the bot tries to let author decide how to apply the tags in appropriate way that they feel comfortable. In the report, we now uses phrases like below If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/ But this may be not clear enough or not the best way to suggest. We will consider whether we can detect some situations (like RFC patch) which is no need for such tags to avoid confusion. > > Thanks, > //richard
On Wed, 2023-06-07 at 17:10 +0800, Philip Li wrote: > > > So it seems we should ask the robot maintainers to just stop suggesting > > > those tags? > > > > Agreed. > > Thanks all for the feedback. We will carefully consider how to present the > suggestion clearly. > > For now, because the bot covers both upstream and developer repos, there > can be various situations, such as the bug is found in upstream. Ah yes, that was actually in my mind, but I forgot to write about it, sorry. I agree completely, in case that you find a bug in an already committed tree, and there will be a separate commit to fix it, it's completely reasonable and useful to have those tags. > So the bot > tries to let author decide how to apply the tags in appropriate way that > they feel comfortable. Right. It just seems that many authors aren't really all that familiar with the processes yet, and take the suggestion at face value. > In the report, we now uses phrases like below > > If you fix the issue, kindly add following tag where applicable > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/ > > But this may be not clear enough or not the best way to suggest. We will > consider whether we can detect some situations (like RFC patch) which is > no need for such tags to avoid confusion. > Right. Maybe the only thing really needed would be to say something like "If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add ..." or even just "If you fix the issue in a separate commit, kindly add ..." so it's clear that if you're changing the commit, it's not really something that should be done? In which case probably even a Fixes tag should be there, but I wouldn't want to recommend adding that since the commits may still change etc. I don't know all the processes behind it, but I'm thinking that even if the bot picked up a patch from the list, it could get committed before and then fixed in a separate commit. johannes
On Wed, Jun 07, 2023 at 11:17:54AM +0200, Johannes Berg wrote: > On Wed, 2023-06-07 at 17:10 +0800, Philip Li wrote: > > > > So it seems we should ask the robot maintainers to just stop suggesting > > > > those tags? > > > > > > Agreed. > > > > Thanks all for the feedback. We will carefully consider how to present the > > suggestion clearly. > > > > For now, because the bot covers both upstream and developer repos, there > > can be various situations, such as the bug is found in upstream. > > Ah yes, that was actually in my mind, but I forgot to write about it, > sorry. > > I agree completely, in case that you find a bug in an already committed > tree, and there will be a separate commit to fix it, it's completely > reasonable and useful to have those tags. > > > So the bot > > tries to let author decide how to apply the tags in appropriate way that > > they feel comfortable. > > Right. It just seems that many authors aren't really all that familiar > with the processes yet, and take the suggestion at face value. > > > In the report, we now uses phrases like below > > > > If you fix the issue, kindly add following tag where applicable > > | Reported-by: kernel test robot <lkp@intel.com> > > | Closes: https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/ > > > > But this may be not clear enough or not the best way to suggest. We will > > consider whether we can detect some situations (like RFC patch) which is > > no need for such tags to avoid confusion. > > > > Right. Maybe the only thing really needed would be to say something like Thanks a lot, the suggestion really helps us to get this better. > > "If you fix the issue in a separate patch/commit (i.e. not just a new > version of the same patch/commit), kindly add ..." Is that ok we just take this phrase as a quick improvement for first step, which is "If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags:" This could help remind for most cases if not all. Also this allows us not doing "complex" judgement by the bot itself. > > or even just > > "If you fix the issue in a separate commit, kindly add ..." > > so it's clear that if you're changing the commit, it's not really > something that should be done? In which case probably even a Fixes tag > should be there, but I wouldn't want to recommend adding that since the > commits may still change etc. > > I don't know all the processes behind it, but I'm thinking that even if > the bot picked up a patch from the list, it could get committed before > and then fixed in a separate commit. You are right, thanks for reminding this. The bot monitors both patches in the mailing list and repos of developers. It could happen that a patch exists in both place, though there's logic to avoid testing both but can't promise which side got tested first. > > johannes
On Wed, 2023-06-07 at 17:39 +0800, Philip Li wrote: > > Is that ok we just take this phrase as a quick improvement for first step, which > is > > "If you fix the issue in a separate patch/commit (i.e. not just a new > version of the same patch/commit), kindly add following tags:" > > This could help remind for most cases if not all. Also this allows us > not doing "complex" judgement by the bot itself. Sure, feel free. But yeah, that was the idea, that the decision logic wouldn't really need to be in the bot, that feels very difficult. Maybe someone else on the thread will have a better suggestion :) johannes
On Wed, Jun 07, 2023 at 11:43:11AM +0200, Johannes Berg wrote: > On Wed, 2023-06-07 at 17:39 +0800, Philip Li wrote: > > > > Is that ok we just take this phrase as a quick improvement for first step, which > > is > > > > "If you fix the issue in a separate patch/commit (i.e. not just a new > > version of the same patch/commit), kindly add following tags:" > > > > This could help remind for most cases if not all. Also this allows us > > not doing "complex" judgement by the bot itself. > > Sure, feel free. But yeah, that was the idea, that the decision logic > wouldn't really need to be in the bot, that feels very difficult. > > Maybe someone else on the thread will have a better suggestion :) Thanks a lot, we will adjust the phrase like this as initial step to resolve some confusions. And as you said, we welcome more suggestions to improve the bot. > > > johannes
diff --git a/arch/um/include/shared/user.h b/arch/um/include/shared/user.h index bda66e5a9d4e..0347a190429c 100644 --- a/arch/um/include/shared/user.h +++ b/arch/um/include/shared/user.h @@ -52,6 +52,7 @@ static inline int printk(const char *fmt, ...) extern int in_aton(char *str); extern size_t strlcpy(char *, const char *, size_t); extern size_t strlcat(char *, const char *, size_t); +extern size_t strscpy(char *, const char *, size_t); /* Copied from linux/compiler-gcc.h since we can't include it directly */ #define barrier() __asm__ __volatile__("": : :"memory") diff --git a/arch/um/os-Linux/drivers/tuntap_user.c b/arch/um/os-Linux/drivers/tuntap_user.c index 53eb3d508645..2284e9c1cbbb 100644 --- a/arch/um/os-Linux/drivers/tuntap_user.c +++ b/arch/um/os-Linux/drivers/tuntap_user.c @@ -146,7 +146,7 @@ static int tuntap_open(void *data) } memset(&ifr, 0, sizeof(ifr)); ifr.ifr_flags = IFF_TAP | IFF_NO_PI; - strlcpy(ifr.ifr_name, pri->dev_name, sizeof(ifr.ifr_name)); + strscpy(ifr.ifr_name, pri->dev_name, sizeof(ifr.ifr_name)); if (ioctl(pri->fd, TUNSETIFF, &ifr) < 0) { err = -errno; printk(UM_KERN_ERR "TUNSETIFF failed, errno = %d\n",
strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy(). No return values were used, so direct replacement is safe. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [2] https://github.com/KSPP/linux/issues/89 Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/ --- v1: https://lore.kernel.org/all/20230530164004.986750-1-azeemshaikh38@gmail.com/ Changes from v1 - added strscpy declaration. v1 does not build. arch/um/include/shared/user.h | 1 + arch/um/os-Linux/drivers/tuntap_user.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)