Re: [PATCH 22/67] entry.c: convert strcpy to xsnprintf
On Tue, Sep 15, 2015 at 12:01 PM, Ramsay Jones wrote: > > > On 15/09/15 16:40, Jeff King wrote: >> This particular conversion is non-obvious, because nobody >> has passed our function the length of the destination >> buffer. However, the interface to checkout_entry specifies >> that the buffer must be at least TEMPORARY_FILENAME_LENGTH >> bytes long, so we can check that (meaning the existing code >> was not buggy, but merely worrisome to somebody reading it). >> >> Signed-off-by: Jeff King >> --- >> entry.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/entry.c b/entry.c >> index 1eda8e9..582c400 100644 >> --- a/entry.c >> +++ b/entry.c >> @@ -96,8 +96,8 @@ static int open_output_fd(char *path, const struct >> cache_entry *ce, int to_tempf >> { >> int symlink = (ce->ce_mode & S_IFMT) != S_IFREG; >> if (to_tempfile) { >> - strcpy(path, symlink >> -? ".merge_link_XX" : ".merge_file_XX"); >> + xsnprintf(path, TEMPORARY_FILENAME_LENGTH, "%s", >> + symlink ? ".merge_link_XX" : >> ".merge_file_XX"); >> return mkstemp(path); >> } else { >> return create_file(path, !symlink ? ce->ce_mode : 0666); > > Hmm, I was going to suggest strlcpy() again. However, if you expect an > overflow to > occur, then xsnprintf() will at least bring it to your attention! Checking > for overflow > with strlcpy() is not rocket science either, and I guess we could add > xstrlcpy() ... :-D I mean having a default action "if overflow, then die" suits most of the cases. It should be a deliberate decision to not die in case of an overflow. (Why did you allocate not enough memory in the first place?) > > dunno. > > ATB, > Ramsay Jones > > > > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 22/67] entry.c: convert strcpy to xsnprintf
On 15/09/15 16:40, Jeff King wrote: > This particular conversion is non-obvious, because nobody > has passed our function the length of the destination > buffer. However, the interface to checkout_entry specifies > that the buffer must be at least TEMPORARY_FILENAME_LENGTH > bytes long, so we can check that (meaning the existing code > was not buggy, but merely worrisome to somebody reading it). > > Signed-off-by: Jeff King > --- > entry.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/entry.c b/entry.c > index 1eda8e9..582c400 100644 > --- a/entry.c > +++ b/entry.c > @@ -96,8 +96,8 @@ static int open_output_fd(char *path, const struct > cache_entry *ce, int to_tempf > { > int symlink = (ce->ce_mode & S_IFMT) != S_IFREG; > if (to_tempfile) { > - strcpy(path, symlink > -? ".merge_link_XX" : ".merge_file_XX"); > + xsnprintf(path, TEMPORARY_FILENAME_LENGTH, "%s", > + symlink ? ".merge_link_XX" : > ".merge_file_XX"); > return mkstemp(path); > } else { > return create_file(path, !symlink ? ce->ce_mode : 0666); Hmm, I was going to suggest strlcpy() again. However, if you expect an overflow to occur, then xsnprintf() will at least bring it to your attention! Checking for overflow with strlcpy() is not rocket science either, and I guess we could add xstrlcpy() ... :-D dunno. ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 22/67] entry.c: convert strcpy to xsnprintf
This particular conversion is non-obvious, because nobody has passed our function the length of the destination buffer. However, the interface to checkout_entry specifies that the buffer must be at least TEMPORARY_FILENAME_LENGTH bytes long, so we can check that (meaning the existing code was not buggy, but merely worrisome to somebody reading it). Signed-off-by: Jeff King --- entry.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/entry.c b/entry.c index 1eda8e9..582c400 100644 --- a/entry.c +++ b/entry.c @@ -96,8 +96,8 @@ static int open_output_fd(char *path, const struct cache_entry *ce, int to_tempf { int symlink = (ce->ce_mode & S_IFMT) != S_IFREG; if (to_tempfile) { - strcpy(path, symlink - ? ".merge_link_XX" : ".merge_file_XX"); + xsnprintf(path, TEMPORARY_FILENAME_LENGTH, "%s", + symlink ? ".merge_link_XX" : ".merge_file_XX"); return mkstemp(path); } else { return create_file(path, !symlink ? ce->ce_mode : 0666); -- 2.6.0.rc2.408.ga2926b9 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html