Skip to content
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

Fix access beyond end of string when SmcOpenConnection() truncates error message #7194

Conversation

dpelle
Copy link
Member

@dpelle dpelle commented Oct 24, 2020

Running make test_startup with valgrind, I see this error:

==10232== Memcheck, a memory error detector
==10232== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==10232== Using Valgrind-3.17.0.GIT and LibVEX; rerun with -h for copyright info
==10232== Command: ../vim -f -u NONE --not-a-term --clean -V2Xverbosefile -c set\ verbose?\ verbosefile? -cq
==10232== Parent PID: 10231
==10232== 
==10232== Conditional jump or move depends on uninitialised value(s)
==10232==    at 0x4C332A8: strlen (vg_replace_strmem.c:459)
==10232==    by 0x366557: vim_vsnprintf_typval (message.c:4487)
==10232==    by 0x367DD1: vim_vsnprintf (message.c:4238)
==10232==    by 0x367DD1: vim_snprintf (message.c:4226)
==10232==    by 0x252CF2: xsmp_init (os_unix.c:8075)
==10232==    by 0x13F1D3: main (main.c:323)
==10232==  Uninitialised value was created by a stack allocation
==10232==    at 0x252B72: xsmp_init (os_unix.c:8023)

Error happens in Test_V_file_arg(). Apparently, function...

extern SmcConn SmcOpenConnection (
    char *              /* networkIdsList */,
    SmPointer           /* context */,
    int                 /* xsmpMajorRev */,
    int                 /* xsmpMinorRev */,
    unsigned long       /* mask */,
    SmcCallbacks *      /* callbacks */,
    const char *        /* previousId */,
    char **             /* clientIdRet */,
    int                 /* errorLength */,
    char *              /* errorStringRet */
);

... called in xsmp_init() in os_unix.c:8057 does not NUL
terminate the errorStringRet string argument when the error
string is truncated to errorLength bytes. I did not find
accurate documentation about its behavior when truncation
happens, but the PR fixes the bug anyway.

@codecov
Copy link

codecov bot commented Oct 24, 2020

Codecov Report

Merging #7194 into master will increase coverage by 86.11%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #7194       +/-   ##
===========================================
+ Coverage    2.63%   88.74%   +86.11%     
===========================================
  Files         139      148        +9     
  Lines      150460   162351    +11891     
===========================================
+ Hits         3960   144077   +140117     
+ Misses     146500    18274   -128226     
Impacted Files Coverage Δ
src/os_unix.c 70.37% <0.00%> (+69.26%) ⬆️
src/if_python.c 83.27% <0.00%> (ø)
src/if_perl.xs 86.08% <0.00%> (ø)
src/if_py_both.h 84.65% <0.00%> (ø)
src/if_tcl.c 90.15% <0.00%> (ø)
src/xxd/xxd.c 75.33% <0.00%> (ø)
src/if_python3.c 87.29% <0.00%> (ø)
src/libvterm/t/harness.c 88.33% <0.00%> (ø)
src/if_ruby.c 91.56% <0.00%> (ø)
src/if_lua.c 93.28% <0.00%> (ø)
... and 135 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 f4c6e1e...eaf1cd6. Read the comment docs.

@brammool brammool closed this in e1be118 Oct 24, 2020
janlazo added a commit to janlazo/neovim that referenced this pull request Oct 24, 2020
Problem:    Crash in command line expansion when out of memory.
Solution:   Check for NULL pointer.  Also make ExpandGeneric() static.
            (Dominique Pelle, closes vim/vim#5437)
vim/vim@61d7c0d

N/A patches for version.c:

vim-patch:8.2.1892: valgrind warns for using uninitialized access in tests

Problem:    Valgrind warns for using uninitialized access in tests.
Solution:   Fix condition for breaking out of loop. (Dominique Pellé,
            closes vim/vim#7187)
vim/vim@9c24cd1

vim-patch:8.2.1896: valgrind warns for using uninitialized memory

Problem:    Valgrind warns for using uninitialized memory.
Solution:   NUL terminate the SmcOpenConnection() error message. (Dominique
            Pellé, closes vim/vim#7194)
vim/vim@e1be118
janlazo added a commit to janlazo/neovim that referenced this pull request Oct 25, 2020
Problem:    Crash in command line expansion when out of memory.
Solution:   Check for NULL pointer.  Also make ExpandGeneric() static.
            (Dominique Pelle, closes vim/vim#5437)
vim/vim@61d7c0d

N/A patches for version.c:

vim-patch:8.2.1892: valgrind warns for using uninitialized access in tests

Problem:    Valgrind warns for using uninitialized access in tests.
Solution:   Fix condition for breaking out of loop. (Dominique Pellé,
            closes vim/vim#7187)
vim/vim@9c24cd1

vim-patch:8.2.1896: valgrind warns for using uninitialized memory

Problem:    Valgrind warns for using uninitialized memory.
Solution:   NUL terminate the SmcOpenConnection() error message. (Dominique
            Pellé, closes vim/vim#7194)
vim/vim@e1be118
skippi pushed a commit to skippi/neovim that referenced this pull request Oct 25, 2020
Problem:    Crash in command line expansion when out of memory.
Solution:   Check for NULL pointer.  Also make ExpandGeneric() static.
            (Dominique Pelle, closes vim/vim#5437)
vim/vim@61d7c0d

N/A patches for version.c:

vim-patch:8.2.1892: valgrind warns for using uninitialized access in tests

Problem:    Valgrind warns for using uninitialized access in tests.
Solution:   Fix condition for breaking out of loop. (Dominique Pellé,
            closes vim/vim#7187)
vim/vim@9c24cd1

vim-patch:8.2.1896: valgrind warns for using uninitialized memory

Problem:    Valgrind warns for using uninitialized memory.
Solution:   NUL terminate the SmcOpenConnection() error message. (Dominique
            Pellé, closes vim/vim#7194)
vim/vim@e1be118
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant