| From | Geert Uytterhoeven <> | Date | Tue, 7 Mar 2023 08:53:39 +0100 | Subject | Re: [PATCH 00/99] fbdev: Fix memory leak in option parsing |
| |
Hi Thomas,
On Mon, Mar 6, 2023 at 5:00 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > Introduce struct option_iter and helpers to parse command-line > options with comma-separated key-value pairs. Then convert fbdev > drivers to the new interface. Fixes a memory leak in the parsing of > the video= option. > > Before commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to > caller; clarify ownership"), a call to fb_get_options() either > returned an internal string or a duplicated string; hence ownership of > the string's memory buffer was not well defined, but depended on how > users specified the video= option on the kernel command line. For > global settings, the caller owned the returned memory and for per-driver > settings, fb_get_options() owned the memory. As calling drivers were > unable to detect the case, the memory was leaked. > > Commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller; > clarify ownership") changed sematics to caller-owned strings. Drivers > still leaked the memory, but at least ownership was clear. > > This patchset fixes the memory leak and changes string ownership back > to fb_get_options(). Patch 1 introduces struct option_iter and a few > helpers. The interface takes an option string, such as video=, in the > common form value1,key2:value2,value3 etc and returns the individial > comma-separated pairs. Various modules use this pattern, so the code > is located under lib/. > > Patches 2 to 98 go through fbdev drivers and convert them to the new > interface. This often requires a number of cleanups. A driver would > typically refer to the option string's video mode. Such strings are now > copied to driver-allocated memory so that drivers don't refer directly > to the option string's memory. The option iterator then replaces manual > parsing loops based on strsep(",").
Thanks for your series!
Unfortunately I cannot say I'm thrilled about this: you are replacing a single small dynamic memory leak by 36 larger static memory leaks. Am I missing something?
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
|