Memory Leak in ldns_rr2str

[marty90]

Hi all,

we are using ldns in our software Tstat to have log files for DNS traffic. We were using the ldns_rr2str() function, but we noticed a slow memory leak which was causing our program to saturate the memory in the long term.

We managed to use a workaround. My guess is that the leak is inside the ldns_buffer_export2str() function. Our workaround was based on the re-writing of the ldns_rr2str() as follows:

static inline char * my_ldns_rr2str(ldns_rr * rr){

  ldns_buffer *tmp_buffer = ldns_buffer_new(MAX_STR_DNS);
  if (!tmp_buffer)
    return NULL;

  int ret = ldns_rr2buffer_str_fmt(tmp_buffer, ldns_output_format_default, rr);
  if (ret != LDNS_STATUS_OK ){
    ldns_buffer_free(tmp_buffer);
    return NULL;
  }

  char * rr_str = malloc(tmp_buffer->_position + 1);
  if (!rr_str){
    ldns_buffer_free(tmp_buffer);
    return NULL;
  }

  memcpy(rr_str, tmp_buffer->_data, tmp_buffer->_position);
  rr_str[tmp_buffer->_position] = '\0';
  ldns_buffer_free(tmp_buffer);

  return rr_str;

}

[wtoorop]

Thanks, I'll have a look.

[hardloaf]

Me too and the issue is still there, in the HEAD.

[wtoorop]

Hi, I cannot reproduce. If I run the attached test program rr2str_leak_check.c.gz

through valgrind, it does not detect a memory leak:

willem@makaak:~/sink/ldns-issue104$ make && valgrind --leak-check=full -s ./rr2str_leak_check 
cc -I /usr/local/include -o rr2str_leak_check rr2str_leak_check.c -L /usr/local/lib -lldns
==135034== Memcheck, a memory error detector
==135034== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==135034== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
==135034== Command: ./rr2str_leak_check
==135034== 
test.   3600    IN  TXT "one" "two"

==135034== 
==135034== HEAP SUMMARY:
==135034==     in use at exit: 0 bytes in 0 blocks
==135034==   total heap usage: 26 allocs, 26 frees, 263,803 bytes allocated
==135034== 
==135034== All heap blocks were freed -- no leaks are possible
==135034== 
==135034== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

[wtoorop]

@marty90 , @hardloaf Do you perhaps mean that the way ldns_buffer_set_capacity() is currently implemented (with realloc freeing the trailing unused space of an earlier allocated bigger buffer), uses more memory than:

1. Allocated a new buffer with the smaller size
2. Copy data from the bigger buffer to the smaller buffer.
3. Free the old temporary bigger buffer ?

What does valgrind say if you do not free the string in the above attached test program?

[marty90]

Dear @wtoorop, thank you for handling this. We use ldns in Tstat which is a very large program, so Valgrind was giving me very large outputs. However, I agree that the problem might be rooted in the buffer used for string.

[wtoorop]

Hi @marty90 , NP, sorry to be late with this!

Could you run the program below (linked against ldns) through valgrind on the system in which you detect the memory leak?

#include <stdio.h>
#include <ldns/ldns.h>

int main()
{
    ldns_status s = LDNS_STATUS_OK;
    ldns_rr    *rr = NULL;
    char       *rr_str = NULL;

    if ((s = ldns_rr_new_frm_str(&rr, "test. TXT one two", 0, NULL, NULL)))
        fprintf(stderr, "Could not create rr");

    else if (!(rr_str = ldns_rr2str(rr))) {
        fprintf(stderr, "Error converting rr to string\n");
        s = LDNS_STATUS_ERR;
    } else {
        ldns_rr_free(rr);
        printf("String to free has length: %lu\n", strlen(rr_str));
    }
    if (s) {
        fprintf(stderr, "%s\n", ldns_get_errorstr_by_id(s));
        return EXIT_FAILURE;
    }
    return EXIT_SUCCESS;
}

This program does not free the string on purpose b.t.w. This is to check if the realloc on your system did not resize the initial buffer to the buffer needed for the string. With me 31 bytes are leaked which is exactly what is needed for the string:

willem@makaak:~/sink/ldns-issue104$ valgrind -s --leak-check=full ./rr2str_leak_check 
==194536== Memcheck, a memory error detector
==194536== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==194536== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
==194536== Command: ./rr2str_leak_check
==194536== 
String to free has length: 30
==194536== 
==194536== HEAP SUMMARY:
==194536==     in use at exit: 31 bytes in 1 blocks
==194536==   total heap usage: 26 allocs, 25 frees, 263,803 bytes allocated
==194536== 
==194536== 31 bytes in 1 blocks are definitely lost in loss record 1 of 1
==194536==    at 0x483EFAF: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==194536==    by 0x4A98DFD: ldns_buffer_set_capacity (in /usr/lib/x86_64-linux-gnu/libldns.so.3.0.0)
==194536==    by 0x4AAC8B2: ldns_buffer_export2str (in /usr/lib/x86_64-linux-gnu/libldns.so.3.0.0)
==194536==    by 0x4AAD0D7: ldns_rr2str_fmt (in /usr/lib/x86_64-linux-gnu/libldns.so.3.0.0)
==194536==    by 0x1092D4: main (in /home/willem/sink/ldns-issue104/rr2str_leak_check)
==194536== 
==194536== LEAK SUMMARY:
==194536==    definitely lost: 31 bytes in 1 blocks
==194536==    indirectly lost: 0 bytes in 0 blocks
==194536==      possibly lost: 0 bytes in 0 blocks
==194536==    still reachable: 0 bytes in 0 blocks
==194536==         suppressed: 0 bytes in 0 blocks
==194536== 
==194536== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

