memory allocation cleanups #101
Conversation
This affects the non-Windows version of map_file, which is called during MMDB_open: File sizes are generally handled with off_t data type. The width of this data type is flexible, but generally 64 bit. In map_file(), the file size is converted from off_t to ssize_t, which is also a flexible data type. On 32 bit systems, it's generally 32 bit. Therefore, if the database file is larger than 4 GB, a truncation can occur on such 32 bit systems. To fix this, simply check if the value was truncated. I chose to flag it as "out of memory", because you can argue that you are out of memory for such a large file -- even though you will never have enough memory with such an architecture to begin with.
The function mmdb_strndup can return NULL if system runs out of memory. This return value is generally checked, but not in function value_for_key_as_string. Return MMDB_OUT_OF_MEMORY_ERROR instead of MMDB_SUCCESS if that happens.
Values parsed from database are used to calculate required amount of memory. These calculations can overflow and must therefore be checked. In some these cases the usage of calloc() would make the code easier to read again, but would hit performance -- if that is an issue.
Strict validations would lead to errors on 64 bit systems, therefore only check size validation on 32 bit systems. Precisely: check on every architecture that is not 64 bit.
|
The compiler does not like last commit (https://travis-ci.org/maxmind/libmaxminddb/jobs/99864912). Looks like maybe you need some extra parens? I don't have the C precedence table memorized, so maybe it's something else. |
|
The issue is that the conditionals are always false on a 64 bit platform and we have |
|
You both are really really quick with feedback, wow. :) Yes, the issue was that the compiler dislikes checks that are always true. I fixed this with a preprocessor statement, basically what I did with OpenBSD code, too. |
|
This looks reasonable to me. I'll let @autarch merge. |
|
@stoeckmann - I'll look at this on Monday when I'm at work. |
|
I made a branch in the main repo with this change plus another to make the check a single macro. If it passes Travis I'll merge it. Thanks! |
Spotted some issues in libmaxminddb. These ones can be grouped into category "memory allocation".