Patch 7.3.648

[Bram Moolenaar]

Patch 7.3.648
Problem: Crash when using a very long file name. (ZyX)
Solution: Properly check length of buffer space.
Files: src/buffer.c


*** ../vim-7.3.647/src/buffer.c 2012-07-16 17:31:48.000000000 +0200
--- src/buffer.c 2012-09-05 13:17:38.000000000 +0200
***************
*** 3234,3245 ****
{
/* format: "fname + (path) (1 of 2) - VIM" */

if (curbuf->b_fname == NULL)
! vim_strncpy(buf, (char_u *)_("[No Name]"), IOSIZE - 100);
else
{
p = transstr(gettail(curbuf->b_fname));
! vim_strncpy(buf, p, IOSIZE - 100);
vim_free(p);
}

--- 3234,3248 ----
{
/* format: "fname + (path) (1 of 2) - VIM" */

+ #define SPACE_FOR_FNAME (IOSIZE - 100)
+ #define SPACE_FOR_DIR (IOSIZE - 20)
+ #define SPACE_FOR_ARGNR (IOSIZE - 10) /* at least room for " - VIM" */
if (curbuf->b_fname == NULL)
! vim_strncpy(buf, (char_u *)_("[No Name]"), SPACE_FOR_FNAME);
else
{
p = transstr(gettail(curbuf->b_fname));
! vim_strncpy(buf, p, SPACE_FOR_FNAME);
vim_free(p);
}

***************
*** 3263,3269 ****
buf[off++] = ' ';
buf[off++] = '(';
home_replace(curbuf, curbuf->b_ffname,
! buf + off, IOSIZE - off, TRUE);
#ifdef BACKSLASH_IN_FILENAME
/* avoid "c:/name" to be reduced to "c" */
if (isalpha(buf[off]) && buf[off + 1] == ':')
--- 3266,3272 ----
buf[off++] = ' ';
buf[off++] = '(';
home_replace(curbuf, curbuf->b_ffname,
! buf + off, SPACE_FOR_DIR - off, TRUE);
#ifdef BACKSLASH_IN_FILENAME
/* avoid "c:/name" to be reduced to "c" */
if (isalpha(buf[off]) && buf[off + 1] == ':')
***************
*** 3274,3291 ****
if (p == buf + off)
/* must be a help buffer */
vim_strncpy(buf + off, (char_u *)_("help"),
! (size_t)(IOSIZE - off - 1));
else
*p = NUL;

! /* translate unprintable chars */
! p = transstr(buf + off);
! vim_strncpy(buf + off, p, (size_t)(IOSIZE - off - 1));
! vim_free(p);
STRCAT(buf, ")");
}

! append_arg_number(curwin, buf, IOSIZE, FALSE);

#if defined(FEAT_CLIENTSERVER)
if (serverName != NULL)
--- 3277,3304 ----
if (p == buf + off)
/* must be a help buffer */
vim_strncpy(buf + off, (char_u *)_("help"),
! (size_t)(SPACE_FOR_DIR - off - 1));
else
*p = NUL;

! /* Translate unprintable chars and concatenate. Keep some
! * room for the server name. When there is no room (very long
! * file name) use (...). */
! if (off < SPACE_FOR_DIR)
! {
! p = transstr(buf + off);
! vim_strncpy(buf + off, p, (size_t)(SPACE_FOR_DIR - off));
! vim_free(p);
! }
! else
! {
! vim_strncpy(buf + off, (char_u *)"...",
! (size_t)(SPACE_FOR_ARGNR - off));
! }
STRCAT(buf, ")");
}

! append_arg_number(curwin, buf, SPACE_FOR_ARGNR, FALSE);

