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.

Sign up for GitHub

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 pc_patch_compress leak (take 2) #206

Merged
merged 5 commits into from
Apr 30, 2018
Merged

Fix pc_patch_compress leak (take 2) #206

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6e3d61a
Move the freeing of stats into its own function
achidlow Jun 21, 2016
3aefbe2
Fix code style
Apr 30, 2018
a7c046e
Rename pc_patch_common_free to pc_patch_free_stats
Apr 30, 2018
fa5d754
Remove unnecessary test
Apr 30, 2018
6e7023f
Add asserts to pc_patch_*_free functions
Apr 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions lib/cunit/cu_pc_patch.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,57 @@ test_patch_dimensional_compression()
if ( pds ) pc_dimstats_free(pds);
}

static void
test_patch_compression_stats_leak()
{
PCPOINT *pt;
int i;
int npts = 400;
PCPOINTLIST *pl1, *pl2;
PCPATCH *pch1, *pch2;
PCDIMSTATS *pds = NULL;

pl1 = pc_pointlist_make(npts);

for ( i = 0; i < npts; i++ )
{
pt = pc_point_make(simpleschema);
pc_point_set_double_by_name(pt, "x", i*2.0);
pc_point_set_double_by_name(pt, "y", i*1.9);
pc_point_set_double_by_name(pt, "Z", i*0.34);
pc_point_set_double_by_name(pt, "intensity", 10);
pc_pointlist_add_point(pl1, pt);
}

pch1 = pc_patch_from_pointlist(pl1);

pch2 = pc_patch_compress(pch1, pds);

pl2 = pc_pointlist_from_patch(pch2);

for ( i = 0; i < npts; i++ )
{
pt = pc_pointlist_get_point(pl2, i);
double v1, v2, v3, v4;
pc_point_get_double_by_name(pt, "x", &v1);
pc_point_get_double_by_name(pt, "y", &v2);
pc_point_get_double_by_name(pt, "Z", &v3);
pc_point_get_double_by_name(pt, "intensity", &v4);

CU_ASSERT_DOUBLE_EQUAL(v1, i*2.0, 0.001);
CU_ASSERT_DOUBLE_EQUAL(v2, i*1.9, 0.001);
CU_ASSERT_DOUBLE_EQUAL(v3, i*0.34, 0.001);
CU_ASSERT_DOUBLE_EQUAL(v4, 10, 0.001);
}

pc_patch_free((PCPATCH*)pch1);
pc_patch_free((PCPATCH*)pch2);
pc_pointlist_free(pl1);
pc_pointlist_free(pl2);
if ( pds ) pc_dimstats_free(pds);
}


