[PATCH] mingw-multibyte: fix memory acces violation and path length limits.

[PATCH] mingw-multibyte: fix memory acces violation and path length limits.

[Wataru Noguchi]

fix: Git for Windows crashes when clone Japanese multibyte repository.

Reproduce condition:

- Japanese Base Encoding is Shift-JIS.
- It happens Japanese multibyte directory name and too-long directory path
- Linux(ex. Ubuntu 13.04 amd64) can clone normally.
- example repository is here:

git clone https://github.com/wnoguchi/mingw-checkout-crash.git

- The reproduce crash repository contains following file only.
  - following directory and file name is encoded for this commit log.
  - actually file name is decoded.]
  %E6%97%A5%E6%9C%AC%E8%AA%9E%E3%83%87%E3%82%A3%E3%83%AC%E3%82%AF%E3%83%88%E3%83%AA%201-long-long-long-dirname/%E6%97%A5%E6%9C%AC%E8%AA%9E%E3%83%87%E3%82%A3%E3%83%AC%E3%82%AF%E3%83%88%E3%83%AA%202-long-long-long-dirname/%E6%97%A5%E6%9C%AC%E8%AA%9E%E3%83%87%E3%82%A3%E3%83%AC%E3%82%AF%E3%83%88%E3%83%AA%203-long-long-long-dirname/%E6%97%A5%E6%9C%AC%E8%AA%9E%E3%83%87%E3%82%A3%E3%83%AC%E3%82%AF%E3%83%88%E3%83%AA%204-long-long-long-dirname/%E6%97%A5%E6%9C%AC%E8%AA%9E%E3%83%87%E3%82%A3%E3%83%AC%E3%82%AF%E3%83%88%E3%83%AA%205-long-long-long-dirname/%E3%81%AF%E3%81%98%E3%82%81%E3%81%AB%E3%81%8A%E8%AA%AD%E3%81%BF%E3%81%8F%E3%81%A0%E3%81%95%E3%81%84.txt
- only one commit.

Cause:

- convert_attrs() in convert.c: if (!ccheck[0].attr) but ccheck[0].attr always not NULL.
  thus git_check_attr() in attr.c (check[i].attr->attr_nr) cause access violation.
- checkout_entry() in entry.c: static char path[PATH_MAX + 1]; declared.
  But its size is 261 on MinGW environment.
  This length is Windows full path limits. But for relative path is too short.

This commit fixes:

- convert_attrs() in convert.c: initialize ccheck[0].attr with NULL.
- git-compat-util.h: redifine PATH_MAX value to 4096 when MinGW environment.

Signed-off-by: Wataru Noguchi <wnoguchi.0727@gmail.com>
---
 convert.c         |  5 +++++
 git-compat-util.h | 10 ++++++++++
 2 files changed, 15 insertions(+)

