Hi Leo, On Tue, 28 Jul 2020 19:56:23 -0400 Leo Famulari wrote: > On Tue, Jul 28, 2020 at 12:52:41PM +0200, Danny Milosavljevic wrote: > > some comments on the lastest patch: > > Thank you for reviewing the patch! > > > * The entire alsa-lib seems to use the idiom "malloc and then strcpy", or > > "malloc and then sprintf", or, worse, "malloc, strcpy and multiple strcat". > > These are a buffer overflow waiting to happen (when changing part of those > > while doing ongoing maintenance; also the places where they use "+" is not > > checked for overflow). That said, if they do it, we can do it that way, too. > > This confirms what I felt — it's hard to feel confident about the bounds > checking in this code. It seems to be based on the names of the plugin > libraries not exceeding some magic length. It's hard to balance "doing > the right thing" and using upstream's idioms. After thinking about it more, I think it's much worse if the thing that is stuck into the malloced block is the value of an environment variable. When it's a compile-time variable you basically trust the code and the distribution package not to have too-long paths in there that could overflow the "+" in malloc(... + ). A distribution or upstream could do much worse things than that, so that is not a credible threat to worry about. For a runtime variable like the environment variable (that anyone can set to anything), I am very much in favor of not using malloc(... + ) and instead using asprintf, in order to prevent an exploitable buffer overflow just by setting up an environment variable. > When writing the patch, my investigation into the code made decide that > it would not overflow, but now I don't remember why I thought that. Thanks for that remark. It made me think and I came to the recommendation above.