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