static void
test_patch_dimensional_extent()
{
Expand Down Expand Up @@ -1394,6 +1445,7 @@ CU_TestInfo patch_tests[] = {
PC_TEST(test_schema_xy),
PC_TEST(test_patch_dimensional),
PC_TEST(test_patch_dimensional_compression),
PC_TEST(test_patch_compression_stats_leak),
PC_TEST(test_patch_dimensional_extent),
PC_TEST(test_patch_union),
PC_TEST(test_patch_wkb),
Expand Down
1 change: 1 addition & 0 deletions lib/pc_api_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ char* pc_dimstats_to_string(const PCDIMSTATS *pds);

/** Returns newly allocated patch that only contains the points fitting the filter condition */
PCPATCH* pc_patch_filter(const PCPATCH *pa, uint32_t dimnum, PC_FILTERTYPE filter, double val1, double val2);
void pc_patch_free_stats(PCPATCH *pa);

/* DIMENSIONAL PATCHES */
char* pc_patch_dimensional_to_string(const PCPATCH_DIMENSIONAL *pa);
Expand Down
6 changes: 5 additions & 1 deletion lib/pc_patch.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,18 @@ pc_patch_compute_stats(PCPATCH *pa)
}

void
pc_patch_free(PCPATCH *patch)
pc_patch_free_stats(PCPATCH *patch)
{
if ( patch->stats )
{
pc_stats_free( patch->stats );
patch->stats = NULL;
}
}

void
pc_patch_free(PCPATCH *patch)
{
switch( patch->type )
{
case PC_NONE:
Expand Down
4 changes: 4 additions & 0 deletions lib/pc_patch_dimensional.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ pc_patch_dimensional_decompress(const PCPATCH_DIMENSIONAL *pdl)
pdl_decompressed = pcalloc(sizeof(PCPATCH_DIMENSIONAL));
memcpy(pdl_decompressed, pdl, sizeof(PCPATCH_DIMENSIONAL));
pdl_decompressed->bytes = pcalloc(ndims*sizeof(PCBYTES));
pdl_decompressed->stats = pc_stats_clone(pdl->stats);

/* Compress each dimension as dictated by stats */
for ( i = 0; i < ndims; i++ )
Expand All @@ -165,6 +166,8 @@ pc_patch_dimensional_free(PCPATCH_DIMENSIONAL *pdl)
assert(pdl);
assert(pdl->schema);

pc_patch_free_stats((PCPATCH*) pdl);

if ( pdl->bytes )
{
for ( i = 0; i < pdl->schema->ndims; i++ )
Expand Down Expand Up @@ -282,6 +285,7 @@ pc_patch_dimensional_from_wkb(const PCSCHEMA *schema, const uint8_t *wkb, size_t
patch->schema = schema;
patch->npoints = npoints;
patch->bytes = pcalloc(ndims*sizeof(PCBYTES));
patch->stats = NULL;

buf = wkb+hdrsz;
for ( i = 0; i < ndims; i++ )
Expand Down
4 changes: 4 additions & 0 deletions lib/pc_patch_ght.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,8 @@ pc_patch_ght_free(PCPATCH_GHT *paght)
assert(paght);
assert(paght->schema);

pc_patch_free_stats((PCPATCH*) paght);

/* A readonly tree won't own it's ght buffer, */
/* so only free a readwrite tree */
if ( ! paght->readonly )
Expand Down Expand Up @@ -363,6 +365,7 @@ pc_patch_ght_from_wkb(const PCSCHEMA *schema, const uint8_t *wkb, size_t wkbsize
patch->readonly = PC_FALSE;
patch->schema = schema;
patch->npoints = npoints;
patch->stats = NULL;

/* Start on the GHT */
buf = wkb+hdrsz;
Expand Down Expand Up @@ -533,6 +536,7 @@ pc_patch_ght_filter(const PCPATCH_GHT *patch, uint32_t dimnum, PC_FILTERTYPE fil
paght->readonly = PC_FALSE;
paght->schema = patch->schema;
paght->npoints = npoints;
paght->stats = NULL;

/* No points, not much to do... */
if ( ! npoints )
Expand Down
2 changes: 2 additions & 0 deletions lib/pc_patch_lazperf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ void
pc_patch_lazperf_free(PCPATCH_LAZPERF *pal)
{
assert(pal);
pc_patch_free_stats((PCPATCH*) pal);
pcfree(pal->lazperf);
pcfree(pal);
}
Expand Down Expand Up @@ -206,6 +207,7 @@ pc_patch_lazperf_from_wkb(const PCSCHEMA *schema, const uint8_t *wkb, size_t wkb
patch->readonly = PC_FALSE;
patch->schema = schema;
patch->npoints = npoints;
patch->stats = NULL;

/* Start on the LAZPERF */
buf = wkb+hdrsz;
Expand Down
4 changes: 4 additions & 0 deletions lib/pc_patch_uncompressed.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ pc_patch_uncompressed_from_wkb(const PCSCHEMA *s, const uint8_t *wkb, size_t wkb
patch->maxpoints = npoints;
patch->datasize = (wkbsize - hdrsz);
patch->data = data;
patch->stats = NULL;

return (PCPATCH*)patch;
}
Expand Down Expand Up @@ -215,6 +216,8 @@ pc_patch_uncompressed_compute_extent(PCPATCH_UNCOMPRESSED *patch)
void
pc_patch_uncompressed_free(PCPATCH_UNCOMPRESSED *patch)
{
pc_patch_free_stats((PCPATCH*) patch);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you removed the check in pc_patch_free_stats, maybe you can add assert(patch); here, just to be consistent across patch types

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll add that.

Nice picture by the way ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New commit added.

if ( patch->data && ! patch->readonly )
{
pcfree(patch->data);
Expand Down Expand Up @@ -279,6 +282,7 @@ pc_patch_uncompressed_from_pointlist(const PCPOINTLIST *pl)
pch = pcalloc(sizeof(PCPATCH_UNCOMPRESSED));
pch->datasize = s->size * numpts;
pch->data = pcalloc(pch->datasize);
pch->stats = NULL;
ptr = pch->data;

/* Initialize bounds */
Expand Down