Skip to content

parser: plug a memory leak - #309

Merged
slyon merged 4 commits into
canonical:mainfrom
daniloegea:routes_mleak
Jan 10, 2023
Merged

parser: plug a memory leak#309
slyon merged 4 commits into
canonical:mainfrom
daniloegea:routes_mleak

Conversation

@daniloegea

@daniloegea daniloegea commented Jan 5, 2023

Copy link
Copy Markdown
Contributor

The route->type field wasn't being free'ed in case of error. Caught by a unit test compiled with ASAN.

Description

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

The route->type field wasn't being free'ed in case of error. Caught by a
unit test compiled with ASAN.

@slyon slyon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The Route is defined as:

typedef struct {
    guint family;
    char* type;
    char* scope;
    guint table;

    char* from;
    char* to;
    char* via;

    gboolean onlink;

    /* valid metrics are valid positive integers.
     * invalid metrics are represented by METRIC_UNSPEC */
    guint metric;

    guint mtubytes;
    guint congestion_window;
    guint advertised_receive_window;
} NetplanIPRoute;

We're freeing route->type here, but what about scope/from/to/via on error? Should we be using free_route() util (from types.c) instead?

@daniloegea

Copy link
Copy Markdown
Contributor Author

Yes, definitely. I didn't pay attention to the fact that the other fields could be allocated later.

A different error might happen later and we will end up not freeing
other fields.

@slyon slyon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, having a closer look myself. I think we can further simplify this, by making use of the route_clear() function (instead of manual free & nullify), as defined using the CLEAR_FROM_FREE(free_route, route_clear, NetplanIPRoute); macro. Also, the route_clear function is already declared in types.h, so no need to add free_route into types-internal.h (and no need to make it non-static).

Comment thread src/parse.c Outdated
if (npp->current.route) {
g_free(npp->current.route);
free_route(npp->current.route);
npp->current.route = NULL;

@slyon slyon Jan 10, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use route_clear() instead of manual free & nullify.

@slyon slyon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm, thanks!

@slyon
slyon merged commit e5ff9f6 into canonical:main Jan 10, 2023
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