diff --git a/convert.c b/convert.c
index 11a95fc..5eaa206 100644
--- a/convert.c
+++ b/convert.c
@@ -724,6 +724,11 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
 {
 	int i;
 	static struct git_attr_check ccheck[NUM_CONV_ATTRS];
+	
+	if (NUM_CONV_ATTRS != 0) {
+		ccheck[0].attr = NULL;
+		ccheck[0].value = NULL;
+	}
 
 	if (!ccheck[0].attr) {
 		for (i = 0; i < NUM_CONV_ATTRS; i++)
diff --git a/git-compat-util.h b/git-compat-util.h
index a31127f..ba02c69 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -237,6 +237,16 @@ extern char *gitbasename(char *);
 #ifndef PATH_MAX
 #define PATH_MAX 4096
 #endif
+#ifdef GIT_WINDOWS_NATIVE
+/* Git for Windows checkout PATH_MAX is reduce to 260.
+ * but if checkout relative long path name, its length too short.
+ * thus, expand length.
+ */
+#ifdef PATH_MAX
+#undef PATH_MAX
+#endif
+#define PATH_MAX 4096
+#endif
 
 #ifndef PRIuMAX
 #define PRIuMAX "llu"
-- 
1.8.1.2

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] mingw-multibyte: fix memory acces violation and path length limits.

[Johannes Schindelin]

Hi,

On Sun, 29 Sep 2013, Wataru Noguchi wrote:

> --- a/convert.c
> +++ b/convert.c
> @@ -724,6 +724,11 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
>  {
>  	int i;
>  	static struct git_attr_check ccheck[NUM_CONV_ATTRS];
> +	
> +	if (NUM_CONV_ATTRS != 0) {
> +		ccheck[0].attr = NULL;
> +		ccheck[0].value = NULL;
> +	}

I wonder whether it would make more sense to use

	memset(ccheck, 0, sizeof(ccheck))

? But then, ccheck is static and *should* be initialized to all 0
according to the C standard. And re-initializing it to NULL would
invalidate the values that were set earlier.

Also, if NUM_CONV_ATTRS == 0, I would expect

>  	if (!ccheck[0].attr) {

to access an invalid location...

> diff --git a/git-compat-util.h b/git-compat-util.h
> index a31127f..ba02c69 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -237,6 +237,16 @@ extern char *gitbasename(char *);
>  #ifndef PATH_MAX
>  #define PATH_MAX 4096
>  #endif
> +#ifdef GIT_WINDOWS_NATIVE
> +/* Git for Windows checkout PATH_MAX is reduce to 260.
> + * but if checkout relative long path name, its length too short.
> + * thus, expand length.
> + */
> +#ifdef PATH_MAX
> +#undef PATH_MAX
> +#endif
> +#define PATH_MAX 4096
> +#endif

This looks fine, but I am wary... did you not say that a crash was caused
by this? In that case, we would have a user that accesses the respective
buffer without checking the size and we would still have to fix that bug..

Ciao,
Dscho

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] mingw-multibyte: fix memory acces violation and path length limits.

[Wataru Noguchi]

Hi,

Thanks for comments.

My currently working repository is

https://github.com/wnoguchi/git/tree/hotfix/mingw-multibyte-path-checkout-failure

I have revert commits to 1f10da3.
I'll try failure step.

- gcc optimization level is O2.(fail)
- gcc O0, O1 works fine.


$ gdb git-clone
GNU gdb 6.8
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-mingw32"...
(gdb) r https://github.com/wnoguchi/mingw-checkout-crash.git
Starting program: C:\msysgit\git/git-clone.exe https://github.com/wnoguchi/mingw
-checkout-crash.git
[New thread 800.0xa10]
Error: dll starting at 0x779f0000 not found.
Error: dll starting at 0x75900000 not found.
Error: dll starting at 0x779f0000 not found.
Error: dll starting at 0x778f0000 not found.
[New thread 800.0x92c]
Cloning into 'mingw-checkout-crash'...
Error: dll starting at 0x29f0000 not found.
remote: Counting objects: 8, done.
remote: Compressing objects: 100% (7/7), done.
remote: Total 8 (delta 0), reused 8 (delta 0)
Unpacking objects: 100% (8/8), done.
Checking connectivity... done
[New thread 800.0xea0]

Program received signal SIGSEGV, Segmentation fault.
0x004d5200 in git_check_attr (
     path=0xacc6a0 ""..., num=5, check=0x572440) at attr.c:754
754                     const char *value = check_all_attr[check[i].attr->attr_n
r].value;
(gdb) list
749             int i;
750
751             collect_all_attrs(path);
752
753             for (i = 0; i < num; i++) {
754                     const char *value = check_all_attr[check[i].attr->attr_n
r].value;
755                     if (value == ATTR__UNKNOWN)
756                             value = ATTR__UNSET;
757                     check[i].value = value;
758             }
(gdb) up
#1  0x004a796b in convert_attrs (ca=0x28f950,
     path=0xacc6a0 ""...) at convert.c:740
740             if (!git_check_attr(path, NUM_CONV_ATTRS, ccheck)) {
(gdb) list
735                             ccheck[i].attr = git_attr(conv_attr_name[i]);
736                     user_convert_tail = &user_convert;
737                     git_config(read_convert_config, NULL);
738             }
739
740             if (!git_check_attr(path, NUM_CONV_ATTRS, ccheck)) {
741                     ca->crlf_action = git_path_check_crlf(path, ccheck + 4);

742                     if (ca->crlf_action == CRLF_GUESS)
743                             ca->crlf_action = git_path_check_crlf(path, cche
ck + 0);
744                     ca->ident = git_path_check_ident(path, ccheck + 1);
(gdb) list -
725             int i;
726             static struct git_attr_check ccheck[NUM_CONV_ATTRS];
727
728     //      if (NUM_CONV_ATTRS != 0) {
729     //              ccheck[0].attr = NULL;
730     //              ccheck[0].value = NULL;
731     //      }
732
733             if (!ccheck[0].attr) {
734                     for (i = 0; i < NUM_CONV_ATTRS; i++)
(gdb) p ccheck[0].attr
$1 = (struct git_attr *) 0xa081e38f


Next, change PATH_MAX value to 4096 if MinGW environment.(6cae216)
Works fine.
Is this failure caused by PATH_MAX length too short? or my repository directory depth too deep?
But when optimization disabled(O0) in Makefile , works fine...(bf0acff)
Do you know what's happen?


Thanks.


(2013/09/29 8:18), Johannes Schindelin wrote:
> Hi,
>
> On Sun, 29 Sep 2013, Wataru Noguchi wrote:
>
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -724,6 +724,11 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
>>   {
>>   	int i;
>>   	static struct git_attr_check ccheck[NUM_CONV_ATTRS];
>> +	
>> +	if (NUM_CONV_ATTRS != 0) {
>> +		ccheck[0].attr = NULL;
>> +		ccheck[0].value = NULL;
>> +	}
>
> I wonder whether it would make more sense to use
>
> 	memset(ccheck, 0, sizeof(ccheck))
>
> ? But then, ccheck is static and *should* be initialized to all 0
> according to the C standard. And re-initializing it to NULL would
> invalidate the values that were set earlier.
>
> Also, if NUM_CONV_ATTRS == 0, I would expect
>
>>   	if (!ccheck[0].attr) {
>
> to access an invalid location...
>
>> diff --git a/git-compat-util.h b/git-compat-util.h
>> index a31127f..ba02c69 100644
>> --- a/git-compat-util.h
>> +++ b/git-compat-util.h
>> @@ -237,6 +237,16 @@ extern char *gitbasename(char *);
>>   #ifndef PATH_MAX
>>   #define PATH_MAX 4096
>>   #endif
>> +#ifdef GIT_WINDOWS_NATIVE
>> +/* Git for Windows checkout PATH_MAX is reduce to 260.
>> + * but if checkout relative long path name, its length too short.
>> + * thus, expand length.
>> + */
>> +#ifdef PATH_MAX
>> +#undef PATH_MAX
>> +#endif
>> +#define PATH_MAX 4096
>> +#endif
>
> This looks fine, but I am wary... did you not say that a crash was caused
> by this? In that case, we would have a user that accesses the respective
> buffer without checking the size and we would still have to fix that bug..
>
> Ciao,
> Dscho
>

-- 
=========================================
   Wataru Noguchi
   wnoguchi.0727@gmail.com
   http://wnoguchi.github.io/
=========================================

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [msysGit] [PATCH] mingw-multibyte: fix memory acces violation and path length limits.

[Stefan Beller]

[-- Attachment #1: Type: text/plain, Size: 273 bytes --]

On 09/29/2013 04:56 AM, Wataru Noguchi wrote:
> 
> - gcc optimization level is O2.(fail)
> - gcc O0, O1 works fine.

Maybe you could try to compile with
STACK found at http://css.csail.mit.edu/stack/
That tool is designed to find
Optimization-unstable code.




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH] mingw-multibyte: fix memory acces violation and path length limits.

[René Scharfe]

Am 29.09.2013 04:56, schrieb Wataru Noguchi:
> Hi,
> 
> Thanks for comments.
> 
> My currently working repository is
> 
> https://github.com/wnoguchi/git/tree/hotfix/mingw-multibyte-path-checkout-failure
> 
> I have revert commits to 1f10da3.
> I'll try failure step.
> 
> - gcc optimization level is O2.(fail)
> - gcc O0, O1 works fine.
> 
> 
> $ gdb git-clone
> GNU gdb 6.8
> Copyright (C) 2008 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
> and "show warranty" for details.
> This GDB was configured as "i686-pc-mingw32"...
> (gdb) r https://github.com/wnoguchi/mingw-checkout-crash.git
> Starting program: C:\msysgit\git/git-clone.exe https://github.com/wnoguchi/mingw
> -checkout-crash.git
> [New thread 800.0xa10]
> Error: dll starting at 0x779f0000 not found.
> Error: dll starting at 0x75900000 not found.
> Error: dll starting at 0x779f0000 not found.
> Error: dll starting at 0x778f0000 not found.
> [New thread 800.0x92c]
> Cloning into 'mingw-checkout-crash'...
> Error: dll starting at 0x29f0000 not found.
> remote: Counting objects: 8, done.
> remote: Compressing objects: 100% (7/7), done.
> remote: Total 8 (delta 0), reused 8 (delta 0)
> Unpacking objects: 100% (8/8), done.
> Checking connectivity... done
> [New thread 800.0xea0]
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x004d5200 in git_check_attr (
>       path=0xacc6a0 ""..., num=5, check=0x572440) at attr.c:754
> 754                     const char *value = check_all_attr[check[i].attr->attr_n
> r].value;
> (gdb) list
> 749             int i;
> 750
> 751             collect_all_attrs(path);
> 752
> 753             for (i = 0; i < num; i++) {
> 754                     const char *value = check_all_attr[check[i].attr->attr_n
> r].value;
> 755                     if (value == ATTR__UNKNOWN)
> 756                             value = ATTR__UNSET;
> 757                     check[i].value = value;
> 758             }

I get a different crash on Linux if I set PATH_MAX to 260.  The following
hackish patch prevents it.  Does it help in your case as well?  If it does
then I'll send a nicer (but longer) one.

Thanks,
René


diff --git a/unpack-trees.c b/unpack-trees.c
index 1a61e6f..9bd7dcb 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -961,7 +961,7 @@ static int clear_ce_flags(struct cache_entry **cache, int nr,
 			    int select_mask, int clear_mask,
 			    struct exclude_list *el)
 {
-	char prefix[PATH_MAX];
+	char prefix[4096];
 	return clear_ce_flags_1(cache, nr,
 				prefix, 0,
 				select_mask, clear_mask,

Re: [PATCH] mingw-multibyte: fix memory acces violation and path length limits.

[Erik Faye-Lund]

On Mon, Sep 30, 2013 at 7:00 PM, René Scharfe <l.s.r@web.de> wrote:
> Am 29.09.2013 04:56, schrieb Wataru Noguchi:
>> Hi,
>>
>> Thanks for comments.
>>
>> My currently working repository is
>>
>> https://github.com/wnoguchi/git/tree/hotfix/mingw-multibyte-path-checkout-failure
>>
>> I have revert commits to 1f10da3.
>> I'll try failure step.
>>
>> - gcc optimization level is O2.(fail)
>> - gcc O0, O1 works fine.
>>
>>
>> $ gdb git-clone
>> GNU gdb 6.8
>> Copyright (C) 2008 Free Software Foundation, Inc.
>> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
>> This is free software: you are free to change and redistribute it.
>> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
>> and "show warranty" for details.
>> This GDB was configured as "i686-pc-mingw32"...
>> (gdb) r https://github.com/wnoguchi/mingw-checkout-crash.git
>> Starting program: C:\msysgit\git/git-clone.exe https://github.com/wnoguchi/mingw
>> -checkout-crash.git
>> [New thread 800.0xa10]
>> Error: dll starting at 0x779f0000 not found.
>> Error: dll starting at 0x75900000 not found.
>> Error: dll starting at 0x779f0000 not found.
>> Error: dll starting at 0x778f0000 not found.
>> [New thread 800.0x92c]
>> Cloning into 'mingw-checkout-crash'...
>> Error: dll starting at 0x29f0000 not found.
>> remote: Counting objects: 8, done.
>> remote: Compressing objects: 100% (7/7), done.
>> remote: Total 8 (delta 0), reused 8 (delta 0)
>> Unpacking objects: 100% (8/8), done.
>> Checking connectivity... done
>> [New thread 800.0xea0]
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x004d5200 in git_check_attr (
>>       path=0xacc6a0 ""..., num=5, check=0x572440) at attr.c:754
>> 754                     const char *value = check_all_attr[check[i].attr->attr_n
>> r].value;
>> (gdb) list
>> 749             int i;
>> 750
>> 751             collect_all_attrs(path);
>> 752
>> 753             for (i = 0; i < num; i++) {
>> 754                     const char *value = check_all_attr[check[i].attr->attr_n
>> r].value;
>> 755                     if (value == ATTR__UNKNOWN)
>> 756                             value = ATTR__UNSET;
>> 757                     check[i].value = value;
>> 758             }
>
> I get a different crash on Linux if I set PATH_MAX to 260.  The following
> hackish patch prevents it.

Interesting. It does not seem to help here. Multiple issues in the
same code-path?

>  Does it help in your case as well?  If it does
> then I'll send a nicer (but longer) one.

Sounds like something that's still worth investigating, even if it
does not help.

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] mingw-multibyte: fix memory acces violation and path length limits.

[Wataru Noguchi]

Hi,

Thanks for your patch.

Unfortunately, in my case still crash...

But PATH_MAX length kinds issues interesting.

I'll try investigate a little more.

- PATH_MAX and O2

Thanks.

(2013/10/01 2:00), René Scharfe wrote:
> Am 29.09.2013 04:56, schrieb Wataru Noguchi:
>> Hi,
>>
>> Thanks for comments.
>>
>> My currently working repository is
>>
>> https://github.com/wnoguchi/git/tree/hotfix/mingw-multibyte-path-checkout-failure
>>
>> I have revert commits to 1f10da3.
>> I'll try failure step.
>>
>> - gcc optimization level is O2.(fail)
>> - gcc O0, O1 works fine.
>>
>>
>> $ gdb git-clone
>> GNU gdb 6.8
>> Copyright (C) 2008 Free Software Foundation, Inc.
>> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
>> This is free software: you are free to change and redistribute it.
>> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
>> and "show warranty" for details.
>> This GDB was configured as "i686-pc-mingw32"...
>> (gdb) r https://github.com/wnoguchi/mingw-checkout-crash.git
>> Starting program: C:\msysgit\git/git-clone.exe https://github.com/wnoguchi/mingw
>> -checkout-crash.git
>> [New thread 800.0xa10]
>> Error: dll starting at 0x779f0000 not found.
>> Error: dll starting at 0x75900000 not found.
>> Error: dll starting at 0x779f0000 not found.
>> Error: dll starting at 0x778f0000 not found.
>> [New thread 800.0x92c]
>> Cloning into 'mingw-checkout-crash'...
>> Error: dll starting at 0x29f0000 not found.
>> remote: Counting objects: 8, done.
>> remote: Compressing objects: 100% (7/7), done.
>> remote: Total 8 (delta 0), reused 8 (delta 0)
>> Unpacking objects: 100% (8/8), done.
>> Checking connectivity... done
>> [New thread 800.0xea0]
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x004d5200 in git_check_attr (
>>        path=0xacc6a0 ""..., num=5, check=0x572440) at attr.c:754
>> 754                     const char *value = check_all_attr[check[i].attr->attr_n
>> r].value;
>> (gdb) list
>> 749             int i;
>> 750
>> 751             collect_all_attrs(path);
>> 752
>> 753             for (i = 0; i < num; i++) {
>> 754                     const char *value = check_all_attr[check[i].attr->attr_n
>> r].value;
>> 755                     if (value == ATTR__UNKNOWN)
>> 756                             value = ATTR__UNSET;
>> 757                     check[i].value = value;
>> 758             }
>
> I get a different crash on Linux if I set PATH_MAX to 260.  The following
> hackish patch prevents it.  Does it help in your case as well?  If it does
> then I'll send a nicer (but longer) one.
>
> Thanks,
> René
>
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 1a61e6f..9bd7dcb 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -961,7 +961,7 @@ static int clear_ce_flags(struct cache_entry **cache, int nr,
>   			    int select_mask, int clear_mask,
>   			    struct exclude_list *el)
>   {
> -	char prefix[PATH_MAX];
> +	char prefix[4096];
>   	return clear_ce_flags_1(cache, nr,
>   				prefix, 0,
>   				select_mask, clear_mask,
>
>

-- 
=========================================
   Wataru Noguchi
   wnoguchi.0727@gmail.com
   http://wnoguchi.github.io/
=========================================

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] mingw-multibyte: fix memory acces violation and path length limits.

[Wataru Noguchi]

Hi,

Thanks for your advice.

I see. I'll try following tool for optimization affection.

Thanks.

(2013/09/29 20:01), Stefan Beller wrote:
> On 09/29/2013 04:56 AM, Wataru Noguchi wrote:
>>
>> - gcc optimization level is O2.(fail)
>> - gcc O0, O1 works fine.
>
> Maybe you could try to compile with
> STACK found at http://css.csail.mit.edu/stack/
> That tool is designed to find
> Optimization-unstable code.
>
>
>

-- 
=========================================
   Wataru Noguchi
   wnoguchi.0727@gmail.com
   http://wnoguchi.github.io/
=========================================

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] mingw-multibyte: fix memory acces violation and path length limits.

[Wataru Noguchi]

Hi,

At last, I foundfollowing Makefile optimization suppression works fine in my case.

CFLAGS = -g -O2 -fno-inline-small-functions -Wall

Following optimization option cause crash,

-finline-small-functions


// entry.c:237

int checkout_entry(struct cache_entry *ce,
		   const struct checkout *state, char *topath)
{
//------------------------------------------------------------------
// crash!
	static char path[PATH_MAX + 1];
//------------------------------------------------------------------
// works fine. but bad practice.
	static char path[4096 + 1];
//------------------------------------------------------------------
	struct stat st;
	int len = state->base_dir_len;

	if (topath)
		return write_entry(ce, topath, state, 1);


Thanks.


(2013/10/01 22:35), Wataru Noguchi wrote:
> Hi,
>
> Thanks for your patch.
>
> Unfortunately, in my case still crash...
>
> But PATH_MAX length kinds issues interesting.
>
> I'll try investigate a little more.
>
> - PATH_MAX and O2
>
> Thanks.
>
> (2013/10/01 2:00), René Scharfe wrote:
>> Am 29.09.2013 04:56, schrieb Wataru Noguchi:
>>> Hi,
>>>
>>> Thanks for comments.
>>>
>>> My currently working repository is
>>>
>>> https://github.com/wnoguchi/git/tree/hotfix/mingw-multibyte-path-checkout-failure
>>>
>>> I have revert commits to 1f10da3.
>>> I'll try failure step.
>>>
>>> - gcc optimization level is O2.(fail)
>>> - gcc O0, O1 works fine.
>>>
>>>
>>> $ gdb git-clone
>>> GNU gdb 6.8
>>> Copyright (C) 2008 Free Software Foundation, Inc.
>>> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
>>> This is free software: you are free to change and redistribute it.
>>> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
>>> and "show warranty" for details.
>>> This GDB was configured as "i686-pc-mingw32"...
>>> (gdb) r https://github.com/wnoguchi/mingw-checkout-crash.git
>>> Starting program: C:\msysgit\git/git-clone.exe https://github.com/wnoguchi/mingw
>>> -checkout-crash.git
>>> [New thread 800.0xa10]
>>> Error: dll starting at 0x779f0000 not found.
>>> Error: dll starting at 0x75900000 not found.
>>> Error: dll starting at 0x779f0000 not found.
>>> Error: dll starting at 0x778f0000 not found.
>>> [New thread 800.0x92c]
>>> Cloning into 'mingw-checkout-crash'...
>>> Error: dll starting at 0x29f0000 not found.
>>> remote: Counting objects: 8, done.
>>> remote: Compressing objects: 100% (7/7), done.
>>> remote: Total 8 (delta 0), reused 8 (delta 0)
>>> Unpacking objects: 100% (8/8), done.
>>> Checking connectivity... done
>>> [New thread 800.0xea0]
>>>
>>> Program received signal SIGSEGV, Segmentation fault.
>>> 0x004d5200 in git_check_attr (
>>>        path=0xacc6a0 ""..., num=5, check=0x572440) at attr.c:754
>>> 754                     const char *value = check_all_attr[check[i].attr->attr_n
>>> r].value;
>>> (gdb) list
>>> 749             int i;
>>> 750
>>> 751             collect_all_attrs(path);
>>> 752
>>> 753             for (i = 0; i < num; i++) {
>>> 754                     const char *value = check_all_attr[check[i].attr->attr_n
>>> r].value;
>>> 755                     if (value == ATTR__UNKNOWN)
>>> 756                             value = ATTR__UNSET;
>>> 757                     check[i].value = value;
>>> 758             }
>>
>> I get a different crash on Linux if I set PATH_MAX to 260.  The following
>> hackish patch prevents it.  Does it help in your case as well?  If it does
>> then I'll send a nicer (but longer) one.
>>
>> Thanks,
>> René
>>
>>
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index 1a61e6f..9bd7dcb 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -961,7 +961,7 @@ static int clear_ce_flags(struct cache_entry **cache, int nr,
>>                   int select_mask, int clear_mask,
>>                   struct exclude_list *el)
>>   {
>> -    char prefix[PATH_MAX];
>> +    char prefix[4096];
>>       return clear_ce_flags_1(cache, nr,
>>                   prefix, 0,
>>                   select_mask, clear_mask,
>>
>>
>


-- 
================================
   Wataru Noguchi
   wnoguchi.0727@gmail.com
   http://wnoguchi.github.io/
================================

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] mingw-multibyte: fix memory acces violation and path length limits.

[Antoine Pelisse]

I've not followed the thread so much but, in that
entry.c::checkout_entry,() we do:

memcpy(path, state->base_dir, len);
strcpy(path + len, ce->name);

which can of course result in memory violation if PATH is not long enough.

On Thu, Oct 3, 2013 at 12:26 AM, Wataru Noguchi <wnoguchi.0727@gmail.com> wrote:
> Hi,
>
> At last, I foundfollowing Makefile optimization suppression works fine in my
> case.
>
> CFLAGS = -g -O2 -fno-inline-small-functions -Wall
>
> Following optimization option cause crash,
>
> -finline-small-functions

Re: [PATCH] mingw-multibyte: fix memory acces violation and path length limits.

[Erik Faye-Lund]

On Thu, Oct 3, 2013 at 7:25 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
> I've not followed the thread so much but, in that
> entry.c::checkout_entry,() we do:
>
> memcpy(path, state->base_dir, len);
> strcpy(path + len, ce->name);
>
> which can of course result in memory violation if PATH is not long enough.
>

...aaand you're spot on. The following patch illustrates it:

$ /git/git-clone.exe mingw-checkout-crash.git
Cloning into 'mingw-checkout-crash'...
done.
fatal: argh, this won't work!
warning: Clone succeeded, but checkout failed.
You can inspect what was checked out with 'git status'
and retry the checkout with 'git checkout -f HEAD'

---

diff --git a/entry.c b/entry.c
index acc892f..505638e 100644
--- a/entry.c
+++ b/entry.c
@@ -244,6 +244,9 @@ int checkout_entry(struct cache_entry *ce,
  if (topath)
  return write_entry(ce, topath, state, 1);

+ if (len > PATH_MAX || len + strlen(ce->name) > PATH_MAX)
+ die("argh, this won't work!");
+
  memcpy(path, state->base_dir, len);
  strcpy(path + len, ce->name);
  len += ce_namelen(ce);


> On Thu, Oct 3, 2013 at 12:26 AM, Wataru Noguchi <wnoguchi.0727@gmail.com> wrote:
>> Hi,
>>
>> At last, I foundfollowing Makefile optimization suppression works fine in my
>> case.
>>
>> CFLAGS = -g -O2 -fno-inline-small-functions -Wall
>>
>> Following optimization option cause crash,
>>
>> -finline-small-functions
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] mingw-multibyte: fix memory acces violation and path length limits.

[Wataru Noguchi]

Hi,

I put following printf logs.

int checkout_entry(struct cache_entry *ce,
		   const struct checkout *state, char *topath)
{
	static char path[PATH_MAX + 1];
	struct stat st;
	int len = state->base_dir_len;

	if (topath)
		return write_entry(ce, topath, state, 1);

	memcpy(path, state->base_dir, len);
	fprintf(stderr, "path: %s\n", path);
	fprintf(stderr, "len: %d\n", len);
	strcpy(path + len, ce->name);
	len += ce_namelen(ce);
	fprintf(stderr, "path: %s\n", path);
	fprintf(stderr, "len: %d\n", len);
	fprintf(stderr, "path_max: %d\n", PATH_MAX);
	

--------------------------------------------------------------------------------------


crash result

wnoguchi@WIN-72R9044R72V /usr/tmp (master)
$ git clone https://github.com/wnoguchi/mingw-checkout-crash.git a2
Cloning into 'a2'...
remote: Counting objects: 8, done.
remote: Compressing objects: 100% (7/7), done.
remote: Total 8 (delta 0), reused 8 (delta 0)
Unpacking objects: 100% (8/8), done.
Checking connectivity... done
path:
len: 0
path: dummy 1-long-long-long-dirname/dummy 2-long-long
-long-dirname/dummy 3-long-long-long-dirname/dummy 4-l
ong-long-long-dirname/dummy 5-long-long-long-dirname/aaaaaaaaaaaa.txt
len: 302
path_max: 259

crash!!

--------------------------------------------------------------------------------------

build with

CFLAGS = -g -O2 -fno-inline-small-functions -Wall


wnoguchi@WIN-72R9044R72V /usr/tmp (master)
$ git clone https://github.com/wnoguchi/mingw-checkout-crash.git a3
Cloning into 'a3'...
remote: Counting objects: 8, done.
remote: Compressing objects: 100% (7/7), done.
remote: Total 8 (delta 0), reused 8 (delta 0)
Unpacking objects: 100% (8/8), done.
Checking connectivity... done
path:
len: 0
path: dummy 1-long-long-long-dirname/dummy 2-long-long
-long-dirname/dummy 3-long-long-long-dirname/dummy 4-l
ong-long-long-dirname/dummy 5-long-long-long-dirname/aaaaaaaaaaaa.txt
len: 302
path_max: 259

Warning: Your console font probably doesn't support Unicode. If you experience s
trange characters in the output, consider switching to a TrueType font such as L
ucida Console!

works fine.

------------------------------------------------------------------------------------

this result means actual path byte length over run path buffer?

	static char path[PATH_MAX + 1];

hmmm...

I'm not sure why -fno-inline-small-functions works.


(2013/10/04 2:36), Erik Faye-Lund wrote:
> On Thu, Oct 3, 2013 at 7:25 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
>> I've not followed the thread so much but, in that
>> entry.c::checkout_entry,() we do:
>>
>> memcpy(path, state->base_dir, len);
>> strcpy(path + len, ce->name);
>>
>> which can of course result in memory violation if PATH is not long enough.
>>
>
> ...aaand you're spot on. The following patch illustrates it:
>
> $ /git/git-clone.exe mingw-checkout-crash.git
> Cloning into 'mingw-checkout-crash'...
> done.
> fatal: argh, this won't work!
> warning: Clone succeeded, but checkout failed.
> You can inspect what was checked out with 'git status'
> and retry the checkout with 'git checkout -f HEAD'
>
> ---
>
> diff --git a/entry.c b/entry.c
> index acc892f..505638e 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -244,6 +244,9 @@ int checkout_entry(struct cache_entry *ce,
>    if (topath)
>    return write_entry(ce, topath, state, 1);
>
> + if (len > PATH_MAX || len + strlen(ce->name) > PATH_MAX)
> + die("argh, this won't work!");
> +
>    memcpy(path, state->base_dir, len);
>    strcpy(path + len, ce->name);
>    len += ce_namelen(ce);
>
>
>> On Thu, Oct 3, 2013 at 12:26 AM, Wataru Noguchi <wnoguchi.0727@gmail.com> wrote:
>>> Hi,
>>>
>>> At last, I foundfollowing Makefile optimization suppression works fine in my
>>> case.
>>>
>>> CFLAGS = -g -O2 -fno-inline-small-functions -Wall
>>>
>>> Following optimization option cause crash,
>>>
>>> -finline-small-functions
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
================================
   Wataru Noguchi
   wnoguchi.0727@gmail.com
   http://wnoguchi.github.io/
================================

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[PATCH] Prevent buffer overflows when path is too big

[Antoine Pelisse]

Currently, most buffers created with PATH_MAX length, are not checked
when being written, and can overflow if PATH_MAX is not big enough to
hold the path.

Fix that by using strlcpy() where strcpy() was used, and also run some
extra checks when copy is done with memcpy().

Reported-by: Wataru Noguchi <wnoguchi.0727@gmail.com>
Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
 abspath.c        | 10 +++++++---
 diffcore-order.c |  2 +-
 entry.c          | 14 ++++++++++----
 unpack-trees.c   |  2 ++
 4 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/abspath.c b/abspath.c
index 64adbe2..0e60ba4 100644
--- a/abspath.c
+++ b/abspath.c
@@ -216,11 +216,15 @@ const char *absolute_path(const char *path)
 const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
 {
 	static char path[PATH_MAX];
+
+	if (pfx_len > PATH_MAX)
+		die("Too long prefix path: %s", pfx);
+
 #ifndef GIT_WINDOWS_NATIVE
 	if (!pfx_len || is_absolute_path(arg))
 		return arg;
 	memcpy(path, pfx, pfx_len);
-	strcpy(path + pfx_len, arg);
+	strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len);
 #else
 	char *p;
 	/* don't add prefix to absolute paths, but still replace '\' by '/' */
@@ -228,8 +232,8 @@ const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
 		pfx_len = 0;
 	else if (pfx_len)
 		memcpy(path, pfx, pfx_len);
-	strcpy(path + pfx_len, arg);
-	for (p = path + pfx_len; *p; p++)
+	strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len);
+	for (p = path + pfx_len; p < path + PATH_MAX && *p; p++)
 		if (*p == '\\')
 			*p = '/';
 #endif
diff --git a/diffcore-order.c b/diffcore-order.c
index 23e9385..f083c82 100644
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -76,7 +76,7 @@ static int match_order(const char *path)
 	char p[PATH_MAX];
 
 	for (i = 0; i < order_cnt; i++) {
-		strcpy(p, path);
+		strlcpy(p, path, PATH_MAX);
 		while (p[0]) {
 			char *cp;
 			if (!fnmatch(order[i], p, 0))
diff --git a/entry.c b/entry.c
index acc892f..39bee42 100644
--- a/entry.c
+++ b/entry.c
@@ -50,17 +50,20 @@ static void remove_subtree(const char *path)
 	struct dirent *de;
 	char pathbuf[PATH_MAX];
 	char *name;
+	size_t pathlen;
 
 	if (!dir)
 		die_errno("cannot opendir '%s'", path);
-	strcpy(pathbuf, path);
-	name = pathbuf + strlen(path);
+	strlcpy(pathbuf, path, PATH_MAX);
+	pathlen = strlen(path);
+	name = pathbuf + pathlen;
 	*name++ = '/';
+	pathlen++;
 	while ((de = readdir(dir)) != NULL) {
 		struct stat st;
 		if (is_dot_or_dotdot(de->d_name))
 			continue;
-		strcpy(name, de->d_name);
+		strlcpy(name, de->d_name, PATH_MAX - pathlen);
 		if (lstat(pathbuf, &st))
 			die_errno("cannot lstat '%s'", pathbuf);
 		if (S_ISDIR(st.st_mode))
@@ -244,8 +247,11 @@ int checkout_entry(struct cache_entry *ce,
 	if (topath)
 		return write_entry(ce, topath, state, 1);
 
+	if (len > PATH_MAX + 1)
+		die("Too long path: %s", state->base_dir);
+
 	memcpy(path, state->base_dir, len);
-	strcpy(path + len, ce->name);
+	strlcpy(path + len, ce->name, PATH_MAX + 1 - len);
 	len += ce_namelen(ce);
 
 	if (!check_path(path, len, &st, state->base_dir_len)) {
diff --git a/unpack-trees.c b/unpack-trees.c
index 1a61e6f..85473b1 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -918,6 +918,8 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,
 			int processed;
 
 			len = slash - name;
+			if (len + prefix_len >= PATH_MAX)
+				len = PATH_MAX - prefix_len - 1;
 			memcpy(prefix + prefix_len, name, len);
 
 			/*
-- 
1.8.4.1.507.g9768648.dirty

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] Prevent buffer overflows when path is too big

[Torsten Bögershausen]

(may be s/path is too big/path is too long/ ?)

On 19.10.13 12:52, Antoine Pelisse wrote:
> Currently, most buffers created with PATH_MAX length, are not checked
> when being written, and can overflow if PATH_MAX is not big enough to
> hold the path.
> 
> Fix that by using strlcpy() where strcpy() was used, and also run some
> extra checks when copy is done with memcpy().
> 
> Reported-by: Wataru Noguchi <wnoguchi.0727@gmail.com>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
>  abspath.c        | 10 +++++++---
>  diffcore-order.c |  2 +-
>  entry.c          | 14 ++++++++++----
>  unpack-trees.c   |  2 ++
>  4 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/abspath.c b/abspath.c
> index 64adbe2..0e60ba4 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -216,11 +216,15 @@ const char *absolute_path(const char *path)
>  const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
>  {
>  	static char path[PATH_MAX];
> +
> +	if (pfx_len > PATH_MAX)
I think this should be 
if (pfx_len > PATH_MAX-1) /* Keep 1 char for '\0'
> +		die("Too long prefix path: %s", pfx);
> +
>  #ifndef GIT_WINDOWS_NATIVE
>  	if (!pfx_len || is_absolute_path(arg))
>  		return arg;
>  	memcpy(path, pfx, pfx_len);
> -	strcpy(path + pfx_len, arg);
> +	strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len);

I'm not sure how to handle overlong path in general, there are several ways:
a) Silently overwrite memory (with help of memcpy() and/or strcpy()
b) Silently shorten the path using strlcpy() instead of strcpy()
c) Avoid the overwriting and call die().
d) Prepare a longer buffer using xmalloc()

Today we do a), this is not a good thing and the worst choice.


A little side note:
  It would be good to have test cases for either b), c) or d).

  As PATH_MAX is OS dependend, we need both a main program written in c
  and a test case written in t/txxxx.sh.
  Some existing code can be used for inspiration, e.g. 
  test-wildmatch.c in combination with t/t3070-wildmatch.sh
  This willl allow us to reproduce the error, and define how git should behave.

End of the side note, let's look closer at the suggested patch, implementing b)

Silently shortening an overlong path like
"/foo/bar/baz" could result something like

"/foo/bar/ba" /* That filename may be part of the repo too */
or
"/foo/bar/" /*  This is a directory, not a file name */

In either case the end user has no idea why git choose another file name.
And this could be hard to debug.
After a couple of hours she/he may send a message asking for help to the mailing list,
and we end up in more people doing debugging.

c) Is much easier to debug:
  Git can not handle this situation, and we print out the parameters in die()

I would prefer c) over b), make clear that git can't handle that situation.

d) Would mean some more re-factoring: Check all callers to prefix_filename().
Some of them call xstrdup() after prefix_filename(), which mean that we could
change prefix_filename() to always return new string which is long enough via xmalloc(),
and not a static buffer.

So we come to the next point (and this is my personal experience,
so please don't get me wrong):
how much time can you spend on this?

If the answer is kind of "very little", I would go for c)
  Avoid the silent memory corruption, and say to the user "we can not handle this"

If the answer is kind of "little", I would go for c) and a test program,
  covering all the different code path in abspath()
  (WHich may deserve a refactoring as well, since the code for GIT_WINDOWS_NATIVE
  is very similar to the non-GIT_WINDOWS_NATIVE)

If the answer is kind of "more than little", a different strategie may be better:
  Start sending a patch for c)
  I think we have enough volunteers here for a review, so we can life without the test code.
  On top of that, some volunteer can develop d).
  
So far I have only looked at abspath(), and your patch touches more places.
I think more and more that calling die()
with all information included why we call die() is a good starting point.

It will allow the users to see what is going on.
May be the repo can be re-arranged to use shorter path names than what we can handle.
[snip]
 
/Torsten

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big

[Ondřej Bílka]

On Sun, Oct 20, 2013 at 07:47:06AM +0200, Torsten Bögershausen wrote:
> (may be s/path is too big/path is too long/ ?)
> 
> On 19.10.13 12:52, Antoine Pelisse wrote:
> > Currently, most buffers created with PATH_MAX length, are not checked
> > when being written, and can overflow if PATH_MAX is not big enough to
> > hold the path.
> > 
> > Fix that by using strlcpy() where strcpy() was used, and also run some
> > extra checks when copy is done with memcpy().
> > 
> > Reported-by: Wataru Noguchi <wnoguchi.0727@gmail.com>
> > Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> > ---
> > diff --git a/abspath.c b/abspath.c
> > index 64adbe2..0e60ba4 100644
> > --- a/abspath.c
> > +++ b/abspath.c
> > @@ -216,11 +216,15 @@ const char *absolute_path(const char *path)
> >  const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
> >  {
> >  	static char path[PATH_MAX];

Why do you need static there?
> > +
> > +	if (pfx_len > PATH_MAX)
> I think this should be 
> if (pfx_len > PATH_MAX-1) /* Keep 1 char for '\0'
> > +		die("Too long prefix path: %s", pfx);
> > +
> >  #ifndef GIT_WINDOWS_NATIVE
> >  	if (!pfx_len || is_absolute_path(arg))
> >  		return arg;
> >  	memcpy(path, pfx, pfx_len);
> > -	strcpy(path + pfx_len, arg);
> > +	strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len);
> 
> I'm not sure how to handle overlong path in general, there are several ways:
> a) Silently overwrite memory (with help of memcpy() and/or strcpy()
> b) Silently shorten the path using strlcpy() instead of strcpy()
> c) Avoid the overwriting and call die().
> d) Prepare a longer buffer using xmalloc()
> 
There is also
e) modify allocation to place write protected page after buffer end.

Re: [PATCH] Prevent buffer overflows when path is too big

[Torsten Bögershausen]

On 20.10.13 08:05, Ondřej Bílka wrote:
> On Sun, Oct 20, 2013 at 07:47:06AM +0200, Torsten Bögershausen wrote:
>> (may be s/path is too big/path is too long/ ?)
>>
>> On 19.10.13 12:52, Antoine Pelisse wrote:
>>> Currently, most buffers created with PATH_MAX length, are not checked
>>> when being written, and can overflow if PATH_MAX is not big enough to
>>> hold the path.
>>>
>>> Fix that by using strlcpy() where strcpy() was used, and also run some
>>> extra checks when copy is done with memcpy().
>>>
>>> Reported-by: Wataru Noguchi <wnoguchi.0727@gmail.com>
>>> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
>>> ---
>>> diff --git a/abspath.c b/abspath.c
>>> index 64adbe2..0e60ba4 100644
>>> --- a/abspath.c
>>> +++ b/abspath.c
>>> @@ -216,11 +216,15 @@ const char *absolute_path(const char *path)
>>>  const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
>>>  {
>>>  	static char path[PATH_MAX];
> 
> Why do you need static there?
Good point.
get_pathname() from path.c may be better.

>>> +
>>> +	if (pfx_len > PATH_MAX)
>> I think this should be 
>> if (pfx_len > PATH_MAX-1) /* Keep 1 char for '\0'
>>> +		die("Too long prefix path: %s", pfx);
>>> +
>>>  #ifndef GIT_WINDOWS_NATIVE
>>>  	if (!pfx_len || is_absolute_path(arg))
>>>  		return arg;
>>>  	memcpy(path, pfx, pfx_len);
>>> -	strcpy(path + pfx_len, arg);
>>> +	strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len);
>>
>> I'm not sure how to handle overlong path in general, there are several ways:
>> a) Silently overwrite memory (with help of memcpy() and/or strcpy()
>> b) Silently shorten the path using strlcpy() instead of strcpy()
>> c) Avoid the overwriting and call die().
>> d) Prepare a longer buffer using xmalloc()
>>
> There is also
> e) modify allocation to place write protected page after buffer end.

Yes, I think this is what electric fence, DUMA or valgrind do:

http://sourceforge.jp/projects/freshmeat_efence/
http://duma.sourceforge.net/
http://valgrind.sourceforge.net/

Theses are very good tools for developers, finding memory corruption
(or other bugs like using uninitialized memory).

One of the motivation I asked for test cases is that a git developer can
run these test cases under valgrind and can verify that we are never out of range.

For an end user a git "crash" caused by trying to write to a write protected page
is better than silently corrupting memory.

And a range check, followed by die(), is even easier to debug.
For an end user.
/Torsten

















-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big

[Ondřej Bílka]

On Sun, Oct 20, 2013 at 08:27:13AM +0200, Torsten Bögershausen wrote:
> Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on
> 	popelka.ms.mff.cuni.cz
> Status: O
> Content-Length: 2690
> Lines: 89
> 
> On 20.10.13 08:05, Ondřej Bílka wrote:
> > On Sun, Oct 20, 2013 at 07:47:06AM +0200, Torsten Bögershausen wrote:
> >> (may be s/path is too big/path is too long/ ?)
> >>
> >> On 19.10.13 12:52, Antoine Pelisse wrote:
> >>> Currently, most buffers created with PATH_MAX length, are not checked
> >>> when being written, and can overflow if PATH_MAX is not big enough to
> >>> hold the path.
> >>>
> >>> Fix that by using strlcpy() where strcpy() was used, and also run some
> >>> extra checks when copy is done with memcpy().
> >>>
> >>> Reported-by: Wataru Noguchi <wnoguchi.0727@gmail.com>
> >>> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> >>> ---
> >>> diff --git a/abspath.c b/abspath.c
> >>> index 64adbe2..0e60ba4 100644
> >>> --- a/abspath.c
> >>> +++ b/abspath.c
> >>> @@ -216,11 +216,15 @@ const char *absolute_path(const char *path)
> >>>  const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
> >>>  {
> >>>  	static char path[PATH_MAX];
> > 
> > Why do you need static there?
> Good point.
> get_pathname() from path.c may be better.
> 
> >>> +
> >>> +	if (pfx_len > PATH_MAX)
> >> I think this should be 
> >> if (pfx_len > PATH_MAX-1) /* Keep 1 char for '\0'
> >>> +		die("Too long prefix path: %s", pfx);
> >>> +
> >>>  #ifndef GIT_WINDOWS_NATIVE
> >>>  	if (!pfx_len || is_absolute_path(arg))
> >>>  		return arg;
> >>>  	memcpy(path, pfx, pfx_len);
> >>> -	strcpy(path + pfx_len, arg);
> >>> +	strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len);
> >>
> >> I'm not sure how to handle overlong path in general, there are several ways:
> >> a) Silently overwrite memory (with help of memcpy() and/or strcpy()
> >> b) Silently shorten the path using strlcpy() instead of strcpy()
> >> c) Avoid the overwriting and call die().
> >> d) Prepare a longer buffer using xmalloc()
> >>
> > There is also
> > e) modify allocation to place write protected page after buffer end.
> 
> Yes, I think this is what electric fence, DUMA or valgrind do:
> 
You need to be selective which buffers are important.

> http://sourceforge.jp/projects/freshmeat_efence/
> http://duma.sourceforge.net/

These are toys, this comes with fact that they need a 8kb of space for
each 8byte malloc. Just run a git diff and differences are huge.

$ /usr/bin/time git diff HEAD^ > x
0.06user 0.01system 0:00.07elapsed 97%CPU (0avgtext+0avgdata
19172maxresident)k
0inputs+8outputs (0major+5591minor)pagefaults 0swaps
$ LD_PRELOAD=libefence.so /usr/bin/time git diff HEAD^ > x

  Electric Fence 2.2 Copyright (C) 1987-1999 Bruce Perens
<bruce@perens.com>
3.49user 0.94system 0:04.45elapsed 99%CPU (0avgtext+0avgdata
91920maxresident)k
0inputs+8outputs (0major+118069minor)pagefaults 0swaps

> http://valgrind.sourceforge.net/
> 
> Theses are very good tools for developers, finding memory corruption
> (or other bugs like using uninitialized memory).
> 
> One of the motivation I asked for test cases is that a git developer can
> run these test cases under valgrind and can verify that we are never out of range.
> 
> For an end user a git "crash" caused by trying to write to a write protected page
> is better than silently corrupting memory.
> 
> And a range check, followed by die(), is even easier to debug.
> For an end user.
> /Torsten
> 

Re: [PATCH] Prevent buffer overflows when path is too big

[Duy Nguyen]

On Sun, Oct 20, 2013 at 12:47 PM, Torsten Bögershausen <tboegi@web.de> wrote:
> I'm not sure how to handle overlong path in general, there are several ways:
> a) Silently overwrite memory (with help of memcpy() and/or strcpy()
> b) Silently shorten the path using strlcpy() instead of strcpy()
> c) Avoid the overwriting and call die().
> d) Prepare a longer buffer using xmalloc()

d+) Use strbuf
-- 
Duy

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] Prevent buffer overflows when path is too big

[Antoine Pelisse]

My main motive was to not *stop* the process when a long path is met.
Because somebody created a repository on Linux with a long file-name
doesn't mean you should not be able to clone it *at all* on Windows.

On Sun, Oct 20, 2013 at 12:33 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Oct 20, 2013 at 12:47 PM, Torsten Bögershausen <tboegi@web.de> wrote:
>> I'm not sure how to handle overlong path in general, there are several ways:
>> a) Silently overwrite memory (with help of memcpy() and/or strcpy()

This one stop the process, as the application crashes :-)

>> b) Silently shorten the path using strlcpy() instead of strcpy()

I was expecting this solution to fail later in a non-blocking way
(e.g. "Can't checkout file $truncated_path, continuing with other
files"). Maybe it would be better to look at each specific call site
and see if there is a way to report a problem ('return error("Can't
checkout %s: path too long")')

>> c) Avoid the overwriting and call die().

This one also stops the process, with an error (of course that's
better than point a)

>> d) Prepare a longer buffer using xmalloc()
> d+) Use strbuf

This of course looks like the best solution, but I believe PATH_MAX
exists for a reason, and maybe we can't simply ignore that value ?

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] Prevent buffer overflows when path is too big

[Duy Nguyen]

On Mon, Oct 21, 2013 at 12:57 AM, Antoine Pelisse <apelisse@gmail.com> wrote:
> My main motive was to not *stop* the process when a long path is met.
> Because somebody created a repository on Linux with a long file-name
> doesn't mean you should not be able to clone it *at all* on Windows.

That should be handled at the Windows compatibility layer if Windows
cannot handle long paths as Linux (or maybe at higher level to skip
checking out those paths). For PATH_MAX value, see [1].

[1] http://thread.gmane.org/gmane.comp.version-control.git/169012/focus=169310
-- 
Duy

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] Prevent buffer overflows when path is too big

[Johannes Sixt]

Am 21.10.2013 03:31, schrieb Duy Nguyen:
> On Mon, Oct 21, 2013 at 12:57 AM, Antoine Pelisse <apelisse@gmail.com> wrote:
>> My main motive was to not *stop* the process when a long path is met.
>> Because somebody created a repository on Linux with a long file-name
>> doesn't mean you should not be able to clone it *at all* on Windows.
> 
> That should be handled at the Windows compatibility layer if Windows
> cannot handle long paths as Linux

Naah... PATH_MAX is a silly, arbitrary limit. The Git data model does
not forbid paths longer than PATH_MAX, be it 4096 or 260. A generic
solution is needed.

> (or maybe at higher level to skip
> checking out those paths).

More like this, yeah.

-- Hannes

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] Prevent buffer overflows when path is too big

[Erik Faye-Lund]

On Mon, Oct 21, 2013 at 9:02 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 21.10.2013 03:31, schrieb Duy Nguyen:
>> On Mon, Oct 21, 2013 at 12:57 AM, Antoine Pelisse <apelisse@gmail.com> wrote:
>>> My main motive was to not *stop* the process when a long path is met.
>>> Because somebody created a repository on Linux with a long file-name
>>> doesn't mean you should not be able to clone it *at all* on Windows.
>>
>> That should be handled at the Windows compatibility layer if Windows
>> cannot handle long paths as Linux
>
> Naah... PATH_MAX is a silly, arbitrary limit. The Git data model does
> not forbid paths longer than PATH_MAX, be it 4096 or 260. A generic
> solution is needed.
>
>> (or maybe at higher level to skip
>> checking out those paths).
>
> More like this, yeah.

I would argue that this is probably even a bug on Linux, only harder
(if not impossible) to trigger by accident as there's probably no
git-client that will generate such trees. But a "malicious" client
might.

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] Prevent buffer overflows when path is too big

[Jeff King]

On Mon, Oct 21, 2013 at 09:07:26PM +0200, Erik Faye-Lund wrote:

> I would argue that this is probably even a bug on Linux, only harder
> (if not impossible) to trigger by accident as there's probably no
> git-client that will generate such trees. But a "malicious" client
> might.

I've just been poking through the impacts of these overflows, for that
exact reason. I don't think any of them are easily triggerable by
somebody sending you a malicious tree (e.g., the `remove_subtree` one
only triggers when we have seen that tree in the filesystem, so it must
be limited to `PATH_MAX`). Some of them are triggerable if you use
particular options (e.g., the one in `match_order` is easy to trigger if
you use `diff -O`).

Still, they should all be fixed, even for Linux. I shouldn't have to
trace the provenance of the data back through 10 functions just to find
out a buffer overflow isn't easily exploitable. :)

-Peff

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] Prevent buffer overflows when path is too big

[Jeff King]

On Mon, Oct 21, 2013 at 03:14:39PM -0400, Jeff King wrote:

> On Mon, Oct 21, 2013 at 09:07:26PM +0200, Erik Faye-Lund wrote:
> 
> > I would argue that this is probably even a bug on Linux, only harder
> > (if not impossible) to trigger by accident as there's probably no
> > git-client that will generate such trees. But a "malicious" client
> > might.
> 
> I've just been poking through the impacts of these overflows, for that
> exact reason. I don't think any of them are easily triggerable by
> somebody sending you a malicious tree (e.g., the `remove_subtree` one
> only triggers when we have seen that tree in the filesystem, so it must
> be limited to `PATH_MAX`). Some of them are triggerable if you use
> particular options (e.g., the one in `match_order` is easy to trigger if
> you use `diff -O`).

Actually, I take that back. The one in checkout_entry is quite easy to
trigger if the victim checks out your tree. The rest are much harder,
though.

-Peff

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[PATCH 1/2] entry.c: convert checkout_entry to use strbuf

[Nguyễn Thái Ngọc Duy]

The old code does not do boundary check so any paths longer than
PATH_MAX can cause buffer overflow. Replace it with strbuf to handle
paths of arbitrary length.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 To get this topic going again. These two patches kill PATH_MAX in
 entry.c and builtin/checkout-index.c

 entry.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/entry.c b/entry.c
index acc892f..d955af5 100644
--- a/entry.c
+++ b/entry.c
@@ -237,16 +237,18 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)
 int checkout_entry(struct cache_entry *ce,
 		   const struct checkout *state, char *topath)
 {
-	static char path[PATH_MAX + 1];
+	static struct strbuf path_buf = STRBUF_INIT;
+	char *path;
 	struct stat st;
-	int len = state->base_dir_len;
+	int len;
 
 	if (topath)
 		return write_entry(ce, topath, state, 1);
 
-	memcpy(path, state->base_dir, len);
-	strcpy(path + len, ce->name);
-	len += ce_namelen(ce);
+	strbuf_reset(&path_buf);
+	strbuf_addf(&path_buf, "%.*s%s", state->base_dir_len, state->base_dir, ce->name);
+	path = path_buf.buf;
+	len = path_buf.len;
 
 	if (!check_path(path, len, &st, state->base_dir_len)) {
 		unsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE);
-- 
1.8.2.83.gc99314b

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[PATCH 2/2] entry.c: convert write_entry to use strbuf

[Nguyễn Thái Ngọc Duy]

The strcpy call in open_output_fd() implies that the output buffer
must be at least 25 chars long. And it's true. The only caller that
can trigger that code is checkout-index, which has the buffer of
PATH_MAX chars (and any systems that have PATH_MAX shorter than 25
chars are just insane).

But in order to say that, one has to walk through a dozen of
functions. Just convert it to strbuf to avoid the constraint and
confusion.

Although my original motivation was simpler than that. I just wanted
to change "char *path" to "const char *path" in checkout_entry() to
make sure no funny business regarding "path" in that function.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout-index.c | 19 ++++++++++++-------
 cache.h                  |  2 +-
 entry.c                  | 29 ++++++++++++++++-------------
 3 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 69e167b..6d88c0c 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -14,7 +14,12 @@
 static int line_termination = '\n';
 static int checkout_stage; /* default to checkout stage0 */
 static int to_tempfile;
-static char topath[4][PATH_MAX + 1];
+static struct strbuf topath[4] = {
+	STRBUF_INIT,
+	STRBUF_INIT,
+	STRBUF_INIT,
+	STRBUF_INIT
+};
 
 static struct checkout state;
 
@@ -26,19 +31,19 @@ static void write_tempfile_record(const char *name, int prefix_length)
 		for (i = 1; i < 4; i++) {
 			if (i > 1)
 				putchar(' ');
-			if (topath[i][0])
-				fputs(topath[i], stdout);
+			if (topath[i].len)
+				fputs(topath[i].buf, stdout);
 			else
 				putchar('.');
 		}
 	} else
-		fputs(topath[checkout_stage], stdout);
+		fputs(topath[checkout_stage].buf, stdout);
 
 	putchar('\t');
 	write_name_quoted(name + prefix_length, stdout, line_termination);
 
 	for (i = 0; i < 4; i++) {
-		topath[i][0] = 0;
+		strbuf_reset(&topath[i]);
 	}
 }
 
@@ -65,7 +70,7 @@ static int checkout_file(const char *name, int prefix_length)
 			continue;
 		did_checkout = 1;
 		if (checkout_entry(ce, &state,
-		    to_tempfile ? topath[ce_stage(ce)] : NULL) < 0)
+		    to_tempfile ? &topath[ce_stage(ce)] : NULL) < 0)
 			errs++;
 	}
 
@@ -109,7 +114,7 @@ static void checkout_all(const char *prefix, int prefix_length)
 				write_tempfile_record(last_ce->name, prefix_length);
 		}
 		if (checkout_entry(ce, &state,
-		    to_tempfile ? topath[ce_stage(ce)] : NULL) < 0)
+		    to_tempfile ? &topath[ce_stage(ce)] : NULL) < 0)
 			errs++;
 		last_ce = ce;
 	}