[marty90]

Got

==7618== Memcheck, a memory error detector
==7618== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==7618== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==7618== Command: ./dns
==7618== 
String to free has length: 30
==7618== 
==7618== HEAP SUMMARY:
==7618==     in use at exit: 31 bytes in 1 blocks
==7618==   total heap usage: 26 allocs, 25 frees, 263,803 bytes allocated
==7618== 
==7618== 31 bytes in 1 blocks are definitely lost in loss record 1 of 1
==7618==    at 0x4C2FD5F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7618==    by 0x4E4E01D: ldns_buffer_set_capacity (in /usr/lib/x86_64-linux-gnu/libldns.so.1.6.17)
==7618==    by 0x4E6029B: ldns_buffer_export2str (in /usr/lib/x86_64-linux-gnu/libldns.so.1.6.17)
==7618==    by 0x4E60A77: ldns_rr2str_fmt (in /usr/lib/x86_64-linux-gnu/libldns.so.1.6.17)
==7618==    by 0x400919: main (in /home/martino/Bureau/tmp/dns)
==7618== 
==7618== LEAK SUMMARY:
==7618==    definitely lost: 31 bytes in 1 blocks
==7618==    indirectly lost: 0 bytes in 0 blocks
==7618==      possibly lost: 0 bytes in 0 blocks
==7618==    still reachable: 0 bytes in 0 blocks
==7618==         suppressed: 0 bytes in 0 blocks
==7618== 
==7618== For counts of detected and suppressed errors, rerun with: -v
==7618== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

[hardloaf]

Hi Willem and Martino.

In my case this is macOS and this is what Xcode Instruments show as a leak for your test program:

0 libsystem_malloc.dylib malloc_zone_realloc 1 libsystem_malloc.dylib realloc 2 libldns.3.dylib ldns_buffer_set_capacity 3 libldns.3.dylib ldns_buffer_export2str 4 libldns.3.dylib ldns_rr2str_fmt 5 libldns.3.dylib ldns_rr2str 6 ldnstest main /Users/andrey/Desktop/ldnstest/ldnstest/main.c:14 7 libdyld.dylib start

On Mar 03, 21, at 05:01 , Willem Toorop notifications@github.com wrote:

Hi @marty90 https://github.com/marty90 , NP, sorry to be late with this!

Could you run the program below (linked against ldns) through valgrind on the system in which you detect the memory leak?

include

include <ldns/ldns.h>

int main() { ldns_status s = LDNS_STATUS_OK; ldns_rr rr = NULL; char rr_str = NULL;

if ((s = ldns_rr_new_frm_str(&rr, "test. TXT one two", 0, NULL, NULL))) fprintf(stderr, "Could not create rr");

else if (!(rr_str = ldns_rr2str(rr))) { fprintf(stderr, "Error converting rr to string\n"); s = LDNS_STATUS_ERR; } else { ldns_rr_free(rr); printf("String to free has length: %lu\n", strlen(rr_str)); } if (s) { fprintf(stderr, "%s\n", ldns_get_errorstr_by_id(s)); return EXIT_FAILURE; } return EXIT_SUCCESS; } This program does not free the string on purpose b.t.w. This is to check if the realloc on your system did not resize the initial buffer to the buffer needed for the string. With me 31 bytes are leaked which is exactly what is needed for the string:

willem@makaak:~/sink/ldns-issue104$ valgrind -s --leak-check=full ./rr2str_leak_check ==194536== Memcheck, a memory error detector ==194536== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==194536== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info ==194536== Command: ./rr2str_leak_check ==194536== String to free has length: 30 ==194536== ==194536== HEAP SUMMARY: ==194536== in use at exit: 31 bytes in 1 blocks ==194536== total heap usage: 26 allocs, 25 frees, 263,803 bytes allocated ==194536== ==194536== 31 bytes in 1 blocks are definitely lost in loss record 1 of 1 ==194536== at 0x483EFAF: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==194536== by 0x4A98DFD: ldns_buffer_set_capacity (in /usr/lib/x86_64-linux-gnu/libldns.so.3.0.0) ==194536== by 0x4AAC8B2: ldns_buffer_export2str (in /usr/lib/x86_64-linux-gnu/libldns.so.3.0.0) ==194536== by 0x4AAD0D7: ldns_rr2str_fmt (in /usr/lib/x86_64-linux-gnu/libldns.so.3.0.0) ==194536== by 0x1092D4: main (in /home/willem/sink/ldns-issue104/rr2str_leak_check) ==194536== ==194536== LEAK SUMMARY: ==194536== definitely lost: 31 bytes in 1 blocks ==194536== indirectly lost: 0 bytes in 0 blocks ==194536== possibly lost: 0 bytes in 0 blocks ==194536== still reachable: 0 bytes in 0 blocks ==194536== suppressed: 0 bytes in 0 blocks ==194536== ==194536== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NLnetLabs/ldns/issues/104#issuecomment-789697804, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANATKBMXUS66TMJ32APYLDTBYXKHANCNFSM4NTMP4VA.

[hardloaf]

[wtoorop]

Thanks @hardloaf , yes that leak is on purpose. I just wanted to know if it was leaking the big block, or the reallocated small amount.

[wtoorop]

Closing because unable to reproduce