| Message ID | cover.1726567217.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | apply: fix leaking buffer of `struct image` | expand |
On Tue, Sep 17, 2024 at 12:07:48PM +0200, Patrick Steinhardt wrote: > Hi, > > this is the second version of my patch series that refactors lifecycle > management of `struct image` in "apply.c" to plug a bunch of memory > leaks. > > Changes compared to v1: > > - Fix two typos. > > - Correct the statement that we don't loop around > `image_remove_first_line()`. No idea how I missed that loop. > > - Split up an overly complex line into multiple lines. > > Thanks! Ugh, sorry. I meant to Cc Junio, not Taylor. Patrick
Patrick Steinhardt <ps@pks.im> writes: > this is the second version of my patch series that refactors lifecycle > management of `struct image` in "apply.c" to plug a bunch of memory > leaks. > > Changes compared to v1: > > - Fix two typos. > > - Correct the statement that we don't loop around > `image_remove_first_line()`. No idea how I missed that loop. > > - Split up an overly complex line into multiple lines. Looking good. Will queue. Thanks.
Hi, this is the second version of my patch series that refactors lifecycle management of `struct image` in "apply.c" to plug a bunch of memory leaks. Changes compared to v1: - Fix two typos. - Correct the statement that we don't loop around `image_remove_first_line()`. No idea how I missed that loop. - Split up an overly complex line into multiple lines. Thanks! Patrick Patrick Steinhardt (6): apply: reorder functions to move image-related things together apply: rename functions operating on `struct image` apply: introduce macro and function to init images apply: refactor code to drop `line_allocated` apply: rename members that track line count and allocation length apply: refactor `struct image` to use a `struct strbuf` apply.c | 449 +++++++++++++---------------- t/t3436-rebase-more-options.sh | 1 + t/t4107-apply-ignore-whitespace.sh | 4 +- t/t4124-apply-ws-rule.sh | 1 + t/t4125-apply-ws-fuzz.sh | 1 + t/t4138-apply-ws-expansion.sh | 1 + 6 files changed, 206 insertions(+), 251 deletions(-) Range-diff against v1: 1: 7b6903ecdd = 1: a713a7aef0 apply: reorder functions to move image-related things together 2: 3f188412f6 = 2: be8f98881f apply: rename functions operating on `struct image` 3: 1b49e39bcd = 3: 506c787d68 apply: introduce macro and function to init images 4: 0427cb7250 ! 4: 6ac37186f2 apply: refactor code to drop `line_allocated` @@ Commit message apply: refactor code to drop `line_allocated` The `struct image` has two members `line` and `line_allocated`. The - former member is the one that should be used throughougt the code, + former member is the one that should be used throughout the code, whereas the latter one is used to track whether the lines have been allocated or not. In practice, the array of lines is always allocated. The reason why we have `line_allocated` is that `remove_first_line()` will advance the - array pointer to drop the first entry, and thus it point into the array + array pointer to drop the first entry, and thus it points into the array instead of to the array header. Refactor the function to use memmove(3P) instead, which allows us to get - rid of this double bookkeeping. We call this function at most once per - image anyway, so this shouldn't cause any performance regressions. + rid of this double bookkeeping. This is less efficient, but I doubt that + this matters much in practice. If this judgement call is found to be + wrong at a later point in time we can likely refactor the surrounding + loop such that we first calculate the number of leading context lines to + remove and then remove them in a single call to memmove(3P). Signed-off-by: Patrick Steinhardt <ps@pks.im> 5: e35816ed56 = 5: 5497708428 apply: rename members that track line count and allocation length 6: 6cf45daf84 ! 6: 42880dcf04 apply: refactor `struct image` to use a `struct strbuf` @@ apply.c: static void image_remove_first_line(struct image *img) static void image_remove_last_line(struct image *img) { - img->len -= img->line[--img->line_nr].len; -+ strbuf_setlen(&img->buf, img->buf.len - img->line[--img->line_nr].len); ++ size_t last_line_len = img->line[img->line_nr - 1].len; ++ strbuf_setlen(&img->buf, img->buf.len - last_line_len); ++ img->line_nr--; } /* fmt must contain _one_ %s and no other substitution */ base-commit: ed155187b429a2a6b6475efe1767053df37ccfe1