search

robmueller / cache-fastmmap

Uses an mmap'ed file to act as a shared memory interprocess cache
http://search.cpan.org/~robm/Cache-FastMmap/
18 stars 15 forks source link

Valgrind and code quality fixes #14

Closed oschwald closed 5 years ago

oschwald commented 5 years ago

While trying to sort out a memory corruption issue, I noticed the following from Valgrind:

   Conditional jump or move depends on uninitialised value(s)
     mmc_unlock (/home/greg/MaxMind/cache-fastmmap/blib/arch/auto/Cache/FastMmap/FastMmap.so) [?:?]
     XS_Cache__FastMmap_fc_unlock (/home/greg/MaxMind/cache-fastmmap/blib/arch/auto/Cache/FastMmap/FastMmap.so) [?:?]
     Perl_pp_entersub (/home/greg/.plenv/versions/5.26.3/bin/perl5.26.3) [?:?]
     Perl_runops_standard (/home/greg/.plenv/versions/5.26.3/bin/perl5.26.3) [?:?]
     Perl_call_sv (/home/greg/.plenv/versions/5.26.3/bin/perl5.26.3) [?:?]
     S_curse (/home/greg/.plenv/versions/5.26.3/bin/perl5.26.3) [?:?]
     Perl_sv_clear (/home/greg/.plenv/versions/5.26.3/bin/perl5.26.3) [?:?]
     Perl_sv_free2 (/home/greg/.plenv/versions/5.26.3/bin/perl5.26.3) [?:?]
     Perl_sv_setsv_flags (/home/greg/.plenv/versions/5.26.3/bin/perl5.26.3) [?:?]
     Perl_pp_sassign (/home/greg/.plenv/versions/5.26.3/bin/perl5.26.3) [?:?]
     Perl_runops_standard (/home/greg/.plenv/versions/5.26.3/bin/perl5.26.3) [?:?]
     perl_run (/home/greg/.plenv/versions/5.26.3/bin/perl5.26.3) [?:?]
     main (/home/greg/.plenv/versions/5.26.3/bin/perl5.26.3) [?:?]
     Uninitialised value was created by a heap allocation
       malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) [?:?]
       mmc_new (/home/greg/MaxMind/cache-fastmmap/blib/arch/auto/Cache/FastMmap/FastMmap.so) [?:?]
       XS_Cache__FastMmap_fc_new (/home/greg/MaxMind/cache-fastmmap/blib/arch/auto/Cache/FastMmap/FastMmap.so) [?:?]
       Perl_pp_entersub (/home/greg/.plenv/versions/5.26.3/bin/perl5.26.3) [?:?]
       Perl_runops_standard (/home/greg/.plenv/versions/5.26.3/bin/perl5.26.3) [?:?]
       perl_run (/home/greg/.plenv/versions/5.26.3/bin/perl5.26.3) [?:?]
       main (/home/greg/.plenv/versions/5.26.3/bin/perl5.26.3) [?:?]

I also noticed some memory leaks and unused variables that I fixed.

There are quite a few places where unsigned integers are compared to 0 or negative numbers.

cache->p_cur is unsigned and gets assigned -1, relying on the underflow behavior. I am not sure if this is intentional, but some of the later comparisons look suspicious if it is.

robmueller commented 5 years ago

There are quite a few places where unsigned integers are compared to 0 or negative numbers.

cache->p_cur is unsigned and gets assigned -1, relying on the underflow behavior. I am not sure if this is intentional, but some of the later comparisons look suspicious if it is.

I'm not sure how I missed this, certainly I'm surprised that I never saw any warnings from the compiler about it.

Perhaps a better idea would be to do something like:

#define NOPAGE ((MU32)~0)

And then use that everywhere instead of -1. I think it would be exactly the same result, but clearer code.

Which comparisons look suspicious to you, I'll take a closer look.

oschwald commented 5 years ago

Some of the sanity checks look suspect, e.g., it->p_cur has a similar issue and there are checks like:

https://github.com/robmueller/cache-fastmmap/blob/090b9f420f0a50698156c19c51217684f62c86cc/mmap_cache.c#L242

I didn't dig into it though.

oschwald commented 5 years ago

Thanks for merging so quickly and doing a release!