Dunk:
> Hi,
> Please see latest version of the patch, I tried testing with valgrind under
> Alpine Linux in a Docker container and it didn?t report any errors. But a
> ?postfix stop? exits postfix/valgrind without a summary.
>
> Duncan
Valgrind always produces output. If you don't get any then your
test is invalid.
Can you test this with:
$ cd src/global
$ make mail_dict
cd ../..
$ sh postfix-env.sh valgrind --tool=memcheck src/global/mail_dict
redis:/path/to/file read <<'EOF'
get something-that-exists
get something-that-does-not-exist
EOF
I will wait for real valgrind output before I look at the code.
Wietse
>
> > On 27 Feb 2021, at 23:20, Wietse Venema <[email protected]> wrote:
> >
> > ?Duncan Bellamy:
> >> Hi,
> >>
> >> This patch is based on the original code by Titus Jose on GitHub, I
> >> updated it to work with the development branch and have added some
> >> documentation.
> >
> > Thanks. I have a few suggestions regarding memory leaks, data
> > owbership, and testing.
> >
> > 115 static const char *dict_redis_lookup(DICT *dict, const char *name)
> > 116 {
> > ...
> > 124 result = vstring_alloc(10);
> > 125 VSTRING_RESET(result); /* Not needed -- WZV */
> > 126 VSTRING_TERMINATE(result); /* Not needed -- WZV */
> >
> > I think that you're leaking one VSTRING per dict_redis_lookup()
> > call. With the Postfix DICT variants, a dictionary owns the lookup
> > result (no multi-threading), it is therefore OK to make the result
> > a member of the DICT_REDIS instance.
> >
> > 141 if(dict_redis->c) {
> > 142 reply = redisCommand(dict_redis->c,"GET
> > %s%s",dict_redis->prefix 142 ,name);
> > 143 }
> > 144 else { /* Can't happen - WZV */
> > 145 dict->error = DICT_ERR_CONFIG;
> > 146 }
> >
> > The code in dict_redis_open guarantees that a DICT_REDIS instance
> > never has null DICT_REDIS.c member. You can delete the test and the
> > unreachable branch.
> >
> > 172 DICT *dict_redis_open(const char *name, int open_flags, int dict_flags
> > 172 )
> > 173 {
> > ...
> > 193 c = redisConnect(dict_redis->host,dict_redis->port);
> > 194 if(c->err) { /* Redirect to surrogate -- WZV */
> > 195 msg_fatal("%s:%s: Cannot connect to Redis server %s: %s\n",
> > 196 DICT_TYPE_REDIS, name, dict_redis->host, c->errstr);
> >
> > For consistency with other code, this should
> >
> > 1) Free the 'c'
> > 2) Set dict_redis->c to null and invoke dict_close(&dict_redis)
> > 3) Return dict_surrogate(DICT_TYPE_REDIS, name, ...)
> >
> > 206 static void dict_redis_close(DICT *dict)
> > 207 {
> > 208 DICT_REDIS *dict_redis = (DICT_REDIS *) dict;
> >
> > This function leaks the dict_redis->c pointer.
> > Here, call free dict_redis->c if it is not null.
> >
> > Be sure to test this under VALGRIND. See Makefile
> > for how to build and run mail_dict.c.
> >
> > Wietse
