fstd / libsrsirc

Lightweight cross-platform IRC lib written in standard C99 + IRC netcat (a.k.a. icat)
BSD 3-Clause "New" or "Revised" License
22 stars 5 forks source link

Return value after free()-ing it #5

Closed precla closed 2 years ago

precla commented 2 years ago

https://github.com/fstd/libsrsirc/blob/e76c45dd0598cf28695eed12cb0d2e05f29cace9/libsrsirc/ucbase.c#L198

free(m) then one line below is return m; https://github.com/fstd/libsrsirc/blob/e76c45dd0598cf28695eed12cb0d2e05f29cace9/libsrsirc/ucbase.c#L199

Return value is never used. Was NULL or true/false maybe intended to be here or is this on purpose?

(Found with cppcheck)

fstd commented 2 years ago

Hey, thanks for your report.

It looks a bit contrived but the logic checks out, I think. The idea is to have the function return whether or not something was dropped, and m (after the implicit conversion to bool) does that, since m will be non-NULL in that case (and we're making use of the fact that free(NULL) is explicitly permitted here.

I agree it looks like a potential use-after-free though, and the return should probably read return !!m instead, for clarity.

You're right about the value eventually not being used though -- turns out we don't really need it in the places it's used (essentially the PART and the KICK handlers). Can't hurt to have the mechanism, though.

Thanks again for the input!