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

[RDY] Replace vim_strncpy with strlcpy #743

Closed
wants to merge 29 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@dougschneider
Copy link
Contributor

dougschneider commented May 21, 2014

No description provided.

@jamessan

This comment has been minimized.

Copy link
Member

jamessan commented May 22, 2014

Is it guaranteed that all calls to strncpy in Vim don't involve overlapping memory? If not, then memmove should be used instead of memcpy.

@philix

This comment has been minimized.

Copy link
Member

philix commented May 22, 2014

@jamessan I expect strncpy and strlcpy to have memcpy semantics. Keeping the same semantics is guaranteed to not add any new bugs. What worried me is that he started with vim_strncpy which is very different from strncpy.

@philix

This comment has been minimized.

Copy link
Member

philix commented May 22, 2014

@dougschneider please don't delete vim_strncpy. It's very tricky to replace it even with strncpy:

  • vim_strncpy uses the char_u* types -- we are not replacing vim_strncpy with calls to other functions and a lot of casts to char*.
  • vim_strncpy and strncpy's behaviors are different

A general rule for commits in any project: try not to break the build between commits.

By removing vim_strncpy you're required to remove all occurrences of vim_strncpy until you can build nvim again. This is really bad, for two reasons: you won't be able to test your refactorings until you remove all calls to vim_strncpy, you may realize you should not remove vim_strncpy anyways which is the case here.

Instead you can:

  • add xstrlcpy to the codebase
  • start analyzing calls to vim_strncpy that can actually be replaced by xstrlcpy
  • call this PR done
  • delete vim_strncpy someday when it's actually not used anymore
@dougschneider

This comment has been minimized.

Copy link
Contributor

dougschneider commented May 22, 2014

@philix A couple of questions. I changed the definition of xstrlcpy to be:

size_t xstrlcpy(char_u* restrict dst, const char_u* restrict src, size_t size);

Perhaps this was the wrong thing to do and if so, let me know. As such the char_u casts aren't necessary anymore.

Locally I've already changed all of the calls from vim_strncpy to xstrlcpy. I went through a lot of the code trying to find errors that could be caused by the fact that vim_strncpy pads the string with NUL's and xstrlcpy doesn't. I didn't see any glaring issues. In addition to account for the placement of the NUL character I added one to the length of each xstrlcpy's length. Upon completing the transition I ran the tests and there are no failing tests caused by the change. Of course there might be issues not covered by the tests.

For this reason I think the transition from vim_strncpy to xstrlcpy can be done smoother than we'd expect. That being said I can go through the code more thoroughly and make sure things should work as expected. In addition to committing the code in smaller chunks.

Your thoughts?

Doug

@philix

This comment has been minimized.

Copy link
Member

philix commented May 23, 2014

I changed the definition of xstrlcpy to be:

size_t xstrlcpy(char_u* restrict dst, const char_u* restrict src, size_t size);

Please don't do this. xstrlcpy should be just like strlcpy. It's OK to leave vim_strncpy still in the codebase.

Locally I've already changed all of the calls from vim_strncpy to xstrlcpy. I went through a lot of the code trying to find errors that could be caused by the fact that vim_strncpy pads the string with NUL's and xstrlcpy doesn't. I didn't see any glaring issues. In addition to account for the placement of the NUL character I added one to the length of each xstrlcpy's length. Upon completing the transition I ran the tests and there are no failing tests caused by the change. Of course there might be issues not covered by the tests.

In vim_strncpy the buffer size for to needs to be len + 1 bytes and the NUL byte is placed at the len index. In contrast, strlcpy takes a buffer of size len and places the NUL at the len - 1 index [1]. An example from the man:

     To detect truncation, perhaps while building a pathname, something like
     the following might be used:

       char *dir, *file, pname[MAXPATHLEN];

       if (strlcpy(pname, dir, sizeof(pname)) >= sizeof(pname))
           goto toolong;
       if (strlcat(pname, file, sizeof(pname)) >= sizeof(pname))
           goto toolong;

[1] http://www.freebsd.org/cgi/man.cgi?query=strlcpy&sektion=3

@philix

This comment has been minimized.

Copy link
Member

philix commented May 23, 2014

You may define a STRLCPY like the STR* macros that avoid the char* casts and replace vim_strncpy(dst, src, len) with STRLCPY(dst, src, len + 1). If this is done carefully we will be able to assume that the code doesn't rely on the zero-padding behavior of strncpy later on.

But I still insist that you keep vim_strncpy around until you refactor all calls file by file (that would make the review easier).

@aktau

This comment has been minimized.

Copy link
Member

aktau commented May 23, 2014

We could try "poisoning" the string in STRLCPY by filling the buffer after the \0 (strlen(src)) with @ characters or something and specifically not NULL-terminating buf[len] (even if it was already), that should hopefully catch any wrong accesses. I notice that OSX doesn't reliably detect this, but glibc's/linux seem to be a bit more strict about this (though I'm not sure if it's guaranteed).

size_t xstrlcpy(char_u* restrict dst, const char_u* restrict src, size_t size);

As @philix said, let's stick with the standard prototype, and create a macro if we must.

What worried me is that he started with vim_strncpy which is very different from strncpy.

Not that different, it just guarantees NULL-termination (i.e.: truncation) like strlcpy but automatically adds +1 to the last argument. It's a logical "fix" for strncpy's asinine behaviour (though the +1 isn't). The thing that worries me is if Bram "forgot" to remove the zero-padding requirement, or actively used it.

A case analysis:

- len(dst) > strlen(src): 

strncpy:     SSSS000000
strlcpy:     SSSS0XXXXX
vim_strncpy: SSSS000000

- len(dst) == strlen(src): 

strncpy:     SSSSSSSSSSSS
strlcpy:     SSSSSSSSSSS0
vim_strncpy: SSSSSSSSSSS0

- len(dst) < strlen(src): 

strncpy:     SSSSSSSSSSSS
strlcpy:     SSSSSSSSSSS0
vim_strncpy: SSSSSSSSSSS0

This is of course accounting for the +1 in the buffer length.

@dougschneider dougschneider changed the title [WIP] Replace strncpy/vim_strncpy/STRNCPY with strlcpy [RFC] Replace strncpy/vim_strncpy/STRNCPY with strlcpy May 30, 2014

@dougschneider

This comment has been minimized.

Copy link
Contributor

dougschneider commented May 30, 2014

@philix I've changed all the files from vim_strncpy to STRLCPY except for eval.c and message.c

These two files break the travis build on the clang/asan and api/python tests. Looking at the error messages I think the issue might be in the tests. I'm not 100% certain but I believe the STRLCPY function is being called with an unallocated char* so this is something that needs to be looked into.

The issue this PR relates to (#731) relates to fixing all of vim_strncpy/STRNCPY/strncpy but to make this more review-able I'm going to leave this PR as just vim_strncpy, and start working on a second and third branch with their own PR's for the other two.

@philix

This comment has been minimized.

Copy link
Member

philix commented May 30, 2014

These two files break the travis build on the clang/asan and api/python tests. Looking at the error messages I think the issue might be in the tests. I'm not 100% certain but I believe the STRLCPY function is being called with an unallocated char* so this is something that needs to be looked into.

Probably not as this would blow up vim_strncpy too. It may be something related to not zero-filling the string anymore.

@dougschneider

This comment has been minimized.

Copy link
Contributor

dougschneider commented May 30, 2014

@philix

That's what I thought to at first. However it's failing on the following line of xstrlcpy:

size_t ret = strlen(src);

With an invalid read of (I believe) size 1. So if just getting the size of the string causes an invalid read I'd say it's a memory allocation problem. Furthermore if we look at vim_strncpy code, if you pass in an unallocated string, with say size 0, then vim_strncpy will have no issues. It will just copy nothing over and place a nul in the 0th position in the destination. This is of course assuming strncpy's behaviour is indeed equivalent to the contents of the man page:

       char *
       strncpy(char *dest, const char *src, size_t n)
       {
           size_t i;

           for (i = 0; i < n && src[i] != '\0'; i++)
               dest[i] = src[i];
           for ( ; i < n; i++)
               dest[i] = '\0';

           return dest;
       }

Doug

@philix

This comment has been minimized.

Copy link
Member

philix commented May 30, 2014

@dougschneider there's still a chance of someone passing a pointer that points to the tail of a string. Now that it's not zero-filled, strlen reads after the end of allocated space as doesn't find a NUL to stop.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented May 31, 2014

@dougschneider there's still a chance of someone passing a pointer that points to the tail of a string. Now that it's not zero-filled, strlen reads after the end of allocated space as doesn't find a NUL to stop.

This is indeed a case where strncpy and strlcpy are markedly different, and one would only notice "downstream" from the offending call to strlcpy. We would do better to fix these assumptions rather than stick with strncpy for that codepath. I believe valgrind might be a big help in getting to the bottom of this quickly.

@dougschneider

This comment has been minimized.

Copy link
Contributor

dougschneider commented Jun 2, 2014

@philix

Looks like we're both wrong. The issue is that in the msgpack_rpc_to_string function (os/msgpack_rpc.c:160) the string is extracted from the message pack using xmemdup. The problem is that the given string is the exact length of the given length. Meaning the new memory has no NUL character at the end. This is fine with vim_strncpy because it copies the string over fine and a NUL is added to the result. But with xstrlcpy, it runs strlen before performing the copy. And since there is no NUL character there is an invalid read.

I tested this by replacing the call to xmemdup with:

arg->data = xmalloc(obj->via.raw.size + 1);
memcpy(arg->data, obj->via.raw.ptr, obj->via.raw.size);
arg->data[obj->via.raw.size] = NUL;

I don't know if this is the fix we're looking for, but it gets rid of all the errors caused by replacing vim_strncpy with xstrlcpy in eval.c.

Can you comment on what the next action is? Should we leave the call to vim_strncpy and add an issue to fix this? Or should we change the call from vim_strncpy to a memcpy call with a NUL character being appended? Or what?

Thanks,
Doug

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 2, 2014

@dougschneider

I tested this by replacing the call to xmemdup with: ...

That will be fixed by #783 (basically using xmemdupz instead of xmemdup, thereby NUL-terminating).

There is one little things that bothers me, and that's the fact that xstrlcpy always runs a full strlen (so that it can return it's return value of course). This implies two things:

  • xstrlcpy can't work with Pascal strings (as we've noticed, that's the reason for the errors you've been getting)
  • if we use xstrlcpy on large strings (of which we already know the string length, presumably), we're leaving quite some performance on the table. I'm thinking of the pathological case of opening huge files.

