Skip to content

apply: fix leaking buffer of `struct image`

Patrick Steinhardt git 2 versions – In review

Get series with b4
b4 am [email protected]

Versions newest first · linked by author and title

  1. v2 Latest6 patches 2 repliesIn review
  2. v16 patches 4 repliesSuperseded

Changes in v2 from the cover letter

- 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.

Patches in v2 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/6apply: reorder functions to move image-related things togetherapply.c+53 -530 replies
2/6apply: rename functions operating on `struct image`apply.c+30 -310 replies
3/6apply: introduce macro and function to init imagesapply.c+14 -80 replies
4/6apply: refactor code to drop `line_allocated`apply.c+14 -190 replies
5/6apply: rename members that track line count and allocation lengthapply.c+45 -460 replies
6/6apply: refactor `struct image` to use a `struct strbuf`apply.c t/t3436-rebase-more-options.sh t/t4107-apply-ignore-whitespace.sh +3 more+79 -1230 replies

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v2 yet.

Read the v2 thread

Cover letter [PATCH v2 0/6] apply: fix leaking buffer of `struct image`

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).

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

Signature · 1 line
2.46.0.551.gc5ee8f2d1c.dirty