Re: [msysGit] [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.0...@gmail.com
  http://wnoguchi.github.io/
=
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stefan Beller]

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.





signature.asc
Description: OpenPGP digital signature

Re: [msysGit] [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 
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 0x779f not found.
Error: dll starting at 0x7590 not found.
Error: dll starting at 0x779f not found.
Error: dll starting at 0x778f not found.
[New thread 800.0x92c]
Cloning into 'mingw-checkout-crash'...
Error: dll starting at 0x29f 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,
Dsc

Re: [msysGit] [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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html