AOMediaCodec / libiamf

Reference Software for IAMF
BSD 3-Clause Clear License
39 stars 13 forks source link

Build target defines and targets apart from __linux__ and _WIN32 #95

Closed trevorknight closed 5 months ago

trevorknight commented 6 months ago

iamf_bear_api.h explicitly has three options: linux, APPLE, _WIN32:

#ifdef __linux__
#define EXPORT_API
#elif defined __APPLE__
#define EXPORT_API
#elif defined _WIN32
...
#endif

vlogging_tool_sr.c assumes Windows if not linux or APPLE:

#if defined(__linux__) || defined(__APPLE__)
#include <unistd.h>
#else
#include <fcntl.h>
#include <io.h>
#endif

iamf2bear.cpp only supports linux or _WIN32:

  int count, i;
#ifdef __linux__
  count = readlink("/proc/self/exe", path, buffsize);
#elif _WIN32
  count = GetModuleFileName(NULL, path, buffsize);
#endif
  for (i = count - 1; i >= 0; --i) {
jwcullen commented 6 months ago

1) Looks like a risk of buffer overrun in iamf2bear if neither __linux__ or _WIN32 are defined.

I think if we are using this style it may be better to check all three everywhere it is being used. Use || when it makes sense.

I have seen some build systems that check for the presence of a header and create some kind of HAS_XX_H define that can be used as well. I'm not sure of the best practice here though.

trevorknight commented 6 months ago

When building Wasm, none of those are by default defined, so would be nice to have defaults.

hkchung72 commented 6 months ago

Thanks, it is good job.