linux-kernel - Re: [PATCH v2 02/19] gendwarfksyms: Add symbol list handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABCJKuc8fOmsbpqOJXwYstwc+7DEf-Bf26_n5_ai2Pax1tXqUg@mail.gmail.com>
Date: Tue, 27 Aug 2024 11:47:51 -0700
From: Sami Tolvanen <samitolvanen@...gle.com>
To: Petr Pavlu <petr.pavlu@...e.com>
Cc: Masahiro Yamada <masahiroy@...nel.org>, Luis Chamberlain <mcgrof@...nel.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Miguel Ojeda <ojeda@...nel.org>, 
	Matthew Maurer <mmaurer@...gle.com>, Alex Gaynor <alex.gaynor@...il.com>, 
	Wedson Almeida Filho <wedsonaf@...il.com>, Gary Guo <gary@...yguo.net>, Neal Gompa <neal@...pa.dev>, 
	Hector Martin <marcan@...can.st>, Janne Grunau <j@...nau.net>, Asahi Linux <asahi@...ts.linux.dev>, 
	linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-modules@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 02/19] gendwarfksyms: Add symbol list handling

Hi Petr,

On Tue, Aug 27, 2024 at 2:16 AM Petr Pavlu <petr.pavlu@...e.com> wrote:
>
> On 8/15/24 19:39, Sami Tolvanen wrote:
> > +static bool is_export_symbol(struct state *state, Dwarf_Die *die)
> > +{
> > +     Dwarf_Die *source = die;
> > +     Dwarf_Die origin;
> > +
> > +     state->sym = NULL;
>
> Nit: This assignment isn't strictly necessary, the value is overwritten
> a few lines below and isn't used in between.

True, I think this was left over from refactoring.

> > +int symbol_read_exports(FILE *file)
> > +{
> > +     struct symbol *sym;
> > +     char *line = NULL;
> > +     char *name = NULL;
> > +     size_t size = 0;
> > +     int nsym = 0;
> > +
> > +     while (getline(&line, &size, file) > 0) {
> > +             if (sscanf(line, "%ms\n", &name) != 1) {
> > +                     error("malformed input line: %s", line);
> > +                     return -1;
> > +             }
> > +
> > +             free(line);
> > +             line = NULL;
> > +
> > +             if (is_exported(name))
> > +                     continue; /* Ignore duplicates */
> > +
> > +             sym = malloc(sizeof(struct symbol));
> > +             if (!sym) {
> > +                     error("malloc failed");
> > +                     return -1;
> > +             }
> > +
> > +             sym->name = name;
> > +             name = NULL;
> > +
> > +             hash_add(symbol_names, &sym->name_hash, name_hash(sym->name));
> > +             ++nsym;
> > +
> > +             debug("%s", sym->name);
> > +     }
> > +
> > +     if (line)
> > +             free(line);
>
> The loop leaks line on a potential sscanf() error and name if the symbol
> is a duplicate or malloc(sizeof(struct symbol)) fails. Additionally, it
> should be possible to avoid allocating line by getline() on each
> iteration.
>
> I would change it to something like this (not tested):

Good points, I'll change this to your suggested version (after testing). Thanks!

Sami