diff --git a/cache.h b/cache.h
index 51d6602..276182f 100644
--- a/cache.h
+++ b/cache.h
@@ -962,7 +962,7 @@ struct checkout {
 		 refresh_cache:1;
 };
 
-extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);
+extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, struct strbuf *topath);
 
 struct cache_def {
 	char path[PATH_MAX + 1];
diff --git a/entry.c b/entry.c
index d955af5..a76942d 100644
--- a/entry.c
+++ b/entry.c
@@ -92,15 +92,15 @@ static void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)
 	return NULL;
 }
 
-static int open_output_fd(char *path, const struct cache_entry *ce, int to_tempfile)
+static int open_output_fd(struct strbuf *path, const struct cache_entry *ce, int to_tempfile)
 {
 	int symlink = (ce->ce_mode & S_IFMT) != S_IFREG;
 	if (to_tempfile) {
-		strcpy(path, symlink
-		       ? ".merge_link_XXXXXX" : ".merge_file_XXXXXX");
-		return mkstemp(path);
+		strbuf_reset(path);
+		strbuf_addstr(path, symlink ? ".merge_link_XXXXXX" : ".merge_file_XXXXXX");
+		return mkstemp(path->buf);
 	} else {
-		return create_file(path, !symlink ? ce->ce_mode : 0666);
+		return create_file(path->buf, !symlink ? ce->ce_mode : 0666);
 	}
 }
 
