[RDY] Coverity fixes (mostly resource leaks) #790

aktau · 2014-05-31T18:24:34Z

To clear the haystack, start by picking up a few pieces of hay.
-- ancient chinese proverb

Not really, but I was looking at the coverity log and decided to throw in a few tiny fixes.

aktau · 2014-05-31T18:44:09Z

src/nvim/ops.c

    }
+    }
+
    set_expr_line(p);


How do you figure?

The formatting was a bit off though (because I used add without whitespace changes, because the codebase still isn't trailing-whitespace clean). Fixed in the latest squash commit.

I was mistaken, got cross-eyed.

justinmk · 2014-05-31T19:04:29Z

LGTM

aktau · 2014-05-31T19:12:05Z

Your comments have been taken into account, will tag RDY tomorrow, hopefully before the next coverity run ;).

oni-link · 2014-05-31T19:54:11Z

src/nvim/ops.c

    }
+
    set_expr_line(p);


Here one could save quite some copying, if expr_line would be used directly instead of get_expr_line and set_expr_line.

That had not escaped me. The question is if we want to sanction direct manipulation or not? I mean, it's all globals anyway, and the copying seems kind of senseless...

What does the rest think?

Within this function it looks fine to mutate expr_line directly. dis_msg already does so.

Alright, if it's a go, I'll trash some allocations.

@justinmk and @oni-link what do you think of the new incarnation: aktau@8ce525e ?

It basically does one realloc and memcpy in all cases.

There's one thing that bothered me a little. Vim doesn't seem to apply maxlen to the total string in the case of must_append. What I mean is that in the must_append case, the expr_line could grow indefinitely even when maxlen is specified. Since I wasn't sure if this was intentional, I kept this behaviour.

aktau · 2014-06-01T11:10:32Z

To facilitate a last comb-over, I've squashed all the fixes resulting from @justinmk and @oni-link's comments. Tagging RDY after travis says it's a go.

UPDATE: tagged RDY.

justinmk · 2014-06-01T19:05:15Z

src/nvim/ops.c

    }
-    set_expr_line(p);
+
+    // modify the global expr_line, extend/shrink it if necessary (realloc).


expr_line is not global, thankfully. get_expr_line() returns a copy to external callers.

It's not an external global, but it is a file local (static) global. I always like to mention it when this happens because I hate reading code that operates on globals, confusing as to where it came from.

justinmk · 2014-06-02T06:24:00Z

Checked coveralls to see if write_reg_contents_ex is covered.

seems like coveralls has not analyzed master since May 20
got a server error when I tried to view ops.c
gcc/unittest build shows "?" for all files, seems like coveralls does not understand the results

aktau · 2014-06-02T12:04:53Z

Checked coveralls to see if write_reg_contents_ex is covered...

That sucks. Perhaps @ashleyh knows more?

I've posted an edited version of write_reg_contents_ex. See this commit: aktau@384a064

A few notes:

I've constified the input str, for this I needed to add a const modifier to some subfunctions. Note that the original documentation of write_reg_contents noted that the contents of str may be changed so that a copy might me necessary. But I didn't see any subfunction in which str is changed. So I took the liberty of constifying it.
Because I had to change parameter names and modifiers, I changed the comments and function declarations to modern-style.
I changed the maxlen parameter to be named len. I also changed its type to ssize_t, as it can be either -1 or an array offset, which is the exact use-case of ssize_t.
I reverted the calculation of (max)len to what it was before.

What do we think of this version?

- New command line option prints the binary API metadata object and exits

Uses a perl script to move it (scripts/movedocs.pl)

- The 'stripdecls.py' script replaces declarations in all headers by includes to generated headers. `ag '#\s*if(?!ndef NEOVIM_).*((?!#\s*endif).*\n)*#ifdef INCLUDE_GENERATED'` was used for this. - Add and integrate gendeclarations.lua into the build system to generate the required includes. - Add -Wno-unused-function - Made a bunch of old-style definitions ANSI This adds a requirement: all type and structure definitions must be present before INCLUDE_GENERATED_DECLARATIONS-protected include. Warning: mch_expandpath (path.h.generated.h) was moved manually. So far it is the only exception.

…IBUTES Required for FUNC_ATTR_UNUSED to work in lib/k*

So that they do the last nvim/func_attr.h include

Otherwise FUNC_ATTR_* macros may appear empty

Also constified the arguments. The double casts for the `xstrdup` are ugly but `vim_strsave` doesn't take `const` arguments for now so I couldn't keep that.