Your thoughts?

@dougschneider

This comment has been minimized.

Copy link
Contributor

dougschneider commented Jun 2, 2014

@aktau

For your first point: I'm not familiar with the problem at hand. I assume what you're saying is that we got a pascal string in through the rpc. And as a result it's not NUL terminated. I assume that there are limited ways for Pascal strings to enter the system. If this is the case, it would be ideal to make sure that all of those strings have a NUL character appended to them when they enter the system. That way we can guarantee that everything inside of vim has a NUL character. Is this unreasonable?

For your second point: We could change xstrlcpy to copy the char* over character by character and stop copying when either the length is reached or the NUL character is reached. It could then count the remainder of the string. This would reduce the number of times the string is gone through. But I assume that memcpy is substantially faster than doing a single character at a time. So this is likely to reduce the efficiency.

I don't know a lot of the internals of vim yet, however I assume files are loaded using a buffer which means we control the max size of a files string at a time. Furthermore, I assume strlen operates at O(n) where n is the number of characters in the string. So even with large strings I don't think xstrlcpy will be much of an efficiency hog. Though we could always profile it.

Doug

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 2, 2014

For your first point: I'm not familiar with the problem at hand. I assume what you're saying is that we got a pascal string in through the rpc. And as a result it's not NUL terminated. I assume that there are limited ways for Pascal strings to enter the system. If this is the case, it would be ideal to make sure that all of those strings have a NUL character appended to them when they enter the system. That way we can guarantee that everything inside of vim has a NUL character. Is this unreasonable?

This is exactly what is being fixed in issue #783 (which has the convenient title "Nul terminate pascal strings" ;).

For your second point: We could change xstrlcpy to copy the char* over character by character and stop copying when either the length is reached or the NUL character is reached. It could then count the remainder of the string. This would reduce the number of times the string is gone through. But I assume that memcpy is substantially faster than doing a single character at a time. So this is likely to reduce the efficiency.

No we can't, that would break the (x)strlcpy contract and make us just as bad as vim_strncpy was. I agree with @philix here (and on many other accounts, but especially here). The return value of (x)strlcpy is strlen(src), so one cannot pass a Pascal string in and one cannot skip scanning over the entire source string.

I don't know a lot of the internals of vim yet, however I assume files are loaded using a buffer which means we control the max size of a files string at a time. Furthermore, I assume strlen operates at O(n) where n is the number of characters in the string. So even with large strings I don't think xstrlcpy will be much of an efficiency hog. Though we could always profile it.

In itself, strlen() is usually a very, very optimized operation (it's also O(n), as you noted). Usually reading 16 bytes at a time through SIMD operations (or even 32 bytes at a time if a libc has bothered to have a special AVX implementation). So no, it doesn't have a huge overhead per-se. But, strings are the bread and butter of vim, strings, strings, strings. So we should think long and hard about how to do it in a safe and performant manner, and not repeat calculations needlessly.

That said, profiling is always best.

@dougschneider

This comment has been minimized.

Copy link
Contributor

dougschneider commented Jun 2, 2014

@aktau

By no means was I recommending that solution but I'm still gonna note any options we have =P

It sounds like the pascal string issue isn't a problem that we need to deal with then. Do we have a current method of testing efficiency? I'm honestly not concerned but to be thorough I'd be happy to push it to it's limits if you think it's needed.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 2, 2014

It sounds like the pascal string issue isn't a problem that we need to deal with then. Do we have a current method of testing efficiency? I'm honestly not concerned but to be thorough I'd be happy to push it to it's limits if you think it's needed.

I think we can take a middle ground here. Once done, we can add a bit of temporary code to xtrlcpy. It will use @philix's DLOG to print a statement every time xstrlcpy is called:

size_t xstrlcpy(char *restrict dst, const char *restrict src, size_t size)
{
    size_t ret = strlen(src);

    if (size) {
        size_t len = (ret >= size) ? size - 1 : ret;
        memcpy(dst, src, len);
        dst[len] = '\0';
    }

    DLOG("ratio: (%zu/%zu) %f", ret, size, (double) ret / (double) size);

    return ret;
}

That way we can see if there's lots of strings being copied around vim that are larger than their destination. xstrlcpy truncates, but that's not what it does best.

That should be a sufficient ghetto benchmark.

@oni-link

This comment has been minimized.

Copy link
Contributor

oni-link commented Jun 2, 2014

For your second point: We could change xstrlcpy to copy the char* over character by character and stop copying when either the length is reached or the NUL character is reached. It could then count the remainder of the string. This would reduce the number of times the string is gone through. But I assume that memcpy is substantially faster than doing a single character at a time. So this is likely to reduce the efficiency.

Sounds like @dougschneider described the FreeBSD version of strlcpy (mentioned by @aktau
here #731 (comment)).

@philix

This comment has been minimized.

Copy link
Member

philix commented Jun 3, 2014

I was sure that I would see bugs related to non-NUL-terminated pascal
strings, but I didn't think they would appear so early. Great job on #783
@aktau.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 3, 2014

Sounds like @dougschneider described the FreeBSD version of strlcpy (mentioned by @aktau
here #731 (comment)).

Unfortunately he didn't. The FreeBSD version is bound by the same contract and also returns strlen(str). Thus, it also has to traverse the entire source string. All versions of strlcpy are equivalent in this regard.

The problems as I see them:

  • strncpy is unsafe, and wasteful if buflen(dst) >> buflen(src), which I imagine happens a lot
  • vim_strncpy is safe, and wasteful if buflen(dst) >> buflen(src), which I imagine happens a lot
  • strlcpy is safe, and wasteful if buflen(dst) << buflen(src), which I don't know if it happens a lot (which is why I suggested the ghetto benchmark)

For now, strlcpy still seems like the right alternative to me. I've been thinking about a non-wasteful copy that perhaps still returns something useful. Perhaps something like the return value of stpncpy that doesn't fill the rest up with NUL's. This would merely require an alternative version of strpncpy with one memset removed.

The only sad thing, is that the optimal implementation of such a thing won't come for free. The memchr + memcpy trick is cute, but it does 2 passes through the data. So we still have a factor of 2 inefficiency. But I can live with that if that means we don't have any pathological cases.

UPDATE: perhaps the strncat function would serve for this, it seems to always NUL-terminate at least... http://pubs.opengroup.org/onlinepubs/009604599/functions/strncat.html (it has efficient, one-pass, asm implementations in glibc as well, which is a big relief, have to study it though).

UPDATE: we might also consider using the memccpy function to implement this new string copying function, since it supposedly folds memchr and memcpy into one (correct me if I'm wrong). Sadly, the version in glibc is not optimized and is in fact exactly the same as what we would come up with: http://pubs.opengroup.org/onlinepubs/000095399/functions/memccpy.html

UPDATE: interesting article of someone who has been thinking of the same "problems" (efficiency and correctness of string copying): http://meyering.net/crusade-to-eliminate-strncpy/

@dougschneider

This comment has been minimized.

Copy link
Contributor

dougschneider commented Jun 5, 2014

@aktau

All your requested changes are in now. The build should fail until #783 is in. At that point I can rebase and put up another changeset.

@dougschneider

This comment has been minimized.

Copy link
Contributor

dougschneider commented Jun 5, 2014

@aktau

Also I was getting an error in the strlcpy of message.c due to uninitialized memory. I've initialized the memory in this commit:

dougschneider@0c37b72

But I'm not sure if we want to have another issue for that fix, or if that's the proper fix.

Doug

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 5, 2014

Also I was getting an error in the strlcpy of message.c due to uninitialized memory. I've initialized the memory in this commit:

Fantastic job @dougschneider. I don't actually see a xstrlcpy call in the neighbourhood of the memory you initialized. Which one was it and how did you find the correct one?

@dougschneider

This comment has been minimized.

Copy link
Contributor

dougschneider commented Jun 5, 2014

@aktau

While running the python api tests I got the following from valgrind:

==32472== Conditional jump or move depends on uninitialised value(s)
==32472== at 0x4C2E0F8: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32472== by 0x571AC5: xstrlcpy (memory.c:287)
==32472== by 0x4F9761: store_sb_text (message.c:1841)
==32472== by 0x4F94C2: msg_puts_display (message.c:1758)
==32472== by 0x4F8D8E: msg_puts_attr_len (message.c:1573)
==32472== by 0x4F81C1: msg_outtrans_len_attr (message.c:1182)
==32472== by 0x4F7ECC: msg_outtrans_len (message.c:1095)
==32472== by 0x4D9186: draw_cmdline (ex_getln.c:2094)
==32472== by 0x4D96C8: put_on_cmdline (ex_getln.c:2222)
==32472== by 0x4D7D0B: getcmdline (ex_getln.c:1350)
==32472== by 0x4D864D: getexline (ex_getln.c:1706)
==32472== by 0x5CE4D5: do_cmdline (ex_docmd.c:561)
==32472== Uninitialised value was created by a heap allocation
==32472== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32472== by 0x5716EE: try_malloc (memory.c:78)
==32472== by 0x57179E: xmalloc (memory.c:119)
==32472== by 0x4D8F28: draw_cmdline (ex_getln.c:2039)
==32472== by 0x4D96C8: put_on_cmdline (ex_getln.c:2222)
==32472== by 0x4D7D0B: getcmdline (ex_getln.c:1350)
==32472== by 0x4D864D: getexline (ex_getln.c:1706)
==32472== by 0x5CE4D5: do_cmdline (ex_docmd.c:561)
==32472== by 0x588FB3: nv_colon (normal.c:4073)
==32472== by 0x582647: normal_cmd (normal.c:907)
==32472== by 0x5152E3: main_loop (main.c:742)
==32472== by 0x514CFD: main (main.c:525)

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 5, 2014

Now that's good information :). The offending line is (as valgrind reports):

// vim_strncpy(mp->sb_text, *sb_str, s - *sb_str);
xstrlcpy(mp->sb_text, *sb_str, s - *sb_str);

This is probably one of those cases where vim_strncpy was being used as a glorified memcpy. *sb_str is most likely a Pascal string here (but produced by vim core, not the API). s - *sb_str means taking a substring. It is also one of those pathological cases I feared for, where the source buffer is much larger than the length which needs to be copied. Even if this would have worked, it would be very inefficient with xstrlcpy.

So, xstrlcpy should be used with actual C strings and if the length of the source is not known. In this case, it seems like the length is known exactly (s - *sb_str) so we can use a NUL-terminating memcpy. It doesn't seem like we have a good function in memory.c right now that does the minimum required. It would be something like:

Note the definition of msgchunk_T.

struct msgchunk_S {
  msgchunk_T  *sb_next;
  msgchunk_T  *sb_prev;
  char sb_eol;                  /* TRUE when line ends after this text */
  int sb_msg_col;               /* column in which text starts */
  int sb_attr;                  /* text attributes */
  char_u sb_text[1];            /* text to be displayed, actually longer */
};

We have a few options:

mp = xmalloc((sizeof(msgchunk_T) + (s - *sb_str)));
mp->sb_eol = finish;
mp->sb_msg_col = *sb_col;
mp->sb_attr = attr;
// correct and inefficient, we know for sure what the length of the source buffer is, and even if there was a '\0' 
// between *sb_str and s, vim_strncpy would just fill up the rest of mp->sb_text with 0's
vim_strncpy(mp->sb_text, *sb_str, s - *sb_str);

// incorrect and inefficient, we know the length of the source and destination, yet xstrlen 
// always tries `strlen(src)`, in this case that's especially bad because src is not even NUL-terminated
xstrlcpy(mp->sb_text, *sb_str, s - *sb_str);

// correct and efficient, but ugly. Note that this is slightly different from the vim_strncpy case, we might 
// be copying more non-0 bytes after the first 0. That shouldn't matter though. Note that there's 
// an extra 1 byte we can use for NULL-termination by virtue of the struct definition. I don't know if 
// we should use that, though.
memcpy(mp->sb_text, *sb_str, s - *sb_str) + (s - *sb_str) = NUL;

// correct and efficient, slightly less ugly. Same as above.
xmempcpy(mp->sb_text, *sb_str, s - *sb_str) = NUL;

// some new function which should be correct and cleanest? Equivalent to the two above but looks better.
xstrzcpy(mp->sb_text, *sb_str, s - *sb_str);

I also find it very, very strange that arabic shaping is being invoked here, the p_arshape option needs to be true for that. I wonder if that's the default... Perhaps we should note that down for later rearchitecting. I'm doubting to turn it in an xrealloc call, obviously the memory it contained is no longer necessary so the implicit memcpy that xrealloc does is no longer necessary, this might be an optimization for a large buffer...

As for the name of this new function, if we are making one. There are some precedents of functions that all do slightly different things:
strzcpy: https://lists.gnu.org/archive/html/qemu-devel/2012-05/msg01192.html
strzcpy: https://gitorious.org/tmw-eathena/tmw-eathena-comments/commit/9e23d66f9e169f299c209bf5a7a0223cefb51128
strzcpy: http://www.conetic.com/helpdoc/manpage/strbcpy.C.3.html

EDIT:

Now this is interesting:

stracpy: http://stracpy.blogspot.be/

@dougschneider

This comment has been minimized.

Copy link
Contributor

dougschneider commented Jun 5, 2014

@aktau

There is some validity in making a new function for memcpying strings and appending the NUL as I've seen a fair bit of this in the code. But I'm not sure if this changeset is the best place to do that as I'd like to keep it to a maintainable size. I think it'd be worth considering the introduction of another function and if we want to go that route, perform a sweeping change to fix it all.

But for now I think it's fine to use something like:

memcpy(mp->sb_text, *sb_str, s - *sb_str);                                     
mp->sb_text[s - *sb_str] = NUL;

As in this context it's quite obvious what is going on. I'm going to remove the commit I made changing malloc to calloc as with this addition the code passes the build (minus #783) just fine. And I think the calloc call is inefficient and unnecessary.

Newest code will be up in a minute.

@dougschneider

This comment has been minimized.

Copy link
Contributor

dougschneider commented Jun 9, 2014

@aktau @philix

I just pushed the changes rebased onto #783 and the build is passing.

Any more comments before this can be changed to RDY?

@dougschneider dougschneider changed the title [RFC] Replace vim_strncpy with strlcpy [RDY] Replace vim_strncpy with strlcpy Jun 12, 2014

@@ -5083,11 +5083,11 @@ win_redr_custom (

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

This comment has been minimized.

@justinmk

justinmk Jun 13, 2014

Member

Does anyone else find this pattern too confusing, just to save a couple lines? I've seen it in other commits, maybe it's an idiom that I need to get used to.

This comment has been minimized.

@aktau

aktau Jun 14, 2014

Member

The problem here is (I think) that we should basically have a min/max function/macro. That would save lines and be much more readable.

This comment has been minimized.

@justinmk

justinmk added a commit that referenced this pull request Jun 13, 2014

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jun 13, 2014

Merged. Thanks!

@justinmk justinmk closed this Jun 13, 2014

@elmart

This comment has been minimized.

Copy link
Member

elmart commented Jun 16, 2014

This was good work. And it's already merged now. But, IMO, and just for the record, I think this shouldn't be 29 commits, but 1. Splitting changes to each individual file doesn't have any added value, if you are doing conceptually the same thing to each of them. It can in fact be harder to review that way (more navigation in and out of commits).

@dougschneider

This comment has been minimized.

Copy link
Contributor

dougschneider commented Jun 16, 2014

@elmart

I guess maybe it's all point of view. I split it up due to @philix 's comment:

But I still insist that you keep vim_strncpy around until you refactor all calls file by file (that would make the review easier).

Maybe I interpreted that comment incorrectly, but I interpreted it as 'break each file into a commit'. However if there's a strong preference, the next set of changes is in #851 So I can do that there (but only if it's requested).

Doug

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jun 16, 2014

Agreed with @elmart, commits should be separated conceptually/logically. No worries though.

@philix

This comment has been minimized.

Copy link
Member

philix commented Jun 17, 2014

@dougschneider I was assuming we would be more careful in the use of
strlcpy. If you're performing a full search & replace there's no possible
file-by-file review for that.

@aktau aktau referenced this pull request Jun 17, 2014

Open

string handling, SDS #859

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 18, 2014

Good news guys! I went searching for a solution and it seems we might have something:

https://groups.google.com/forum/#!topic/address-sanitizer/BCIpilSw7us

We can manually call __msan_poison or __msan_partial_poison in xstrlcpy to detect these cases. It would also work for your garray reworks I think, @philix.

Best to use the very latest llvm/clang available (3.4.1), as we know they're heavily working on it and even 3.2 had some non-deterministic finds.

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