@@ -115,7 +115,7 @@ static int fstat_output(int fd, const struct checkout *state, struct stat *st)
 	return 0;
 }
 
-static int streaming_write_entry(const struct cache_entry *ce, char *path,
+static int streaming_write_entry(const struct cache_entry *ce, struct strbuf *path,
 				 struct stream_filter *filter,
 				 const struct checkout *state, int to_tempfile,
 				 int *fstat_done, struct stat *statbuf)
@@ -132,12 +132,12 @@ static int streaming_write_entry(const struct cache_entry *ce, char *path,
 	result |= close(fd);
 
 	if (result)
-		unlink(path);
+		unlink(path->buf);
 	return result;
 }
 
 static int write_entry(struct cache_entry *ce,
-		       char *path, const struct checkout *state, int to_tempfile)
+		       struct strbuf *path_buf, const struct checkout *state, int to_tempfile)
 {
 	unsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;
 	int fd, ret, fstat_done = 0;
@@ -146,15 +146,17 @@ static int write_entry(struct cache_entry *ce,
 	unsigned long size;
 	size_t wrote, newsize = 0;
 	struct stat st;
+	const char *path;
 
 	if (ce_mode_s_ifmt == S_IFREG) {
 		struct stream_filter *filter = get_stream_filter(ce->name, ce->sha1);
 		if (filter &&
-		    !streaming_write_entry(ce, path, filter,
+		    !streaming_write_entry(ce, path_buf, filter,
 					   state, to_tempfile,
 					   &fstat_done, &st))
 			goto finish;
 	}
+	path = path_buf->buf;
 
 	switch (ce_mode_s_ifmt) {
 	case S_IFREG:
@@ -183,7 +185,8 @@ static int write_entry(struct cache_entry *ce,
 			size = newsize;
 		}
 
-		fd = open_output_fd(path, ce, to_tempfile);
+		fd = open_output_fd(path_buf, ce, to_tempfile);
+		path = path_buf->buf;
 		if (fd < 0) {
 			free(new);
 			return error("unable to create file %s (%s)",
@@ -235,10 +238,10 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)
 }
 
 int checkout_entry(struct cache_entry *ce,
-		   const struct checkout *state, char *topath)
+		   const struct checkout *state, struct strbuf *topath)
 {
 	static struct strbuf path_buf = STRBUF_INIT;
-	char *path;
+	const char *path;
 	struct stat st;
 	int len;
 
@@ -278,5 +281,5 @@ int checkout_entry(struct cache_entry *ce,
 	} else if (state->not_new)
 		return 0;
 	create_directories(path, len, state);
-	return write_entry(ce, path, state, 0);
+	return write_entry(ce, &path_buf, state, 0);
 }
-- 
1.8.2.83.gc99314b

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] Prevent buffer overflows when path is too big

[Duy Nguyen]

On Tue, Oct 22, 2013 at 2:02 AM, Johannes Sixt <j6t@kdbg.org> wrote:
>> (or maybe at higher level to skip checking out those paths).
>
> More like this, yeah.

The good thing is we do not stop checking out if one entry fails. But
due to the lack of worktree entries, one may accidentally remove files
in new commits. So setting CE_VALID on failed-to-checkout entries
might help. I'm not sure. But I won't pursue because Windows is not
really my itch.
-- 
Duy

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf

[Antoine Pelisse]

On Wed, Oct 23, 2013 at 2:55 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> The old code does not do boundary check so any paths longer than
> PATH_MAX can cause buffer overflow. Replace it with strbuf to handle
> paths of arbitrary length.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  To get this topic going again. These two patches kill PATH_MAX in
>  entry.c and builtin/checkout-index.c

Thanks !

> diff --git a/entry.c b/entry.c
> index acc892f..d955af5 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -237,16 +237,18 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)
>  int checkout_entry(struct cache_entry *ce,
>                    const struct checkout *state, char *topath)
>  {
> -       static char path[PATH_MAX + 1];
> +       static struct strbuf path_buf = STRBUF_INIT;
> +       char *path;
>         struct stat st;
> -       int len = state->base_dir_len;
> +       int len;
>
>         if (topath)
>                 return write_entry(ce, topath, state, 1);
>
> -       memcpy(path, state->base_dir, len);
> -       strcpy(path + len, ce->name);
> -       len += ce_namelen(ce);
> +       strbuf_reset(&path_buf);

I think this is not required

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf

[Duy Nguyen]

On Wed, Oct 23, 2013 at 7:58 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
>> diff --git a/entry.c b/entry.c
>> index acc892f..d955af5 100644
>> --- a/entry.c
>> +++ b/entry.c
>> @@ -237,16 +237,18 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)
>>  int checkout_entry(struct cache_entry *ce,
>>                    const struct checkout *state, char *topath)
>>  {
>> -       static char path[PATH_MAX + 1];
>> +       static struct strbuf path_buf = STRBUF_INIT;
>> +       char *path;
>>         struct stat st;
>> -       int len = state->base_dir_len;
>> +       int len;
>>
>>         if (topath)
>>                 return write_entry(ce, topath, state, 1);
>>
>> -       memcpy(path, state->base_dir, len);
>> -       strcpy(path + len, ce->name);
>> -       len += ce_namelen(ce);
>> +       strbuf_reset(&path_buf);
>
> I think this is not required

If you mean strbuf_reset, I think it is. path_buf is still static (I
don't want to remove that because it'll add a lot more strbuf_release)
so we can't be sure what it contains from the second checkout_entry()
call.
-- 
Duy

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf

[Antoine Pelisse]

On Wed, Oct 23, 2013 at 3:04 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, Oct 23, 2013 at 7:58 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
>>> diff --git a/entry.c b/entry.c
>>> index acc892f..d955af5 100644
>>> --- a/entry.c
>>> +++ b/entry.c
>>> @@ -237,16 +237,18 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)
>>>  int checkout_entry(struct cache_entry *ce,
>>>                    const struct checkout *state, char *topath)
>>>  {
>>> -       static char path[PATH_MAX + 1];
>>> +       static struct strbuf path_buf = STRBUF_INIT;
>>> +       char *path;
>>>         struct stat st;
>>> -       int len = state->base_dir_len;
>>> +       int len;
>>>
>>>         if (topath)
>>>                 return write_entry(ce, topath, state, 1);
>>>
>>> -       memcpy(path, state->base_dir, len);
>>> -       strcpy(path + len, ce->name);
>>> -       len += ce_namelen(ce);
>>> +       strbuf_reset(&path_buf);
>>
>> I think this is not required
>
> If you mean strbuf_reset, I think it is. path_buf is still static (I
> don't want to remove that because it'll add a lot more strbuf_release)
> so we can't be sure what it contains from the second checkout_entry()
> call.

Of course, I forgot about the static,
Thanks :-)

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf

[Jeff King]

On Wed, Oct 23, 2013 at 07:55:06PM +0700, Nguyen Thai Ngoc Duy wrote:

> The old code does not do boundary check so any paths longer than
> PATH_MAX can cause buffer overflow. Replace it with strbuf to handle
> paths of arbitrary length.

I think this is a reasonable solution. If we have such a long path, we
are probably about to feed it to open() or another syscall, and we will
just get ENAMETOOLONG there anyway. But certainly we need to fix the
buffer overflow, and we are probably better off letting the syscall
report failure than calling die(), because we generally handle the
syscall failure more gracefully (e.g., by reporting the failed path but
continuing).

> -	memcpy(path, state->base_dir, len);
> -	strcpy(path + len, ce->name);
> -	len += ce_namelen(ce);
> +	strbuf_reset(&path_buf);
> +	strbuf_addf(&path_buf, "%.*s%s", state->base_dir_len, state->base_dir, ce->name);
> +	path = path_buf.buf;
> +	len = path_buf.len;

This is not something you introduced, but while we are here, you may
want to use ce->namelen, which would be a little faster than treating it
as a string (especially for strbuf, as it can then know up front how big
the size is).

I doubt it's measurable, though (especially as the growth cost is
amortized due to the static buffer).

-Peff

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf

[Erik Faye-Lund]

On Wed, Oct 23, 2013 at 7:29 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Oct 23, 2013 at 07:55:06PM +0700, Nguyen Thai Ngoc Duy wrote:
>
>> The old code does not do boundary check so any paths longer than
>> PATH_MAX can cause buffer overflow. Replace it with strbuf to handle
>> paths of arbitrary length.
>
> I think this is a reasonable solution. If we have such a long path, we
> are probably about to feed it to open() or another syscall, and we will
> just get ENAMETOOLONG there anyway. But certainly we need to fix the
> buffer overflow, and we are probably better off letting the syscall
> report failure than calling die(), because we generally handle the
> syscall failure more gracefully (e.g., by reporting the failed path but
> continuing).
>
>> -     memcpy(path, state->base_dir, len);
>> -     strcpy(path + len, ce->name);
>> -     len += ce_namelen(ce);
>> +     strbuf_reset(&path_buf);
>> +     strbuf_addf(&path_buf, "%.*s%s", state->base_dir_len, state->base_dir, ce->name);
>> +     path = path_buf.buf;
>> +     len = path_buf.len;
>
> This is not something you introduced, but while we are here, you may
> want to use ce->namelen, which would be a little faster than treating it
> as a string (especially for strbuf, as it can then know up front how big
> the size is).
>
> I doubt it's measurable, though (especially as the growth cost is
> amortized due to the static buffer).