Coverity detected a memory leak caused by not free'ing the value returned by get_expr_line_src (basically vim_strsave(expr_line)). Replaced the copying with direct manipulation of expr_line, since that also happens in other parts of the codebase. NOTE: I'm aware that this has different behaviour than vim_strnsave, namely vim_strnsave always allocates `len` bytes, even if the string is shorter. I don't see how that behaviour is helpful here though.

Also cleaned up the function a little bit.

It was a false positive, but it can't hurt to "fix" it. Original warning: CID 13685 (#1 of 1): Buffer not null terminated (BUFFER_SIZE) 6. buffer_size: Calling strncpy with a source string whose length (4 chars) is greater than or equal to the size argument (4) will fail to null-terminate b0p->b0_version.

aktau · 2014-06-02T18:28:21Z

@oni-link I fixed and immediately squashed the latest things you suggested. I hope it's ok now. Fingers crossed!

@justinmk I'm not sure why github is freaking out about this branch, but if you pull from it in your cmdline you'll (probably?) see that only my changes are "new". I just rebased to master.

aktau · 2014-06-02T18:33:41Z

Alright guys I've made PR #804 as an alternative because this smells fishy.

aktau changed the title ~~Coverity fixes (mostly resource leaks)~~ [RFC] Coverity fixes (mostly resource leaks) May 31, 2014

aktau reviewed May 31, 2014
View reviewed changes

oni-link reviewed May 31, 2014
View reviewed changes

aktau changed the title ~~[RFC] Coverity fixes (mostly resource leaks)~~ [RDY] Coverity fixes (mostly resource leaks) Jun 1, 2014

justinmk reviewed Jun 1, 2014
View reviewed changes

equalsraf and others added 16 commits June 2, 2014 20:20

Add --api-metadata command line option

214f55b

- New command line option prints the binary API metadata object and exits

Rename --api-metadata to --api-msgpack-metadata

61ad4da

Move documentation from function declarations to definitions

7caae43

Uses a perl script to move it (scripts/movedocs.pl)

Remove remaining declarations with new script: finddeclarations.pl

c3f7d6d

Surround nvim/func_attr.h includes with define/undef DEFINE_FUNC_ATTR…

a1a9750

…IBUTES Required for FUNC_ATTR_UNUSED to work in lib/k*

Make nvim/lib/k*.h headers be the last one in the list

a485ac4

So that they do the last nvim/func_attr.h include

Add include guard to ex_cmds_defs

ed48b28

Fix some styles

10ad900

Add missing include to mbyte.c

17e5d2f

Remove a number of unneeded func_attr includes

b4f9d91

Remove unneeded klist include in channel.c

34e1905

Move memory.h include above func_attr.h include

78390be

Add REAL_FATTR_ macros to func_attr.h, use them in lib/k*

5dca766

Otherwise FUNC_ATTR_* macros may appear empty

Use generated headers in os.h

e970fbb

Use generated static declarations in cursor.c

f6e870b

aktau added 4 commits June 2, 2014 20:21

coverity/62617: fix leak in set_string_default

302a161

Also constified the arguments. The double casts for the `xstrdup` are ugly but `vim_strsave` doesn't take `const` arguments for now so I couldn't keep that.

coverity/13770: add_keyword(), mark as false pos.

747e15b

Also cleaned up the function a little bit.

aktau mentioned this pull request Jun 2, 2014

[RDY] Coverity fix resource leaks 1b #804

Closed

aktau closed this Jun 2, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RDY] Coverity fixes (mostly resource leaks) #790

[RDY] Coverity fixes (mostly resource leaks) #790

aktau commented May 31, 2014

aktau May 31, 2014

aktau May 31, 2014

justinmk May 31, 2014

justinmk commented May 31, 2014

aktau commented May 31, 2014

oni-link May 31, 2014

aktau May 31, 2014

justinmk May 31, 2014

aktau May 31, 2014

aktau Jun 1, 2014

aktau commented Jun 1, 2014

justinmk Jun 1, 2014

aktau Jun 1, 2014

justinmk commented Jun 2, 2014

aktau commented Jun 2, 2014

aktau commented Jun 2, 2014

aktau commented Jun 2, 2014

[RDY] Coverity fixes (mostly resource leaks) #790

[RDY] Coverity fixes (mostly resource leaks) #790

Conversation

aktau commented May 31, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk commented May 31, 2014

aktau commented May 31, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aktau commented Jun 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk commented Jun 2, 2014

aktau commented Jun 2, 2014

aktau commented Jun 2, 2014

aktau commented Jun 2, 2014