Possible bug in `freehostent()`

[jwt27]

Tried building with gcc 14 today. It produces some new warnings, mostly bogus (swapped calloc() arguments). But this one looks valid:

i386-pc-msdosdjgpp-gcc @build/djgpp/gcc.arg -o build/djgpp/get_ip.o get_ip.c
get_ip.c: In function 'freehostent':
get_ip.c:252:36: warning: pointer 'p' used after 'free' [-Wuse-after-free]
  252 |   for (p = he->h_addr_list[0]; p; p++)
      |                                   ~^~
get_ip.c:253:7: note: call to 'free' here
  253 |       free (p);
      |       ^~~~~~~~
get_ip.c:255:34: warning: pointer 'p' used after 'free' [-Wuse-after-free]
  255 |   for (p = he->h_aliases[0]; p; p++)
      |                                 ~^~
get_ip.c:256:7: note: call to 'free' here
  256 |       free (p);
      |       ^~~~~~~~

I think this should be:

--- a/src/get_ip.c
+++ b/src/get_ip.c
@@ -240,7 +240,7 @@ ret_copy:

 void W32_CALL freehostent (struct hostent *he)
 {
-  char *p;
+  char **p;

   SOCK_DEBUGF (("\nfreehostent: %s ", he->h_name));

@@ -249,11 +249,11 @@ void W32_CALL freehostent (struct hostent *he)

   DO_FREE (he->h_name);

-  for (p = he->h_addr_list[0]; p; p++)
-      free (p);
+  for (p = he->h_addr_list; *p; p++)
+      free (*p);

-  for (p = he->h_aliases[0]; p; p++)
-      free (p);
+  for (p = he->h_aliases; *p; p++)
+      free (*p);

   free (he->h_aliases);
   free (he->h_addr_list);

[Lethja]

That is a very weird loop. Might be a good idea to see what ccc-analyzer and memcheck report

[sezero]

Why don't you just use an incremented index, e.g.:

diff --git a/src/get_ip.c b/src/get_ip.c
index dd039c1..196f751 100644
--- a/src/get_ip.c
+++ b/src/get_ip.c
@@ -238,7 +238,7 @@ ret_copy:

 void W32_CALL freehostent (struct hostent *he)
 {
-  char *p;
+  int idx;

   SOCK_DEBUGF (("\nfreehostent: %s ", he->h_name));

@@ -248,11 +248,11 @@ void W32_CALL freehostent (struct hostent *he)
   free (he->h_name);
   he->h_name = NULL;

-  for (p = he->h_addr_list[0]; p; p++)
-      free (p);
+  for (idx = 0; he->h_addr_list[idx]; ++idx)
+      free (he->h_addr_list[idx]);

-  for (p = he->h_aliases[0]; p; p++)
-      free (p);
+  for (idx = 0; he->h_aliases[idx]; ++idx)
+      free (he->h_aliases[idx]);

   free (he->h_aliases);
   free (he->h_addr_list);

[gvanem]

That code must be ~15 years old. But no code in Watt-32 is calling it AFAICS. I'll add some program this to test it with everything built with ASAN/UBSAN (busy with work now). Or perhaps this is yet another overzealous GCC warning?

[jwt27]

@sezero:

Why don't you just use an incremented index, e.g.:

Agree, much easier to read.

@gvanem:

That code must be ~15 years old. But no code in Watt-32 is calling it AFAICS. I'll add some program this to test it with everything built with ASAN/UBSAN (busy with work now). Or perhaps this is yet another overzealous GCC warning?

I think this never showed up because compilers already identified the bug, and optimized the whole loop away. They just didn't emit a warning about it.

Test case:

$ cat test.c
#include <stdlib.h>

void test (char **ptr)
{
  for (char *p = ptr[0]; p; p++)
    free (p);
}

$ i386-pc-msdosdjgpp-gcc -O3 -S test.c -o -
        .file   "test.c"
        .section .text
        .p2align 2
        .globl  _test
_test:
        ret
        .ident  "GCC: (GNU) 12.2.0"

Would be curious to see what static analysis / sanitizers have to say about it.

[gvanem]

I've committed a fix for this in https://github.com/gvanem/Watt-32/commit/dca78e7800c0d523c5b67a2a66afdd1556179632. Please test.

[Lethja]

Please test.

The following test code freezes when freehostent is run (tested under Dosbox-X at 9a709c72c4f2aa0357b702269acaac7b3fd192d3)

/*
Compile with:
    wcl -ml -i=inc test.c lib\wattcpwl.lib
    wcl386 -mf -i=inc test.c lib\wattcpwf.lib
*/

#include <stdio.h>
#include <malloc.h>

#include <netdb.h>
#include <arpa/inet.h>
#include <sys/types.h>
#include <sys/socket.h>

void heap_dump() {
    struct _heapinfo h_info;
    int heap_status, it = 0;

    h_info._pentry = NULL;
    for(;;) {
        heap_status = _heapwalk( &h_info );
        if( heap_status != _HEAPOK ) break;
        printf( "  %s block at %Fp of size %4.4X\n",
                (h_info._useflag == _USEDENTRY ? "USED" : "FREE"),
                h_info._pentry, h_info._size );
        ++it;
    }

    switch( heap_status ) {
    case _HEAPEND:
        printf( "OK - end of heap - %d blocks\n", it);
        break;
    case _HEAPEMPTY:
        printf( "OK - heap is empty\n" );
        break;
    case _HEAPBADBEGIN:
        printf( "ERROR - heap is damaged\n" );
        break;
    case _HEAPBADPTR:
        printf( "ERROR - bad pointer to heap\n" );
        break;
    case _HEAPBADNODE:
        printf( "ERROR - bad node in heap\n" );
    }
}

int main() {
    struct hostent *host_info;
    struct in_addr addr;

    puts("Startup");
    heap_dump();
    addr.s_addr = inet_addr("127.0.0.1");

    if(!(host_info = gethostbyaddr((char *)&addr, sizeof(addr), AF_INET))) {
        puts("Couldn't get hostinfo");
        return 1;
    }

    puts("gethostbyaddr() success");
    heap_dump();
    freehostent(host_info);

    puts("freehostent() success");
    heap_dump();

    return 0;
}

[gvanem]

Checking this with clang-cl + ASAN on Windows, I get:

...
OK - end of heap - 936 blocks
=================================================================
==10132==ERROR: AddressSanitizer: attempting free on address which was not malloc()-ed: 0x7ffc15459940 in thread T0
    #0 0x7ffc0bd899c8 in free D:\a\_work\1\s\src\vctools\asan\llvm\compiler-rt\lib\asan\asan_malloc_win.cpp:248
    #1 0x7ffc15217d4a in freehostent E:\WATT\src\get_ip.c:250
    #2 0x7ff78c511503 in main E:\WATT\bin\test-freehostent.c:63
    #3 0x7ff78c52b4a7 in invoke_main D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:78
...

[gvanem]

Seems the cause is simply that fill_hostent() returns static stuff. And freehostent() tries to free() this static stuff. AFAICR, fill_hostent() once tried to calloc() what was returned. But I forgot to fix freehostent() to compensate.

[Lethja]

Checking this with clang-cl + ASAN on Windows

Can Watt-32 be built on Linux for Linux? If so I'd be more than happy to run this and other test code through Valgrind

[gvanem]

for Linux does not make sense currently since Watt-32 would need some special feature (SOCK_PACKET?) to read/write raw eth-packets.

[Lethja]

for Linux does not make sense currently since Watt-32 would need some special feature (SOCK_PACKET?) to read/write raw eth-packets.

If I'm not mistaken doesn't the Windows version of Watt-32 requires Npcap to function? If so libpcap in promiscuous mode can be used as an equivalent. This is one way both 86Box and DosBOX-X can setup their virtual network cards on a Linux host.

I do agree that Watt-32 on Linux is kinda pointless for an actual application since Linux already has a great Berkeley socket implementation but for testing and debugging its rich C eco-system would be fantastic.

As an example: I build a CMocka unit testing framework binary with scan-build make and run it with GDB and/or Valgrind in one of my projects. It's a great combination for verifying that the portable C code being written is and remains infallible

[gvanem]

requires Npcap to function?

Yes. WinPcap or NPcap (and SwsVpkt for Win-XP). But adding libpcap would be a massive change.