I somehow feel that:

strbuf_reset(&path_buf);
strbuf_add(&path_buf, state->base_dir, state->base_dir_len);
strbuf_addch(&path_buf, '/');
strbuf_add(&path_buf, state->name, state->name_len);

feels a bit neater than using strbuf_addf. But that might just be me.

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf

[Jeff King]

On Wed, Oct 23, 2013 at 07:34:18PM +0200, Erik Faye-Lund wrote:

> >> -     memcpy(path, state->base_dir, len);
> >> -     strcpy(path + len, ce->name);
> >> -     len += ce_namelen(ce);
> >> +     strbuf_reset(&path_buf);
> >> +     strbuf_addf(&path_buf, "%.*s%s", state->base_dir_len, state->base_dir, ce->name);
> >> +     path = path_buf.buf;
> >> +     len = path_buf.len;
> >
> > This is not something you introduced, but while we are here, you may
> > want to use ce->namelen, which would be a little faster than treating it
> > as a string (especially for strbuf, as it can then know up front how big
> > the size is).
> >
> > I doubt it's measurable, though (especially as the growth cost is
> > amortized due to the static buffer).
> 
> I somehow feel that:
> 
> strbuf_reset(&path_buf);
> strbuf_add(&path_buf, state->base_dir, state->base_dir_len);
> strbuf_addch(&path_buf, '/');
> strbuf_add(&path_buf, state->name, state->name_len);
> 
> feels a bit neater than using strbuf_addf. But that might just be me.

I agree. But note that your addch is a bug. :)

-Peff

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH 2/2] entry.c: convert write_entry to use strbuf

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> The strcpy call in open_output_fd() implies that the output buffer
> must be at least 25 chars long.

Hmph, where does that 25 come from?

> And it's true. The only caller that
> can trigger that code is checkout-index, which has the buffer of
> PATH_MAX chars (and any systems that have PATH_MAX shorter than 25
> chars are just insane).
>
> But in order to say that, one has to walk through a dozen of
> functions. Just convert it to strbuf to avoid the constraint and
> confusion.

Wouldn't it be far clearer to document what is going on especially
around the topath parameter to checkout_entry(), than to introduce
unnecessary strbuf overhead?

At first glance, it might appear that the caller of checkout_entry()
can specify to which path the contents are written out, but in
reality topath[] is to point at the buffer to store the temporary
path generated by the lower guts of write_entry().  It is unclear in
the original code and that is worth an in-code comment.

And when describing that API requirement, we would need to say how
big a buffer the caller must allocate for topath[] in the comment.
That size does not have to be platform-dependent PATH_MAX.

Something like this?

 builtin/checkout-index.c | 2 +-
 cache.h                  | 1 +
 entry.c                  | 8 ++++++++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index b1feda7..4ed6b23 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -14,7 +14,7 @@
 static int line_termination = '\n';
 static int checkout_stage; /* default to checkout stage0 */
 static int to_tempfile;
-static char topath[4][PATH_MAX + 1];
+static char topath[4][TEMPORARY_FILENAME_LENGTH + 1];
 
 static struct checkout state;
 
diff --git a/cache.h b/cache.h
index 85b544f..3118b7f 100644
--- a/cache.h
+++ b/cache.h
@@ -975,6 +975,7 @@ struct checkout {
 		 refresh_cache:1;
 };
 
+#define TEMPORARY_FILENAME_LENGTH 25
 extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);
 
 struct cache_def {
diff --git a/entry.c b/entry.c
index d955af5..2df4ee1 100644
--- a/entry.c
+++ b/entry.c
@@ -234,6 +234,14 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)
 	return lstat(path, st);
 }
 
+/*
+ * Write the contents from ce out to the working tree.
+ *
+ * When topath[] is not NULL, instead of writing to the working tree
+ * file named by ce, a temporary file is created by this function and
+ * its name is returned in topath[], which must be able to hold at
+ * least TEMPORARY_FILENAME_LENGTH bytes long.
+ */
 int checkout_entry(struct cache_entry *ce,
 		   const struct checkout *state, char *topath)
 {

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Oct 23, 2013 at 07:55:06PM +0700, Nguyen Thai Ngoc Duy wrote:
> ...
>> -	memcpy(path, state->base_dir, len);
>> -	strcpy(path + len, ce->name);
>> -	len += ce_namelen(ce);
>> +	strbuf_reset(&path_buf);
>> +	strbuf_addf(&path_buf, "%.*s%s", state->base_dir_len, state->base_dir, ce->name);
>> +	path = path_buf.buf;
>> +	len = path_buf.len;
>
> This is not something you introduced, but while we are here, you may
> want to use ce->namelen, which would be a little faster than treating it
> as a string (especially for strbuf, as it can then know up front how big
> the size is).

Hmmmm, do you mean something like this on top?

diff --git a/entry.c b/entry.c
index d955af5..0d48292 100644
--- a/entry.c
+++ b/entry.c
@@ -246,7 +246,9 @@ int checkout_entry(struct cache_entry *ce,
 		return write_entry(ce, topath, state, 1);
 
 	strbuf_reset(&path_buf);
-	strbuf_addf(&path_buf, "%.*s%s", state->base_dir_len, state->base_dir, ce->name);
+	strbuf_addf(&path_buf, "%.*s%.*s",
+		    state->base_dir_len, state->base_dir,
+		    ce_namelen(ce), ce->name);
 	path = path_buf.buf;
 	len = path_buf.len;
 

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH 1/2] entry.c: convert checkout_entry to use strbuf

[Jeff King]

On Wed, Oct 23, 2013 at 11:09:27AM -0700, Junio C Hamano wrote:

> > This is not something you introduced, but while we are here, you may
> > want to use ce->namelen, which would be a little faster than treating it
> > as a string (especially for strbuf, as it can then know up front how big
> > the size is).
> 
> Hmmmm, do you mean something like this on top?
> 
> diff --git a/entry.c b/entry.c
> index d955af5..0d48292 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -246,7 +246,9 @@ int checkout_entry(struct cache_entry *ce,
>  		return write_entry(ce, topath, state, 1);
>  
>  	strbuf_reset(&path_buf);
> -	strbuf_addf(&path_buf, "%.*s%s", state->base_dir_len, state->base_dir, ce->name);
> +	strbuf_addf(&path_buf, "%.*s%.*s",
> +		    state->base_dir_len, state->base_dir,
> +		    ce_namelen(ce), ce->name);
>  	path = path_buf.buf;
>  	len = path_buf.len;

Yes, though I actually find Erik's version with two separate strbuf_add
invocations slightly more readable (it _could_ result in two
allocations, but again, we are amortizing the growth over many calls
anyway, so most of them will not need to grow the buffer at all).

-Peff

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH 2/2] entry.c: convert write_entry to use strbuf

[Duy Nguyen]

On Thu, Oct 24, 2013 at 12:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> The strcpy call in open_output_fd() implies that the output buffer
>> must be at least 25 chars long.
>
> Hmph, where does that 25 come from?
>
> [snipped]

Much better. Thanks.
-- 
Duy

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[PATCH v2] entry.c: convert checkout_entry to use strbuf

[Nguyễn Thái Ngọc Duy]

The old code does not do boundary check so any paths longer than
PATH_MAX can cause buffer overflow. Replace it with strbuf to handle
paths of arbitrary length.

The OS may reject if the path is too long though. But in that case we
report the cause (e.g. name too long) and usually move on to checking
out the next entry.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 v2 does two strbuf_add() instead of one hard-to-read strbuf_addf()

 entry.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/entry.c b/entry.c
index acc892f..fbb4863 100644
--- a/entry.c
+++ b/entry.c
@@ -237,16 +237,19 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)
 int checkout_entry(struct cache_entry *ce,
 		   const struct checkout *state, char *topath)
 {
-	static char path[PATH_MAX + 1];
+	static struct strbuf path_buf = STRBUF_INIT;
+	char *path;
 	struct stat st;
-	int len = state->base_dir_len;
+	int len;
 
 	if (topath)
 		return write_entry(ce, topath, state, 1);
 
-	memcpy(path, state->base_dir, len);
-	strcpy(path + len, ce->name);
-	len += ce_namelen(ce);
+	strbuf_reset(&path_buf);
+	strbuf_add(&path_buf, state->base_dir, state->base_dir_len);
+	strbuf_add(&path_buf, ce->name, ce_namelen(ce));
+	path = path_buf.buf;
+	len = path_buf.len;
 
 	if (!check_path(path, len, &st, state->base_dir_len)) {
 		unsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE);
-- 
1.8.2.82.gc24b958

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH 2/2] entry.c: convert write_entry to use strbuf

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Thu, Oct 24, 2013 at 12:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>
>>> The strcpy call in open_output_fd() implies that the output buffer
>>> must be at least 25 chars long.
>>
>> Hmph, where does that 25 come from?
>>
>> [snipped]
>
> Much better. Thanks.

So where does that 25 come from?

We strcpy ".merge_link_XXXXXX" or ".merge_file_XXXXXX" into path[]
and run mkstemp() on it, and these templates are 18 bytes long, so I
am puzzled.

Is 25 "just a small random number that is surely longer than these
templates--did not bother to count how long the templates are"?
That's fine by me; I am just trying to make sure I am not missing
anything that turns these templates into a longer filename.

Thanks.

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH 2/2] entry.c: convert write_entry to use strbuf

[Duy Nguyen]

On Fri, Oct 25, 2013 at 2:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Thu, Oct 24, 2013 at 12:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>>
>>>> The strcpy call in open_output_fd() implies that the output buffer
>>>> must be at least 25 chars long.
>>>
>>> Hmph, where does that 25 come from?
>>>
>>> [snipped]
>>
>> Much better. Thanks.
>
> So where does that 25 come from?
>
> We strcpy ".merge_link_XXXXXX" or ".merge_file_XXXXXX" into path[]
> and run mkstemp() on it, and these templates are 18 bytes long, so I
> am puzzled.
>
> Is 25 "just a small random number that is surely longer than these
> templates--did not bother to count how long the templates are"?

Yes. I was too lazy to subtract precisely the column number from
between the quotes, so I just made sure the number is large enough to
cover the columns..

> That's fine by me; I am just trying to make sure I am not missing
> anything that turns these templates into a longer filename.
-- 
Duy

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[PATCH] Prevent buffer overflows when path is too long

[Antoine Pelisse]

Some buffers created with PATH_MAX length are not checked when being
written, and can overflow if PATH_MAX is not big enough to hold the
path.

Some of the use-case are probably impossible to reach, and the program
dies if the path looks too long. When it would be possible for the user
to use a longer path, simply use strbuf to build it.

Reported-by: Wataru Noguchi <wnoguchi.0727@gmail.com>
Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
 abspath.c        | 10 ++++++++--
 diffcore-order.c | 14 +++++++++-----
 unpack-trees.c   |  2 ++
 3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/abspath.c b/abspath.c
index e390994..29a5f9d 100644
--- a/abspath.c
+++ b/abspath.c
@@ -216,11 +216,16 @@ const char *absolute_path(const char *path)
 const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
 {
 	static char path[PATH_MAX];
+
+	if (pfx_len >= PATH_MAX)
+		die("Too long prefix path: %s", pfx);
+
 #ifndef GIT_WINDOWS_NATIVE
 	if (!pfx_len || is_absolute_path(arg))
 		return arg;
 	memcpy(path, pfx, pfx_len);
-	strcpy(path + pfx_len, arg);
+	if (strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len) > PATH_MAX)
+		die("Too long path: %s", path);
 #else
 	char *p;
 	/* don't add prefix to absolute paths, but still replace '\' by '/' */
@@ -228,7 +233,8 @@ const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
 		pfx_len = 0;
 	else if (pfx_len)
 		memcpy(path, pfx, pfx_len);
-	strcpy(path + pfx_len, arg);
+	if (strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len) > PATH_MAX)
+		die("Too long path: %s", path);
 	for (p = path + pfx_len; *p; p++)
 		if (*p == '\\')
 			*p = '/';
