[vim/vim] Increase buffer size for tag name (#8769)
Gregory Anders
100 bytes is too small in some extreme cases. We can more than double it to be safe and still be well within a reasonable size.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/8769
Commit Summary
- Increase buffer size for tag name
File Changes
- M src/tag.c (2)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
codecov[bot]
Codecov Report
Merging #8769 (0b1c650) into master (dea5611) will decrease coverage by
87.69%.
The diff coverage isn/a.
@@ Coverage Diff @@ ## master #8769 +/- ## =========================================== - Coverage 90.15% 2.46% -87.70% =========================================== Files 151 149 -2 Lines 170685 165430 -5255 =========================================== - Hits 153883 4072 -149811 - Misses 16802 161358 +144556
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | ? |
|
| huge-gcc-none | ? |
|
| huge-gcc-testgui | ? |
|
| huge-gcc-unittests | 2.46% <ø> (ø) |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/tag.c | 0.00% <ø> (-93.96%) |
⬇️ |
| src/float.c | 0.00% <0.00%> (-99.22%) |
⬇️ |
| src/gui_gtk_f.c | 0.00% <0.00%> (-97.54%) |
⬇️ |
| src/crypt_zip.c | 0.00% <0.00%> (-97.06%) |
⬇️ |
| src/match.c | 0.00% <0.00%> (-96.98%) |
⬇️ |
| src/sha256.c | 0.00% <0.00%> (-96.94%) |
⬇️ |
| src/evalbuffer.c | 0.00% <0.00%> (-96.92%) |
⬇️ |
| src/textprop.c | 0.00% <0.00%> (-96.85%) |
⬇️ |
| src/cmdhist.c | 0.00% <0.00%> (-96.76%) |
⬇️ |
| src/debugger.c | 0.00% <0.00%> (-96.62%) |
⬇️ |
| ... and 138 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update dea5611...0b1c650. Read the comment docs.
Ben Reeves
I'm not familiar with the development practices here, but this still seems like a temporary solution. Seems to me that what this really needs is additional checks to make sure it does not buffer overflow.
Could we simply make a check that (int)(t_p.tagname_end - t_p.tagname) + 3 <= 256?
Gregory Anders
I'm not familiar with the development practices here, but this still seems like a temporary solution. Seems to me that what this really needs is additional checks to make sure it does not buffer overflow. I don't want my (neo)vim crashing no matter what pathological case it encounters.
Could we simply make a check that
(int)(t_p.tagname_end - t_p.tagname) + 3 <= 256?
You're right, simply increasing the buffer size is just kicking the can down the road, so adding a check is probably the right thing to do. I'm not sure exactly what to do if the check fails though. Truncating the tag name isn't helpful. I suppose we could show a "Name too long" error, but that isn't exactly clear either.
Gregory Anders
@gpanders pushed 1 commit.
- f347578 Increase buffer size for tag name
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or unsubscribe.
Gregory Anders
@gpanders pushed 1 commit.
- a633725 Increase buffer size for tag name
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or unsubscribe.
Bram Moolenaar
If you have a tag name of more than 100 characters I'm not sure what you are doing. However, I suppose it's actually 100 bytes, and some languages these days support using Unicode text for keywords. So it would be 100 characters of about 6 bytes.
A fixed buffer is just more efficient and easier to deal with. We could make the length dynamic, but then we need to pass it around, it's no longer constant. The efficiency might not be so important these days, but I like to keep the code simple.
Gregory Anders
A fixed buffer is just more efficient and easier to deal with. We could make the length dynamic, but then we need to pass it around, it's no longer constant. The efficiency might not be so important these days, but I like to keep the code simple.
I agree and I think 256 bytes on a tag name is a pretty generous upper bound. Even if this bound is met we're at least now just issuing a (albeit cryptic) error message rather than crashing due to a buffer overflow.
Ben Reeves
If you have a tag name of more than 100 characters I'm not sure what you are doing
We have some very long symbol names for generated code where I work. Doesn't matter if it's weird, it shouldn't crash my neovim 🤷
Not to mention the security concerns associated with unbounded memory copies into stack-allocated buffers.
Ben Reeves
In the (so-claimed) pathological case of >= 256 byte-long tags, could we simply exclude those tags from the expansion results?
Also, tab completion for tags probably isn't a very hot function call, so that could justify either:
- Making the buffer size dynamic
- Upping the buffer size to something more drastic like 512, 1024, 4096, presuming tags may consist of long Unicode code points.
Gregory Anders
In the (so-claimed) pathological case of >= 256 byte-long tags, could we simply exclude those tags from the expansion results?
I don't think this is a good default, especially because if >= 256 byte-long tags is not pathological (as you claim) then we'll just get another bug report down the road about how some tags aren't appearing in the results.
Also, tab completion for tags probably isn't a very hot function call, so that could justify either:
* Making the buffer size dynamic
@brammool We could probably do this with minimal memory allocations by simply allocating a large buffer up front and only reallocing when necessary. In this case the buffer never leaves the calling function so we don't need to worry about managing its lifetime.
Gregory Anders
@gpanders pushed 1 commit.
- 3435045 Allocate tag name buffer dynamically
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or unsubscribe.
Dominique Pellé
@brammool wrote:
If you have a tag name of more than 100 characters I'm not sure what you are doing.
I just looked at the the max tag size of the C++ I have at work and
it's 188 bytes long. I used this to find the longest tag name:
$ perl -ane 'printf "%d\n", length($F[0])' tags | sort -n | tail -1
188
It suffices to say that tags longer than 100 happen for legitimate
reasons. And increasing to 256 bytes still seems dangerously close
to the max tag size I see in my project.
The longest tag name in my project contains nested namespaces and
nested class names and look more or less like this (I replaced the
actual namespace and class names by something else since it's
code from work which I don't want to put here):
namespace_foo::nested_namespace_bar::class_name_foo::nested_class_name_bar::~nested_class_name_bar
So C++ nested namespaces, nested classes and method names
make it quite likely to have long tag names.
If speed matters (?) in that function that as an array in stack,
maybe we can have a buffer of 100 bytes in stack and
only dynamically allocate only when the tag name is longer
than the buffer in stack. So it would rarely need to allocate
dynamically.
Bram Moolenaar
> > Also, tab completion for tags probably isn't a very hot function
> > call, so that could justify either:
> >
> > * Making the buffer size dynamic
>
> @brammool We could probably do this with minimal memory allocations by
> necessary. In this case the buffer never leaves the calling function
> so we don't need to worry about managing its lifetime.
length is a #define used in several places. If you adjust the buffer
size, that size needs to be communicated to all places that need the
size.
If there is a missing size check somewhere, increasing the buffer size
is not the proper solution, it will only fail less often. Please
mention a reproduction case. Or write a test the fails without the fix.
--
bashian roulette:
$ ((RANDOM%6)) || rm -rf ~
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
Gregory Anders
I'm not sure where you are looking, but I thought that the max tag length is a #define used in several places. If you adjust the buffer size, that size needs to be communicated to all places that need the size.
I'm looking here, in 'expand_tags': https://github.com/vim/vim/blob/dea561111a5761bf99397a246b8baa43e73288de/src/tag.c#L3883
If there is a globally defined max tag length this buffer isn't using it.
If there is a missing size check somewhere, increasing the buffer size is not the proper solution, it will only fail less often.
Right, which is why @BGR360 and I proposed allocating the buffer dynamically. Or we can take @dpelle's suggestion and use a hybrid approach if minimizing dynamic allocations is important.
Please mention a reproduction case. Or write a test the fails without the fix.
We have a reproduction case mentioned in the first post of this thread: neovim/neovim#14026
Bram Moolenaar
> > I'm not sure where you are looking, but I thought that the max tag
> > length is a #define used in several places. If you adjust the buffer
> > size, that size needs to be communicated to all places that need the
> > size.
>
> I'm looking here, in 'expand_tags': https://github.com/vim/vim/blob/dea561111a5761bf99397a246b8baa43e73288de/src/tag.c#L3883
>
> If there is a globally defined max tag length this buffer isn't using it.
to allocate and reallocate to the right size.
> > If there is a missing size check somewhere, increasing the buffer size is not the proper solution, it will only fail less often.
>
> Right, which is why @BGR360 and I proposed allocating the buffer
> dynamically. Or we can take @dpelle's suggestion and use a hybrid
> approach if minimizing dynamic allocations is important.
overhead should not be relevant.
> > Please mention a reproduction case. Or write a test the fails without the fix.
>
Should not be difficult to turn this into a test case.
--
Yesterday, all my deadlines seemed so far away
now it looks as though it's freeze in four days
oh I believe in cvs..
[ CVS log "Beatles style" for FreeBSD ports/INDEX, Satoshi Asami ]
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
Bram Moolenaar
Please add the example as a test case. It should fail before applying the patch.
Gregory Anders
@gpanders pushed 1 commit.
- 51e0024 Add test case for tag expansion buffer overflow
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or unsubscribe.
Gregory Anders
Done, I added a test to Test_tag_line_too_long.