New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Weird statusline behaviour with airline and vertical split #858

Closed
raichoo opened this Issue Jun 17, 2014 · 24 comments

Comments

Projects
None yet
7 participants
@raichoo
Copy link
Contributor

raichoo commented Jun 17, 2014

Hi,

I'm observing an interesting effect with neovim and airline when having a vertically split window. The content of the statusline seems to repeat and leaks into the other window.

screenshot from 2014-06-17 11 45 17

I'm on commit d199d18

I cannot observe the same behavior in in a recent version of vim, so this looks like a neovim issue to me.

Kind regards,
raichoo

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 17, 2014

Hey @raichoo, thanks for reporting that issue.

I guess the quickest way to track it down is to use git bisect. Once you get the hang of it, it's a godsend. Do you know how to use it and if so would you like to try it in this case?

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Jun 17, 2014

@raichoo thanks for reporting

I believe the culprit must be in the win_redr_custom function, I'm gonna investigate

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Jun 17, 2014

I managed to reproduce the bug, and compared vim version of win_redr_custom with our own. It seems there's a problem with string length calculation. The following patch should fix it:

diff --git a/src/nvim/screen.c b/src/nvim/screen.c
index 0dab25a..bfb4ba7 100644
--- a/src/nvim/screen.c
+++ b/src/nvim/screen.c
@@ -5082,8 +5082,8 @@ win_redr_custom (

   /* Make all characters printable. */
   p = transstr(buf);
-  len = STRLCPY(buf, p, sizeof(buf));
-  len = (size_t)len < sizeof(buf) ? len : (int)sizeof(buf) - 1;
+  vim_strncpy(buf, p, sizeof(buf) - 1);
+  len = (int)STRLEN(buf);
   free(p);

   /* fill up with "fillchar" */

Anyone with a keen eye for bugs(cc @oni-link) willing to dig the real problem? The STRLCPY version seems correct to me!

@oni-link

This comment has been minimized.

Copy link
Contributor

oni-link commented Jun 17, 2014

Quick guess: vim_strncpy fills buf with NUL to the end. STRLCPY does not "clear" buf, so
for windows with small maxwidth the following while loop won't set enough fillchars to "clear" buf.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 17, 2014

This is one of those things @philix @dougschneider and I feared for when exchanging strncpy with strlcpy. strncpy fills the buffer with zeroes after the terminating NUL, strlcpy doesn't. I hope we didn't introduce more of that...

@philix

This comment has been minimized.

Copy link
Member

philix commented Jun 17, 2014

I think we should not merge the latest massive 'search & replace' PR that
uses strlcpy everywhere, to avoid nasty bugs like this.

We can start refactoring the vim_strncpy calls whenever we touch the code
and know exactly what it's doing.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 17, 2014

I think we should not merge the latest massive 'search & replace' PR that
uses strlcpy everywhere, to avoid nasty bugs like this.

Or perhaps introduce a grace period where we poison the strings, to rat them out quicker.

@dougschneider

This comment has been minimized.

Copy link
Contributor

dougschneider commented Jun 17, 2014

This is tricky.

At the very least I agree with poisoning as suggested by @aktau but a full revert might just be a safer option. It depends how long we expect side-effects to stick around and if we're okay with neovim (possibly) acting weird for a while.

I thought I'd investigated things thoroughly enough. Which means if we leave them for when we understand them better It'll still be a confusing/frustrating thing to deal with. That being said my understanding of vim is much more limited than any of you since I'm new to the project. I'll be happy with whatever the decision is. Regardless, from a beginner point of view I think entry-level needs to be assigned more strictly to issues, this is clearly not the best entry-level task (although it did force me to read lots of vim code; so that's a plus).

Doug

@justinmk justinmk added the bug label Jun 17, 2014

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 17, 2014

I'll strike mea culpa here. It was I who assigned entry-level to the issue without thinking clearly enough about the possible subtleties of the approach. Perhaps in my mind I envisioned Bram using strncpy as a length delimited strcpy as I would've done some time ago (before studying up on the different ways people have thought of dealing with C strings). It's clear now he didn't always.

I still believe that reliance on the zero filling behaviour of strncpy is not widespread, but it is indeed trickier than I suspected. Does anyone know of a tool with which we are able to mark memory regions and break when they are read before they are changed?

It any rate, I want to thank @dougschneider for his persistence and good work on this one. It's clear that it's pretty difficult to get this just right.

@philix

This comment has been minimized.

Copy link
Member

philix commented Jun 18, 2014

It was I who assigned entry-level to the issue without thinking clearly enough...

When I saw that you were going to assign that as an entry-level task I was worried that someone would replace all calls in one fell swoop, but imagined that if people approached it in baby steps it would be great for newcomers to get experience with the code.

By "baby steps" I mean: small enough changes that make it possible to the author and reviewers to check that there's no possibility of a bug due to reliance on the zero-filling behavior.

I still believe that reliance on the zero filling behaviour of strncpy is not widespread...

The problem is that we will never be sure unless we prove it for every callsite.

Does anyone know of a tool with which we are able to mark memory regions and break when they are read before they are changed?

The problem with this tool is that by being dynamic instead of static it won't be able to prove the total absence of bugs caused by strlcpy. This is like the old tests vs. formal proofs debate (Dijkstra).

@philix

This comment has been minimized.

Copy link
Member

philix commented Jun 18, 2014

@aktau think of hardcopy.c which we keep refactoring but never actually testing. Who's been printing anything from Neovim?

@philix

This comment has been minimized.

Copy link
Member

philix commented Jun 18, 2014

@aktau more bad news: garray also zero-fills when reallocating. Client code is sometimes double cautious (memset calls after ga_grow, NULL appending) and very likely reliant on zero-filling behavior intentionally or by accident (forgot to append NULL to a NULL-terminated garray, but nothing bad happens because reallocation has zero-filled it).

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 18, 2014

@aktau more bad news: garray also zero-fills when reallocating. Client code is sometimes double cautious (memset calls after ga_grow, NULL appending) and very likely reliant on zero-filling behavior intentionally or by accident (forgot to append NULL to a NULL-terminated garray, but nothing bad happens because reallocation has zero-filled it).

This is -- I'm afraid -- entirely accurate.

Perhaps something like Address Sanitizers's manual poisoning could give us a head start. The only thing we'd need is to only poison reads, not writes.

EDIT: I just realized that the difficult way in which I've been describing it is too complex. I said: mark the memory region after the copied string as forbidden for reads until there is a write. That's basically just declaring the memory uninitialized. Perhaps Address Sanitizer uses some sort of sentinel value we could manually mark somewhere?

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Jun 18, 2014

One option would be to introduce a xstrlcpy parameter to control the padding behavior. This has the advantage of letting us document when we actually want to clear the target buffer.

@philix

This comment has been minimized.

Copy link
Member

philix commented Jun 18, 2014

@tarruda That should be another function (xstrlcpyz is my suggestion) for
two reasons:

  • xstrlcpy is our implementation of BSD's strlcpy (changing its signature
    just complicate things)
  • boolean parameters are evil (a code smell that begs for a different
    function implementation)
    On Jun 18, 2014 7:18 AM, "Thiago de Arruda" notifications@github.com
    wrote:

One option would be to introduce a xstrlcpy parameter to control the
padding behavior. This has the advantage of letting us document when we
actually want to clear the target buffer.


Reply to this email directly or view it on GitHub
#858 (comment).

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 18, 2014

One option would be to introduce a xstrlcpy parameter to control the padding behavior. This has the advantage of letting us document when we actually want to clear the target buffer.

That would unfortunately break the strlcpy contract. However, I think I found a better solution: #743 (comment) (https://groups.google.com/forum/#!forum/address-sanitizer)

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jun 19, 2014

@raichoo Would you please try #866 to see if it fixes your issue?

@raichoo

This comment has been minimized.

Copy link
Contributor

raichoo commented Jun 19, 2014

@justinmk Yep, this seems to fix it.

@oni-link

This comment has been minimized.

Copy link
Contributor

oni-link commented Jun 19, 2014

Quick guess: vim_strncpy fills buf with NUL to the end. STRLCPY does not "clear" buf, so
for windows with small maxwidth the following while loop won't set enough fillchars to "clear" buf.

On a second look, the while loop has nothing to do with the problem for me.

The problem is located in the snippet drawing with highlights (the for loop), specificly the start position p (points into buf) of the last snippet(is it a dummy?) that can be greater than strlen(buf). The drawing of this snippet is special, because the length is not known, so it is drawn unitl the next NUL in buf (see screen_puts and screen_puts_len). The original vim code fills buf with NULs, which makes every snippet outside of strlen(buf) an empty string and screen_puts draws nothing. For our code these strings have random length and something is drawn.

This patch seems to fix the problem for me:

diff --git a/src/nvim/screen.c b/src/nvim/screen.c
index b3d6288..a9d6db3 100644
--- a/src/nvim/screen.c
+++ b/src/nvim/screen.c
@@ -5099,7 +5099,7 @@ win_redr_custom (
   curattr = attr;
   p = buf;
   for (n = 0; hltab[n].start != NULL; n++) {
-    len = (int)(hltab[n].start - p);
+    int len = (int)(hltab[n].start - p);
     screen_puts_len(p, len, row, col, curattr);
     col += vim_strnsize(p, len);
     p = hltab[n].start;
@@ -5113,7 +5113,8 @@ win_redr_custom (
     else
       curattr = highlight_user[hltab[n].userhl - 1];
   }
-  screen_puts(p, row, col, curattr);
+  // Make sure to use an empty string instead of p, if p is beyond buf + len.
+  screen_puts(p >= buf + len ? (char_u *)"" : p, row, col, curattr);

   if (wp == NULL) {
     /* Fill the TabPageIdxs[] array for clicking in the tab pagesline. */
@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jun 19, 2014

@oni-link Nice. I was focused on screen_puts_len and assumed that screen_puts only put one char, but looking at its impl now, it's clear that it prints until it reaches NUL (-1 means "stop at NUL"):

void screen_puts(char_u *text, int row, int col, int attr)
{
  screen_puts_len(text, -1, row, col, attr);
}

So actually, we don't need to call screen_puts at all if p >= buf + len:

diff --git a/src/nvim/screen.c b/src/nvim/screen.c
index b3d6288..a9d6db3 100644
--- a/src/nvim/screen.c
+++ b/src/nvim/screen.c
@@ -5099,7 +5099,7 @@ win_redr_custom (
   curattr = attr;
   p = buf;
   for (n = 0; hltab[n].start != NULL; n++) {
-    len = (int)(hltab[n].start - p);
+    int len = (int)(hltab[n].start - p);
     screen_puts_len(p, len, row, col, curattr);
     col += vim_strnsize(p, len);
     p = hltab[n].start;
@@ -5113,7 +5113,8 @@ win_redr_custom (
     else
       curattr = highlight_user[hltab[n].userhl - 1];
   }
-  screen_puts(p, row, col, curattr);
+  // if p is beyond buf + len, window is not wide enough.
+  if (p < buf + len) {
+    screen_puts(p, row, col, curattr);
+  }

   if (wp == NULL) {
     /* Fill the TabPageIdxs[] array for clicking in the tab pagesline. */

justinmk added a commit to justinmk/neovim that referenced this issue Jun 19, 2014

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jun 19, 2014

@raichoo @tarruda Updated patch here: #866

@oni-link

This comment has been minimized.

Copy link
Contributor

oni-link commented Jun 19, 2014

@justinmk, I was not sure what other side effects a call to screen_puts could have. So I did not made
the call to screen_puts conditional.

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jun 19, 2014

@oni-link Agree, but screen_puts_len bails if *text (*ptr) is NUL:

  while (col < screen_Columns
         && (len < 0 || (int)(ptr - text) < len)
         && *ptr != NUL) {
@oni-link

This comment has been minimized.

Copy link
Contributor

oni-link commented Jun 19, 2014

@justinmk, there are side effects that are independent from text and could happen before and after the while loop in screen_puts_len: Modify some global variables (ScreeLines, ScreenAttr) and even draw a character (screen_char).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment