Flux-core: Memory leak in Jtostr

[SteVwonder]

As caught by @nikhil-jain via valgrind in https://github.com/flux-framework/flux-sched/pull/289, the jansson function json_dumps doesn't own the returned string, the caller owns it (and must free it). This was not the case in json-c, which returned an internal string pointer that was still owned by the JSON object.

It seems pretty common place in the flux codebase to call Jtostr within another function call, leaking the string.

[grondo]

Thanks @SteVwonder, good catch!

[garlick]

Note that shortjson.h::Jtostr() in flux-core maps to json-c json_object_to_json_string() which returns a const string that should not be freed.

In flux-sched, shortjansson.h::Jtostr() maps to jansson json_dumps(). Those strings must be freed.

[grondo]

Oh, sorry I didn't remember that. but having the same function behave differently between the 2 projects should be fixed at some point...

[garlick]

We're probably pretty close to being able to transition off of json-c (and shortjson.h) in flux-core.

find . -name \*.[ch] |xargs grep shortjson.h
./t/request/req.c:#include "src/common/libutil/shortjson.h"
./t/request/treq.c:#include "src/common/libutil/shortjson.h"
./t/rpc/rpc.c:#include "src/common/libutil/shortjson.h"
./t/rpc/mrpc.c:#include "src/common/libutil/shortjson.h"
./doc/man3/treduce.c:#include "src/common/libutil/shortjson.h"
./doc/man3/tmrpc_then.c:#include "src/common/libutil/shortjson.h"
./src/cmd/cmdhelp.c:#include "src/common/libutil/shortjson.h"
./src/cmd/flux-comms.c:#include "src/common/libutil/shortjson.h"
./src/cmd/flux.c:#include "src/common/libutil/shortjson.h"
./src/cmd/flux-jstat.c:#include "src/common/libutil/shortjson.h"
./src/modules/wreck/job.c:#include "src/common/libutil/shortjson.h"
./src/modules/wreck/wrexecd.c:#include "src/common/libutil/shortjson.h"
./src/modules/aggregator/aggregator.c:#include "src/common/libutil/shortjson.h"
./src/common/libsubprocess/subprocess.c:#include "src/common/libutil/shortjson.h"
./src/common/libflux/test/module.c:#include "src/common/libutil/shortjson.h"
./src/common/libjsc/jstatctl.c:#include "src/common/libutil/shortjson.h"
./src/common/libutil/tstat.c:#include "shortjson.h"
./src/bindings/lua/flux-lua.c:#include "src/common/libutil/shortjson.h"

we should do that.

[garlick]

So no memory leak here, right?

I'll open a separate bug to remind us to vanquish json-c.

[nikhil-jain]

Yes, the only memory leak Valgrind reported for my experiments with flux-core was the one merged earlier: https://github.com/flux-framework/flux-core/pull/1322.

All leaks related to Jtostr function are in flux-sched.

[garlick]

Thanks! I'll go ahead and close this one then.