diff --git a/diffcore-order.c b/diffcore-order.c
index 23e9385..87193f8 100644
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -73,20 +73,24 @@ struct pair_order {
 static int match_order(const char *path)
 {
 	int i;
-	char p[PATH_MAX];
+	struct strbuf p = STRBUF_INIT;
 
 	for (i = 0; i < order_cnt; i++) {
-		strcpy(p, path);
-		while (p[0]) {
+		strbuf_reset(&p);
+		strbuf_addstr(&p, path);
+		while (p.buf[0]) {
 			char *cp;
-			if (!fnmatch(order[i], p, 0))
+			if (!fnmatch(order[i], p.buf, 0)) {
+				strbuf_release(&p);
 				return i;
-			cp = strrchr(p, '/');
+			}
+			cp = strrchr(p.buf, '/');
 			if (!cp)
 				break;
 			*cp = 0;
 		}
 	}
+	strbuf_release(&p);
 	return order_cnt;
 }
 
diff --git a/unpack-trees.c b/unpack-trees.c
index 35cb05e..f93565b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -918,6 +918,8 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,
 			int processed;
 
 			len = slash - name;
+			if (len + prefix_len >= PATH_MAX)
+				die("Too long path: %s", prefix);
 			memcpy(prefix + prefix_len, name, len);
 
 			/*
-- 
1.8.5.rc3.1.ga0b6b91

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] Prevent buffer overflows when path is too long

[Junio C Hamano]

Antoine Pelisse <apelisse@gmail.com> writes:

> Some buffers created with PATH_MAX length are not checked when being
> written, and can overflow if PATH_MAX is not big enough to hold the
> path.

Perhaps it is time to update all of them to use strbuf?  The callers
of prefix_filename() aren't that many, and all of them are prepared
to stash the returned value away when they keep it longer term, so
they would not notice if we used "static struct strbuf path" and
gave back "path.buf" (without strbuf_detach() on it).  The buffer
used in clear_ce_flags() and passed to clear_ce_flags_{1,dir} are
not seen outside the callchain, and can safely become strbuf, I
think.

>  abspath.c        | 10 ++++++++--
>  diffcore-order.c | 14 +++++++++-----
>  unpack-trees.c   |  2 ++
>  3 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/abspath.c b/abspath.c
> index e390994..29a5f9d 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -216,11 +216,16 @@ const char *absolute_path(const char *path)
>  const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
>  {
>  	static char path[PATH_MAX];
> +
> +	if (pfx_len >= PATH_MAX)
> +		die("Too long prefix path: %s", pfx);

I do not think this is needed, and will reject a valid input that
used to be accepted (e.g. arg is absolute so pfx does not matter).

>  #ifndef GIT_WINDOWS_NATIVE
>  	if (!pfx_len || is_absolute_path(arg))
>  		return arg;
>  	memcpy(path, pfx, pfx_len);
> -	strcpy(path + pfx_len, arg);
> +	if (strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len) > PATH_MAX)
> +		die("Too long path: %s", path);

Rather, have that "too long a prefix?" check before that memcpy().

>  #else
>  	char *p;
>  	/* don't add prefix to absolute paths, but still replace '\' by '/' */
> @@ -228,7 +233,8 @@ const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
>  		pfx_len = 0;
>  	else if (pfx_len)
>  		memcpy(path, pfx, pfx_len);

... and around here.

> -	strcpy(path + pfx_len, arg);
> +	if (strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len) > PATH_MAX)
> +		die("Too long path: %s", path);
>  	for (p = path + pfx_len; *p; p++)
>  		if (*p == '\\')
>  			*p = '/';

The above is curious. Unless we are doing the short-cut for "no
prefix so we can just return arg" codepath, we know that the
resulting length is always pfx_len + strlen(arg), no?

> diff --git a/diffcore-order.c b/diffcore-order.c
> index 23e9385..87193f8 100644
> --- a/diffcore-order.c
> +++ b/diffcore-order.c
> @@ -73,20 +73,24 @@ struct pair_order {
>  static int match_order(const char *path)
>  {
>  	int i;
> -	char p[PATH_MAX];
> +	struct strbuf p = STRBUF_INIT;
>  
>  	for (i = 0; i < order_cnt; i++) {
> -		strcpy(p, path);
> -		while (p[0]) {
> +		strbuf_reset(&p);
> +		strbuf_addstr(&p, path);
> +		while (p.buf[0]) {
>  			char *cp;
> -			if (!fnmatch(order[i], p, 0))
> +			if (!fnmatch(order[i], p.buf, 0)) {
> +				strbuf_release(&p);
>  				return i;
> -			cp = strrchr(p, '/');
> +			}
> +			cp = strrchr(p.buf, '/');
>  			if (!cp)
>  				break;
>  			*cp = 0;
>  		}
>  	}
> +	strbuf_release(&p);
>  	return order_cnt;
>  }
>  
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 35cb05e..f93565b 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -918,6 +918,8 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,
>  			int processed;
>  
>  			len = slash - name;
> +			if (len + prefix_len >= PATH_MAX)
> +				die("Too long path: %s", prefix);
>  			memcpy(prefix + prefix_len, name, len);
>  
>  			/*
> -- 
> 1.8.5.rc3.1.ga0b6b91
>
> -- 

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] Prevent buffer overflows when path is too long

[Antoine Pelisse]

On Tue, Nov 26, 2013 at 8:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Antoine Pelisse <apelisse@gmail.com> writes:
>
>> Some buffers created with PATH_MAX length are not checked when being
>> written, and can overflow if PATH_MAX is not big enough to hold the
>> path.
>
> Perhaps it is time to update all of them to use strbuf?  The callers
> of prefix_filename() aren't that many, and all of them are prepared
> to stash the returned value away when they keep it longer term, so
> they would not notice if we used "static struct strbuf path" and
> gave back "path.buf" (without strbuf_detach() on it).  The buffer
> used in clear_ce_flags() and passed to clear_ce_flags_{1,dir} are
> not seen outside the callchain, and can safely become strbuf, I
> think.

Let's do that, but shouldn't we also modify those that are currently
safe, like absolute_path() just above prefix_filename() ?

>>  abspath.c        | 10 ++++++++--
>>  diffcore-order.c | 14 +++++++++-----
>>  unpack-trees.c   |  2 ++
>>  3 files changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/abspath.c b/abspath.c
>> index e390994..29a5f9d 100644
>> --- a/abspath.c
>> +++ b/abspath.c
>> @@ -216,11 +216,16 @@ const char *absolute_path(const char *path)
>>  const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
>>  {
>>       static char path[PATH_MAX];
>> +
>> +     if (pfx_len >= PATH_MAX)
>> +             die("Too long prefix path: %s", pfx);
>
> I do not think this is needed, and will reject a valid input that
> used to be accepted (e.g. arg is absolute so pfx does not matter).

My mistake

>> -     strcpy(path + pfx_len, arg);
>> +     if (strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len) > PATH_MAX)
>> +             die("Too long path: %s", path);
>>       for (p = path + pfx_len; *p; p++)
>>               if (*p == '\\')
>>                       *p = '/';
>
> The above is curious. Unless we are doing the short-cut for "no
> prefix so we can just return arg" codepath, we know that the
> resulting length is always pfx_len + strlen(arg), no?

If you mean that the test should be more like the following:
+     if (strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len) > PATH_MAX - pfx_len)

Then of course, you are right, that's my mistake.

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[PATCH] Prevent buffer overflows when path is too long

[Antoine Pelisse]

Some buffers created with PATH_MAX length are not checked when being
written, and can overflow if PATH_MAX is not big enough to hold the
path.

Replace those buffers by strbufs so that their size is automatically
grown if necessary. They are created as static local variables to avoid
reallocating memory on each call. Note that prefix_filename() returns
this static buffer so each callers should copy or use the string
immediately (this is currently true).

Reported-by: Wataru Noguchi <wnoguchi.0727@gmail.com>
Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
 abspath.c        | 16 +++++++++-------
 diffcore-order.c | 11 ++++++-----
 unpack-trees.c   | 51 +++++++++++++++++++++++++++------------------------
 3 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/abspath.c b/abspath.c
index e390994..9c908e3 100644
--- a/abspath.c
+++ b/abspath.c
@@ -215,23 +215,25 @@ const char *absolute_path(const char *path)
  */
 const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
 {
-	static char path[PATH_MAX];
+	static struct strbuf path = STRBUF_INIT;
 #ifndef GIT_WINDOWS_NATIVE
 	if (!pfx_len || is_absolute_path(arg))
 		return arg;
-	memcpy(path, pfx, pfx_len);
-	strcpy(path + pfx_len, arg);
+	strbuf_reset(&path);
+	strbuf_add(&path, pfx, pfx_len);
+	strbuf_addstr(&path, arg);
 #else
 	char *p;
 	/* don't add prefix to absolute paths, but still replace '\' by '/' */
+	strbuf_reset(&path);
 	if (is_absolute_path(arg))
 		pfx_len = 0;
 	else if (pfx_len)
-		memcpy(path, pfx, pfx_len);
-	strcpy(path + pfx_len, arg);
-	for (p = path + pfx_len; *p; p++)
+		strbuf_add(&path, pfx, pfx_len);
+	strbuf_addstr(&path, arg);
+	for (p = path.buf + pfx_len; *p; p++)
 		if (*p == '\\')
 			*p = '/';
 #endif
-	return path;
+	return path.buf;
 }
diff --git a/diffcore-order.c b/diffcore-order.c
index 23e9385..50c089b 100644
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -73,15 +73,16 @@ struct pair_order {
 static int match_order(const char *path)
 {
 	int i;
-	char p[PATH_MAX];
+	static struct strbuf p = STRBUF_INIT;
 
 	for (i = 0; i < order_cnt; i++) {
-		strcpy(p, path);
-		while (p[0]) {
+		strbuf_reset(&p);
+		strbuf_addstr(&p, path);
+		while (p.buf[0]) {
 			char *cp;
-			if (!fnmatch(order[i], p, 0))
+			if (!fnmatch(order[i], p.buf, 0))
 				return i;
-			cp = strrchr(p, '/');
+			cp = strrchr(p.buf, '/');
 			if (!cp)
 				break;
 			*cp = 0;
diff --git a/unpack-trees.c b/unpack-trees.c
index ad3e9a0..164354d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -830,23 +830,24 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str
 }
 
 static int clear_ce_flags_1(struct cache_entry **cache, int nr,
-			    char *prefix, int prefix_len,
+			    struct strbuf *prefix,
 			    int select_mask, int clear_mask,
 			    struct exclude_list *el, int defval);
 
 /* Whole directory matching */
 static int clear_ce_flags_dir(struct cache_entry **cache, int nr,
-			      char *prefix, int prefix_len,
+			      struct strbuf *prefix,
 			      char *basename,
 			      int select_mask, int clear_mask,
 			      struct exclude_list *el, int defval)
 {
 	struct cache_entry **cache_end;
 	int dtype = DT_DIR;
-	int ret = is_excluded_from_list(prefix, prefix_len,
+	int ret = is_excluded_from_list(prefix->buf, prefix->len,
 					basename, &dtype, el);
+	int rc;
 
-	prefix[prefix_len++] = '/';
+	strbuf_addch(prefix, '/');
 
 	/* If undecided, use matching result of parent dir in defval */
 	if (ret < 0)
@@ -854,7 +855,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,
 
 	for (cache_end = cache; cache_end != cache + nr; cache_end++) {
 		struct cache_entry *ce = *cache_end;
-		if (strncmp(ce->name, prefix, prefix_len))
+		if (strncmp(ce->name, prefix->buf, prefix->len))
 			break;
 	}
 
@@ -865,10 +866,12 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,
 	 * calling clear_ce_flags_1(). That function will call
 	 * the expensive is_excluded_from_list() on every entry.
 	 */
-	return clear_ce_flags_1(cache, cache_end - cache,
-				prefix, prefix_len,
-				select_mask, clear_mask,
-				el, ret);
+	rc = clear_ce_flags_1(cache, cache_end - cache,
+			      prefix,
+			      select_mask, clear_mask,
+			      el, ret);
+	strbuf_setlen(prefix, prefix->len - 1);
+	return rc;
 }
 
 /*
@@ -887,7 +890,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,
  * Top level path has prefix_len zero.
  */
 static int clear_ce_flags_1(struct cache_entry **cache, int nr,
-			    char *prefix, int prefix_len,
+			    struct strbuf *prefix,
 			    int select_mask, int clear_mask,
 			    struct exclude_list *el, int defval)
 {
@@ -907,10 +910,10 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,
 			continue;
 		}
 
-		if (prefix_len && strncmp(ce->name, prefix, prefix_len))
+		if (prefix->len && strncmp(ce->name, prefix->buf, prefix->len))
 			break;
 
-		name = ce->name + prefix_len;
+		name = ce->name + prefix->len;
 		slash = strchr(name, '/');
 
 		/* If it's a directory, try whole directory match first */
@@ -918,29 +921,26 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,
 			int processed;
 
 			len = slash - name;
-			memcpy(prefix + prefix_len, name, len);
+			strbuf_add(prefix, name, len);
 
-			/*
-			 * terminate the string (no trailing slash),
-			 * clear_c_f_dir needs it
-			 */
-			prefix[prefix_len + len] = '\0';
 			processed = clear_ce_flags_dir(cache, cache_end - cache,
-						       prefix, prefix_len + len,
-						       prefix + prefix_len,
+						       prefix,
+						       prefix->buf + prefix->len - len,
 						       select_mask, clear_mask,
 						       el, defval);
 
 			/* clear_c_f_dir eats a whole dir already? */
 			if (processed) {
 				cache += processed;
+				strbuf_setlen(prefix, prefix->len - len);
 				continue;
 			}
 
-			prefix[prefix_len + len++] = '/';
+			strbuf_addch(prefix, '/');
 			cache += clear_ce_flags_1(cache, cache_end - cache,
-						  prefix, prefix_len + len,
+						  prefix,
 						  select_mask, clear_mask, el, defval);
+			strbuf_setlen(prefix, prefix->len - len - 1);
 			continue;
 		}
 
@@ -961,9 +961,12 @@ static int clear_ce_flags(struct cache_entry **cache, int nr,
 			    int select_mask, int clear_mask,
 			    struct exclude_list *el)
 {
-	char prefix[PATH_MAX];
+	static struct strbuf prefix = STRBUF_INIT;
+
+	strbuf_reset(&prefix);
+
 	return clear_ce_flags_1(cache, nr,
-				prefix, 0,
+				&prefix,
 				select_mask, clear_mask,
 				el, 0);
 }
-- 
1.8.5.1.94.g19422b2