Prevent zlib from being dynamically loaded

[IlanTruanovsky]

A dynamically allocated pointer was not being properly cleaned up.

I think the ideal way of handling this (if this was in C++) would be to create a "library" class with proper constructors/deconstructors. Is there a particular reason why this is written in C?

This fixes the following Coverity issue:

lib/pkg_editor/src/zlib.c:119:1:
  Type: Resource leak (RESOURCE_LEAK)

lib/pkg_editor/src/zlib.c:98:3:
  1. path: Condition "zlib_interface.deflateInit_", taking false branch.
lib/pkg_editor/src/zlib.c:101:3:
  2. alloc_fn: Storage is returned from allocation function "zlib_load_library".
lib/pkg_editor/src/zlib.c:90:3:
  2.1. path: Condition "dlopen_handle", taking false branch.
lib/pkg_editor/src/zlib.c:93:3:
  2.2. alloc_fn: Storage is returned from allocation function "dlopen".
lib/pkg_editor/src/zlib.c:93:3:
  2.3. return_alloc_fn: Directly returning storage allocated by "dlopen".
lib/pkg_editor/src/zlib.c:101:3:
  3. var_assign: Assigning: "library" = storage returned from "zlib_load_library()".
lib/pkg_editor/src/zlib.c:102:3:
  4. path: Condition "library == NULL", taking false branch.
lib/pkg_editor/src/zlib.c:110:3:
  5. noescape: Resource "library" is not freed or pointed-to in "load_one_symbol".
lib/pkg_editor/src/zlib.c:39:36:
  5.1. noescape: "load_one_symbol(void *, char const *)" does not free or save its parameter "library".
lib/pkg_editor/src/zlib.c:111:3:
  6. noescape: Resource "library" is not freed or pointed-to in "load_one_symbol".
lib/pkg_editor/src/zlib.c:39:36:
  6.1. noescape: "load_one_symbol(void *, char const *)" does not free or save its parameter "library".
lib/pkg_editor/src/zlib.c:112:3:
  7. noescape: Resource "library" is not freed or pointed-to in "load_one_symbol".
lib/pkg_editor/src/zlib.c:39:36:
  7.1. noescape: "load_one_symbol(void *, char const *)" does not free or save its parameter "library".
lib/pkg_editor/src/zlib.c:113:3:
  8. noescape: Resource "library" is not freed or pointed-to in "load_one_symbol".
lib/pkg_editor/src/zlib.c:39:36:
  8.1. noescape: "load_one_symbol(void *, char const *)" does not free or save its parameter "library".
lib/pkg_editor/src/zlib.c:114:3:
  9. noescape: Resource "library" is not freed or pointed-to in "load_one_symbol".
lib/pkg_editor/src/zlib.c:39:36:
  9.1. noescape: "load_one_symbol(void *, char const *)" does not free or save its parameter "library".
lib/pkg_editor/src/zlib.c:115:3:
  10. noescape: Resource "library" is not freed or pointed-to in "load_one_symbol".
lib/pkg_editor/src/zlib.c:39:36:
  10.1. noescape: "load_one_symbol(void *, char const *)" does not free or save its parameter "library".
lib/pkg_editor/src/zlib.c:119:1:
  11. leaked_storage: Variable "library" going out of scope leaks the storage it points to.

[pcolberg]

@IlanTruanovsky The Windows build fails due to a syntax error:

lib\pkg_editor\src\zlib.c(124): error C2143: syntax error: missing ';' before '}'

[pcolberg]

I think the ideal way of handling this (if this was in C++) would be to create a "library" class with proper constructors/deconstructors. Is there a particular reason why this is written in C?

The entire runtime project was initially written in C and only later converted to C++. I doubt that we still have downstream users of pkg_editor written in C; but I am also wary of refactoring this library too much without a substantial reason, e.g., rethinking the choice of data format. Elf is less than optimal for portability to non-POSIX platforms and its API design, e.g., storing data pointers in a struct without a hint on who owns them, the library or the user, would at least complicate attempts to improve memory safety.

[pcolberg]

@IlanTruanovsky Could you summarize your understanding of the Coverity error so we can figure out what needs to be done here? Is the issue only in a non-default, error flow? Or is the zlib handle not unloaded at all, in the default flow?

[IlanTruanovsky]

@IlanTruanovsky Could you summarize your understanding of the Coverity error in words so we can figure out what needs to be done here?

Coverity is complaining about the library variable going out of scope in the zlib_load function. The library variable was loaded in memory using dlopen, and since it goes out of scope, it should be unloaded to prevent a memory leak.

@pcolberg, you made an interesting point in one of the above conversations. We shouldn't be unloading the library, even if it goes out of scope. There should be some other solution.

Is the issue only in a non-default, error flow? Or is the zlib handle not unloaded at all, in the default flow?

The issue would happen in any flow that uses zlib (not sure if this the default or non-default flow). The library is never explicitly
unloaded using any function call.

There must be a simple way to free the library after having called the symbols loaded in from the library. I'll look over it some more and see if I can come up with something.

[pcolberg]

The issue would happen in any flow that uses zlib (not sure if this the default or non-default flow). The library is never explicitly unloaded using any function call.

There must be a simple way to free the library after having called the symbols loaded in from the library. I'll look over it some more and see if I can come up with something.

Alright, so the question boils down to the lifetime of the library handle. The library is loaded on-demand on zlib_deflateInit()/zlib_inflateInit() in acl_pkg_pack()/acl_pkg_unpack_buffer_or_file(), respectively, and is unloaded implicitly on program exit. So the straight-forward solution would be to unload the library in zlib_deflateEnd()/zlib_inflateEnd(). You would store the handle in zlib_interface alongside the resolved library symbols.

However, in a bigger context, I am wondering whether dynamic loading at run-time is still needed at all. As mentioned, the runtime is already linked against zlib. The other consumer of pkg_editor is the compiler.

[pcolberg]

@IlanTruanovsky Now that this path has been proven to work, could you remove the wrapper functions and update the commit message to reflect what and why it is being changed?

You could start with a summary of the Coverity issue, how it could be resolved by storing the library handle alongside the library function pointers and closing the library when the zlib stream is ended; then follow that instead the dynamic loading of zlib may be removed and that the consequence is that the runtime needs to be linked against zlib at build time rather than run-time, which was in fact already the case.

[pcolberg]

You should probably also remove this entire section:

fpga-runtime-for-opencl/lib/pkg_editor/src/pkg_editor.c

Lines 1746 to 1763 in 2425af6

	#else // USE_ZLIB
	int acl_pkg_pack(const char *out_file, const char **input_files_dirs) {
	// Not implemented if no ZLIB
	return 0;
	}
	int acl_pkg_unpack(const char *in_file, const char *out_dir) {
	// Not implemented if no ZLIB
	return 0;
	}
	int acl_pkg_unpack_buffer(const char *buffer, size_t buffer_size,
	const char *out_dir) {
	// Not implemented if no ZLIB
	return 0;
	}

If pkg_editor is compiled without zlib, then it is preferable to have an error at build- rather than run-time if these functions are being called. For a shared library this might be different, but pkg_editor is merely a static library and its consumers are aware whether zlib is available or not since they have provide the link dependency in the first place.

[pcolberg]

Thanks @IlanTruanovsky!