[vim/vim] check length of line (PR #18953)
sxt1001
Add a check for the length of the line, it should be a number not less than 0.
Fixes:#17935
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/18953
Commit Summary
- 6dd1fe9 check length of line
File Changes
(1 file)
- M src/memline.c (2)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
sxt1001
@xiaoge1001 pushed 1 commit.
- 97259a1 check length of line
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#18953)Hm, that indicates an overflow. Looking at your reproduction:
dd if=/dev/zero bs=1M count=10240 | tr '\0' 'A' > bigline.txt
that indicates 1 single line of 1,048,576 bytes * 10,240 = 10,737,418,240 bytes (without newlines).
colnr_T is int. So are you using 32bit arch? Don't you see an error message for such a long error with your patch?
I guess it makes sense. @zeertzjq what do you think?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#18953)I guess it makes sense.
Actually I am not so sure anymore. I think we should rather use something like this:
diff --git a/src/fileio.c b/src/fileio.c index 8156b80e1..a856ec050 100644 --- a/src/fileio.c +++ b/src/fileio.c @@ -2225,7 +2225,7 @@ rewind_retry: { *ptr = NUL; // end of line len = (colnr_T) (ptr - line_start + 1); - if (ml_append(lnum, line_start, len, newfile) == FAIL) + if (len < 0 || ml_append(lnum, line_start, len, newfile) == FAIL) { error = TRUE; break; @@ -2295,7 +2295,7 @@ rewind_retry: ff_error = EOL_DOS; } } - if (ml_append(lnum, line_start, len, newfile) == FAIL) + if (len < 0 || ml_append(lnum, line_start, len, newfile) == FAIL) { error = TRUE; break; @@ -2355,7 +2355,7 @@ failed: curbuf->b_p_eol = FALSE; *ptr = NUL; len = (colnr_T)(ptr - line_start + 1); - if (ml_append(lnum, line_start, len, newfile) == FAIL) + if (len < 0 || ml_append(lnum, line_start, len, newfile) == FAIL) error = TRUE; else {
or probably even better, split the lines manually if len would become negative (see readfile(), search for Protect against the argument of lalloc() going negative). Note this is also mentioned briefly at :h limits:
Maximum line length 2147483647 characters. Longer lines are split.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
sxt1001
xiaoge1001 left a comment (vim/vim#18953)So are you using 32bit arch? Don't you see an error message for such a long error with your patch?
I am using the AArch64 architecture. After applying the patch, there was an error reported, which is acceptable. I do not want a core dump to occur.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
sxt1001
xiaoge1001 left a comment (vim/vim#18953)I guess it makes sense.
Actually I am not so sure anymore. I think we should rather use something like this
The principles of the two patches are the same; they just differ in the timing of detection. Could you submit your patch? Or I can do it after a few hours and apply your idea to the current PR.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#18953)I am using the AArch64 architecture. After applying the patch, there was an error reported, which is acceptable. I do not want a core dump to occur.
So I googled and it seems int is 32bit on AArch64 and it overflows. Note, that from your screenshot it mentions "long lines split". I think we should make this work. However I have to think about this work.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
sxt1001
xiaoge1001 left a comment (vim/vim#18953)I am using the AArch64 architecture. After applying the patch, there was an error reported, which is acceptable. I do not want a core dump to occur.
So I googled and it seems int is 32bit on AArch64 and it overflows. Note, that from your screenshot it mentions "long lines split". I think we should make this work. However I have to think about this work.
Okay, if it can be resolved, that would be great. I'm not very familiar with Vim code, so I'm at a loss here. And even if you make this work, should we also check if len is less than 0?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
sxt1001
@xiaoge1001 pushed 1 commit.
- 323b773 add check len
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#18953)I finally got around to have a closer look. I think this PR #19332 should fix your issue correcly
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.