#if defined(FEAT_CLIENTSERVER)
if (serverName != NULL)
*** ../vim-7.3.647/src/version.c 2012-09-05 12:16:40.000000000 +0200
--- src/version.c 2012-09-05 13:29:53.000000000 +0200
***************
*** 721,722 ****
--- 721,724 ----
{ /* Add new patch number below this line */
+ /**/
+ 648,
/**/

--
Q: How does a UNIX Guru do Sex ?
A: unzip;strip;touch;finger;mount;fsck;more;yes;umount;sleep

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[ZyX]

среда, 5 сентября 2012 г., 15:31:07 UTC+4 пользователь Bram Moolenaar написал:

> Patch 7.3.648
> Problem: Crash when using a very long file name. (ZyX)
> Solution: Properly check length of buffer space.
> Files: src/buffer.c

That actually did not fix the problem: gcc optimized build now behaves like non-optimized one, thats all. I still get a crash where I was getting it before (aurum, “AuDiff rev1 1 rev2 tip **”). And I still observe invalid number of lines (1041 lines with last one truncated).

I don’t get invalid number of lines if I replace “expand("<amatch>")” with “bufname(+expand("<abuf>"))”. Doing the same in aurum does not fix the crash. Crash happens at the very end of “fileinfo” function from buffer.c. The following patch seems to fix the crash:
# HG changeset patch
# User ZyX <kp-...@ya.ru>
# Date 1346961521 -14400
# Node ID acfe7154579e02ff10e39d50fa2a9794eb2d3f0b
# Parent 1052677493beb941eab0d1e33d63c73ee4148350
Fixed how fileinfo handles long filenames

diff -r 1052677493be -r acfe7154579e src/buffer.c
--- a/src/buffer.c Wed Sep 05 19:17:17 2012 +0200
+++ b/src/buffer.c Thu Sep 06 23:58:16 2012 +0400
@@ -3041,6 +3041,7 @@
char_u *name;
int n;
char_u *p;
+ size_t psize;
char_u *buffer;
size_t len;

@@ -3052,13 +3053,17 @@
{
vim_snprintf((char *)buffer, IOSIZE, "buf %d: ", curbuf->b_fnum);
p = buffer + STRLEN(buffer);
+ psize = IOSIZE - (p-buffer);
}
else
+ {
p = buffer;
+ psize = IOSIZE;
+ }

*p++ = '"';
if (buf_spname(curbuf) != NULL)
- STRCPY(p, buf_spname(curbuf));
+ STRNCPY(p, buf_spname(curbuf), psize-1);
else
{
if (!fullname && curbuf->b_fname != NULL)

[ZyX]

пятница, 7 сентября 2012 г., 0:04:00 UTC+4 пользователь ZyX написал:

> среда, 5 сентября 2012 г., 15:31:07 UTC+4 пользователь Bram Moolenaar написал:
> > Patch 7.3.648
> > Problem: Crash when using a very long file name. (ZyX)
> > Solution: Properly check length of buffer space.
> > Files: src/buffer.c
>
> That actually did not fix the problem: gcc optimized build now behaves like non-optimized one, thats all. I still get a crash where I was getting it before (aurum, “AuDiff rev1 1 rev2 tip **”). And I still observe invalid number of lines (1041 lines with last one truncated).
>
> I don’t get invalid number of lines if I replace “expand("<amatch>")” with “bufname(+expand("<abuf>"))”. Doing the same in aurum does not fix the crash. Crash happens at the very end of “fileinfo” function from buffer.c. The following patch seems to fix the crash:

Note: it fixes the crash, not invalid behavior of “expand()”.

[ZyX]

Previous patch missed one length decrement and thus was invalid. There was also no need to introduce one new variable, here is the updated one. There is still one issue: at the end of :file outputs garbage is shown meaning that something is wrong. I currently do not know how to fix this or reproduce without using aurum.

# HG changeset patch
# User ZyX <kp-...@ya.ru>

# Date 1346991136 -14400
# Node ID d6b78f099bfb0b347935765f333cdfe4a5bf0e59
# Parent 1052677493beb941eab0d1e33d63c73ee4148350
Fixed handling of long paths in fileinfo.

diff -r 1052677493be -r d6b78f099bfb src/buffer.c

--- a/src/buffer.c Wed Sep 05 19:17:17 2012 +0200

+++ b/src/buffer.c Fri Sep 07 08:11:51 2012 +0400
@@ -3058,7 +3058,7 @@

*p++ = '"';
if (buf_spname(curbuf) != NULL)
- STRCPY(p, buf_spname(curbuf));

+ STRNCPY(p, buf_spname(curbuf), IOSIZE - (p-buffer) - 1);

[Bram Moolenaar]

ZyX wrote:

> > Patch 7.3.648
> > Problem: Crash when using a very long file name. (ZyX)
> > Solution: Properly check length of buffer space.
> > Files: src/buffer.c
>
> That actually did not fix the problem: gcc optimized build now behaves
> like non-optimized one, thats all. I still get a crash where I was
> getting it before (aurum, "AuDiff rev1 1 rev2 tip **"). And I still
> observe invalid number of lines (1041 lines with last one truncated).
>
> I don't get invalid number of lines if I replace "expand("<amatch>")"
> with "bufname(+expand("<abuf>"))". Doing the same in aurum does not
> fix the crash. Crash happens at the very end of "fileinfo" function
> from buffer.c. The following patch seems to fix the crash:

And:

> Previous patch missed one length decrement and thus was invalid. There
> was also no need to introduce one new variable, here is the updated
> one. There is still one issue: at the end of :file outputs garbage is
> shown meaning that something is wrong. I currently do not know how to
> fix this or reproduce without using aurum.

So what is the simplest way to reproduce this crash?


--
A computer programmer is a device for turning requirements into
undocumented features. It runs on cola, pizza and Dilbert cartoons.
Bram Moolenaar

[ZyX]

суббота, 8 сентября 2012 г., 0:00:19 UTC+4 пользователь Bram Moolenaar написал:

> > That actually did not fix the problem: gcc optimized build now behaves
> > like non-optimized one, thats all. I still get a crash where I was
> > getting it before (aurum, "AuDiff rev1 1 rev2 tip **"). And I still
> > observe invalid number of lines (1041 lines with last one truncated).
>
> > I don't get invalid number of lines if I replace "expand("<amatch>")"
> > with "bufname(+expand("<abuf>"))". Doing the same in aurum does not
> > fix the crash. Crash happens at the very end of "fileinfo" function
> > from buffer.c. The following patch seems to fix the crash:
>
> And:
>
> > Previous patch missed one length decrement and thus was invalid. There
> > was also no need to introduce one new variable, here is the updated
> > one. There is still one issue: at the end of :file outputs garbage is
> > shown meaning that something is wrong. I currently do not know how to
> > fix this or reproduce without using aurum.
>
> So what is the simplest way to reproduce this crash?

After the patch there is no crash anymore, just seeing garbage. Before it was crashing when I did

vim -c 'AuDidd rev1 1 rev2 tip **'
while current directory is inside aurum repository. The simplest way I know is to do

mkdir test && cd test
hg clone https://bitbucket.org/ZyX_I/frawor
hg clone https://bitbucket.org/ZyX_I/aurum
vim -u /dev/null -N --cmd 'let &rtp=getcwd()."/frawor,".getcwd()."/aurum"' \
-c 'cd aurum' \
-c 'AuDiff rev1 1 rev2 tip **'
I have no idea why initial “longfilenamebug.vim” file is not crashing: STRCPY that I replaced with STRNCPY must overwrite something in both cases (and in that file filename is much longer). Now if you do “:file” after this you will see garbage at the end.

I found another issue though: “python anything_that_throws_exception” is a crash. I don’t think it is related, will explore this now.

[ZyX]

I now understand the source of the garbage: STRNCPY is not adding NUL byte if it copied n bytes and there is no NUL in this n bytes. Everything was in the man, I just had to reread it.

# HG changeset patch
# User ZyX <kp-...@ya.ru>

# Date 1347095322 -14400
# Node ID 36b3878656262abb03c3cd98b116208569be58c4
# Parent 78b679bbe662f809323ea4127aa424627ab0d01d
Fixed garbage at the end of the message.

diff -r 78b679bbe662 -r 36b387865626 src/buffer.c
--- a/src/buffer.c Sat Sep 08 12:30:23 2012 +0400
+++ b/src/buffer.c Sat Sep 08 13:08:17 2012 +0400
@@ -3044,7 +3044,7 @@
char_u *buffer;
size_t len;

- buffer = alloc(IOSIZE);
+ buffer = alloc_clear(IOSIZE);
if (buffer == NULL)
return;

[Bram Moolenaar]

ZyX wrote:

> I now understand the source of the garbage: STRNCPY is not adding NUL
> byte if it copied n bytes and there is no NUL in this n bytes.
> Everything was in the man, I just had to reread it.

It might be easier to use vim_strncpy(). And vim_strcat() where
appropriate. These always add a NUL and check the size of the buffer.


--
Q: Is selling software the same as selling hardware?
A: No, good hardware is sold new, good software has already been used by many.

[ZyX]

суббота, 8 сентября 2012 г., 17:00:45 UTC+4 пользователь Bram Moolenaar написал:

> It might be easier to use vim_strncpy(). And vim_strcat() where
> appropriate. These always add a NUL and check the size of the buffer.

It is not needed: the following fixes garbage without using alloc_clear:

# HG changeset patch
# User ZyX <kp-...@ya.ru>

# Date 1348347789 -14400
# Node ID 4c1027d39c5857cfc87a1916785f2f73ad012399
# Parent 04592728474a35794ae6397a0b90f2989864fb66
Fix crash with long filenames, without alloc_clear

diff -r 04592728474a -r 4c1027d39c58 src/buffer.c
--- a/src/buffer.c Fri Sep 21 14:54:05 2012 +0200
+++ b/src/buffer.c Sun Sep 23 01:02:44 2012 +0400
@@ -3047,6 +3047,7 @@
buffer = alloc(IOSIZE);

if (buffer == NULL)
return;

+ buffer[IOSIZE] = NUL;

if (fullname > 1) /* 2 CTRL-G: include buffer number */
{
@@ -3058,7 +3059,7 @@

[Bram Moolenaar]

ZyX wrote:

> ԕÂДÁ, 8 ԅϔ҂ӑ 2012šÇ., 17:00:45 UTC+4 я͘ۏ؁Յ͘ Bram Moolenaar ρщԁÌ:

> > It might be easier to use vim_strncpy(). And vim_strcat() where
> > appropriate. These always add a NUL and check the size of the buffer.
>
> It is not needed: the following fixes garbage without using alloc_clear:

Thanks. I'll look into it this week.

--
If "R" is Reverse, how come "D" is FORWARD?