Track bstring/bdestroy

[gwiesenekker]

Hi,

I am not sure where to put this. I recently started using bstring and one of the challenges is avoiding memory leaks. I found the following set of macro's and functions useful to track if bstring/bdestroy are balanced. This code is more meant as a template that could be included into a project by updating the various support macro's and functions.

#define PUSH_LEAK(P) push_leak(#P, __FILE__, __FUNC__, __LINE__);

#define POP_LEAK(P) pop_leak(#P, __FILE__, __FUNC__, __LINE__);

#define BSTRING(B)\
  PUSH_LEAK(#B) bstring B; HARDBUG((B = bfromcstr("")) == NULL);

#define BDESTROY(B) POP_LEAK(#B) HARDBUG(bdestroy(B) == BSTR_ERR)

typedef struct
{
  char *ML_name;
  char *ML_file;
  const char *ML_func;
  int ML_line;
  unsigned long ML_pthread_self;
} my_leak_t;

local int nmy_leaks = 0;
local my_leak_t *my_leaks = NULL;

void print_my_leaks(int arg_all)
{
  for (int ileak = nmy_leaks - 1; ileak >= 0; --ileak)
  {
    if (arg_all or (my_leaks[ileak].ML_pthread_self == compat_pthread_self()))
    {
      PRINTF("ileak=%d name=%s file=%s func=%s line=%d pthread_self=%#X\n",
        ileak,
        my_leaks[ileak].ML_name,
        my_leaks[ileak].ML_file,
        my_leaks[ileak].ML_func,
        my_leaks[ileak].ML_line,
        my_leaks[ileak].ML_pthread_self);
    }
  }
}

void push_leak(char *arg_name, char *arg_file, const char *arg_func,
  int arg_line)
{
  HARDBUG(compat_mutex_lock(&my_leak_mutex) != 0)

  if (nmy_leaks >= NMY_LEAKS_MAX)
  {
    PRINTF("PUSH_LEAK name=%s file=%s func=%s line=%d pthread_self=%#X\n",
           arg_name, arg_file, arg_func, arg_line, compat_pthread_self());

    print_my_leaks(FALSE);

    FATAL("TOO MANY PUSH_LEAKS", EXIT_FAILURE)
  }

  my_leaks[nmy_leaks].ML_name = arg_name;
  my_leaks[nmy_leaks].ML_file = arg_file;
  my_leaks[nmy_leaks].ML_func = arg_func;
  my_leaks[nmy_leaks].ML_line = arg_line;
  my_leaks[nmy_leaks].ML_pthread_self = compat_pthread_self();

  nmy_leaks++;

  HARDBUG(compat_mutex_unlock(&my_leak_mutex) != 0)
}

void pop_leak(char *arg_name, char *arg_file, const char *arg_func,
  int arg_line)
{
  HARDBUG(compat_mutex_lock(&my_leak_mutex) != 0)

  if (nmy_leaks == 0)
  {
    PRINTF("POP_LEAK name=%s file=%s func=%s line=%d pthread_self=%#X\n",
           arg_name, arg_file, arg_func, arg_line, compat_pthread_self());

    FATAL("NO PUSHED LEAKS", EXIT_FAILURE)
  }

  int ileak;

  for (ileak = nmy_leaks - 1; ileak >= 0; --ileak)
    if (my_leaks[ileak].ML_pthread_self == compat_pthread_self()) break;

  if (ileak < 0)
  {
    PRINTF("POP_LEAK name=%s file=%s func=%s line=%d pthread_self=%#X\n",
           arg_name, arg_file, arg_func, arg_line, compat_pthread_self());

    print_my_leaks(FALSE);

    FATAL("POP LEAK DID NOT FIND ANY PUSHED LEAKS", EXIT_FAILURE)
  }

  if (strcmp(my_leaks[ileak].ML_name, arg_name) != 0)
  {
    PRINTF("POP_LEAK name=%s file=%s func=%s line=%d pthread_self=%#X\n",
           arg_name, arg_file, arg_func, arg_line, compat_pthread_self());

    print_my_leaks(FALSE);

    FATAL("POP LEAK DID NOT FIND CORRESPONDING PUSH LEAK", EXIT_FAILURE)
  }

  for (int jleak = ileak; jleak < nmy_leaks; jleak++)
    my_leaks[jleak] = my_leaks[jleak + 1];

  nmy_leaks--;

  HARDBUG(compat_mutex_unlock(&my_leak_mutex) != 0)
}

Regards,
GW

[rdmark]

In my project, Sonarqube static analysis flags potential memory leaks in bdestroy() specifically.

This might be my overly simplistic view of the memory management here, but wouldn't it be a major improvement to be more aggressive about freeing memory in bdestroy() when the data is malformed?

Looking at the current implementation:

int bdestroy (bstring b) {
	if (b == NULL || b->slen < 0 || b->mlen <= 0 || b->mlen < b->slen ||
	    b->data == NULL)
		return BSTR_ERR;

	bstr__free (b->data);

	/* In case there is any stale usage, there is one more chance to
	   notice this error. */

	b->slen = -1;
	b->mlen = -__LINE__;
	b->data = NULL;

	bstr__free (b);
	return BSTR_OK;
}

If slen and mlen are in a bad state for whatever reason, the function bails out and leaves memory allocated. Wouldn't it be sufficient to do a null check on b and b->data?