Skip to content

Buffer growing cleanups#4255

Merged
ethomson merged 5 commits into
libgit2:masterfrom
pks-t:pks/buffer-grow-errors
Jun 8, 2017
Merged

Buffer growing cleanups#4255
ethomson merged 5 commits into
libgit2:masterfrom
pks-t:pks/buffer-grow-errors

Conversation

@pks-t
Copy link
Copy Markdown
Member

@pks-t pks-t commented Jun 7, 2017

Small-ish cleanups for growing buffers. While this is mostly done to silence Coverity, I think it even improves code by a tiny fraction.

@pks-t pks-t force-pushed the pks/buffer-grow-errors branch from 5b8227a to db42831 Compare June 7, 2017 12:03
@ethomson
Copy link
Copy Markdown
Member

ethomson commented Jun 8, 2017

Seems reasonable to me. One thing I noticed here is that the ENSURE_SIZE macro is pretty wacky...!

#define ENSURE_SIZE(b, d) \
    if ((d) > buf->asize && git_buf_grow(b, (d)) < 0)\
        return -1;

Notice the buf there instead of using the b argument... turns out that we're real consistent with naming and didn't notice this!

@ethomson
Copy link
Copy Markdown
Member

ethomson commented Jun 8, 2017

I'm going to merge this manually, with a fix for ENSURE_SIZE.

@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Jun 8, 2017

Oh, good eye! That's just been waiting to explode. Seeing you didn't yet create the commit, I'll fix this up.

@ethomson
Copy link
Copy Markdown
Member

ethomson commented Jun 8, 2017

Oddly, fixing that makes a unit test fail on Windows...! I'm digging in now.

@ethomson
Copy link
Copy Markdown
Member

ethomson commented Jun 8, 2017

But feel free to push a change to this branch - I'm curious if AppVeyor sees the same thing or if it's something bizarre on my machine.

@pks-t pks-t force-pushed the pks/buffer-grow-errors branch from db42831 to 643189a Compare June 8, 2017 09:56
@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Jun 8, 2017

Oh, strange. Didn't get any errors on Linux.

@pks-t pks-t force-pushed the pks/buffer-grow-errors branch from 643189a to a693b87 Compare June 8, 2017 09:58
pks-t added 5 commits June 8, 2017 11:58
The function `git_buf_try_grow` consistently calls `giterr_set_oom`
whenever growing the buffer fails due to insufficient memory being
available. So in fact, we do not have to do this ourselves when a call
to any buffer-growing function has failed due to an OOM situation. But
we still do so in two functions, which this patch cleans up.
While the `ENSURE_SIZE` macro gets a reference to both the buffer that
is to be resized and a new size, we were not consistently referencing
the passed buffer, but instead a variable `buf`, which is not passed in.
Funnily enough, we never noticed because our buffers seem to always be
named `buf` whenever the macro was being used.

Fix the macro by always using the passed-in buffer. While at it, add
braces around all mentions of passed-in variables as should be done with
macros to avoid subtle errors.

Found-by: Edward Thompson
The `ENSURE_SIZE` macro can be used to grow a buffer if its currently
allocated size does not suffice a required target size. While most of
the code already uses this macro, the `git_buf_join` and `git_buf_join3`
functions do not yet use it. Due to the macro first checking whether we
have to grow the buffer at all, this has the benefit of saving a
function call when it is not needed. While this is nice to have, it will
probably not matter at all performance-wise -- instead, this only serves
for consistency across the code.
Both the `git_buf_init` and `git_buf_attach` functions may call
`git_buf_grow` in case they were given an allocation length as
parameter. As such, it is possible for these functions to fail when we
run out of memory. While it won't probably be used anytime soon, it does
indeed make sense to also record this fact by returning an error code
from both functions. As they belong to the internal API only, this
change does not break our interface.
The `git_buf_init` function has an optional length parameter, which will
cause the buffer to be initialized and allocated in one step. This can
be used instead of static initialization with `GIT_BUF_INIT` followed by
a `git_buf_grow`. This patch does so for two functions where it is
applicable.
@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Jun 8, 2017

Taking a look at all calls to ENSURE_SIZE, it is always called with buf, so the generated code should always be the same.

@ethomson
Copy link
Copy Markdown
Member

ethomson commented Jun 8, 2017

Yeah... presumably something else entirely, but the repo::discover tests are failing to clean up their tempdir on my machine now... Very odd.

@ethomson
Copy link
Copy Markdown
Member

ethomson commented Jun 8, 2017

Failing for me in origin/master as well, so... 🤷‍♂️ No idea what's going on, but I'm going to assume it's isolated to my machine for now and we can go ahead and merge this as-is.

@ethomson ethomson merged commit 458cea5 into libgit2:master Jun 8, 2017
@pks-t pks-t deleted the pks/buffer-grow-errors branch June 12, 2017 06:11
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.

2 participants