NuxiNL / cloudlibc

CloudABI's standard C library
BSD 2-Clause "Simplified" License
295 stars 17 forks source link

Fixed small memleak #5

Closed CurlyMoo closed 7 years ago

CurlyMoo commented 7 years ago

A free was missing before one of the returns.

EdSchouten commented 7 years ago

Hi there,

Thanks for the patch, but it's probably not correct, right? The idea behind that code is: we keep track of all strings that we return through struct tm::tm_zone in a string pool, so that we only need to allocate them once. If that specific if () succeeds, it means we've successfully placed the new timezone abbreviation in the string pool. We should never try to free it from then on, as callers of localtime_l() may access the string.

In theory we shouldn't need to use a string pool. We could make struct tm::tm_zone an array containing the string. Unfortunately, all other operating systems already have that field as a pointer to a string instead of an array, so our hands our tied.

How did you discover this? Did you use some memory leak checker to detect this? In theory this is a memory leak, but considering that the number of possible timezone abbreviations is fairly small, the string pool should also remain small.

CurlyMoo commented 7 years ago

How did you discover this? Did you use some memory leak checker to detect this?

Yes.

I also see that my patch isn't precise enough (if needed in the first place). The abbreviation should be cached first.

EdSchouten commented 7 years ago

Ah, got it. Then this specific allocation should be added to the checker's whitelist, as there is nothing that can be done to prevent it. There are some other places in the C library where we need to memoize stuff. getservbyname()/getservbyport() is another good example.

Good catch, though!