Improve ethernet handling, add PCAP as a network implementation

[nbriggs]

Quite a bit of cleanup in the ethernet, and adds a raw ethernet implementation via libpcap. Fixing the interrupt handling for ethernet also involved the regular I/O interrupts, and display subsystem I/O signals. There is still a fair bit to be done with regard to multiple instances running on the same machine -- scripts to build pairs of virtual ethernet interfaces.

[gemini-code-assist]

Code Review

This pull request introduces a new raw ethernet implementation using the libpcap library (src/ether_pcap.c) and refactors the build system and event loop to support multiple networking backends. The code review identified several critical and high-severity issues in the new implementation, including stack buffer overflows in ether_send and read_Xoption, improper handling of pcap_next_ex return values, potential NULL pointer dereferences, and undefined behavior when calling FD_SET with an invalid file descriptor. Additionally, improvements were suggested for error handling in pcap_create and removing a redundant pre-activation call to pcap_setfilter.

[gemini-code-assist]

There is a critical stack buffer overflow vulnerability in ether_send.

The networkOrderBuffer array is declared with a fixed size of 750 words (1500 bytes):
DLword networkOrderBuffer[750];

However, wordCount is retrieved from Lisp via LispIntToCInt(args[0]) and is not validated before copying up to wordCount elements into networkOrderBuffer:

for (int i = 0; i < wordCount; i++) {
  networkOrderBuffer[i] = htons(GETBASEWORD(bufferAddr, i));
}

If wordCount exceeds 750, this will overflow the stack buffer, leading to memory corruption or arbitrary code execution. We must add a bounds check to ensure wordCount does not exceed 750.

LispPTR ether_send(LispPTR args[])
{
  DLword wordCount;
  DLword *bufferAddr; /* buffer address pointer(in native address) */
#ifdef BYTESWAP
  DLword networkOrderBuffer[750];
#endif
  if (ether_fd < 0) return (NIL);
  wordCount = LispIntToCInt(args[0]);
  if (wordCount > 750) return (NIL);
  bufferAddr = NativeAligned2FromLAddr(args[1]);
#ifdef BYTESWAP
  for (int i = 0; i < wordCount; i++) {
    networkOrderBuffer[i] = htons(GETBASEWORD(bufferAddr, i));
  }
  bufferAddr = networkOrderBuffer;
#endif
  pcap_inject(pcap_handle, bufferAddr, 2 * wordCount);
  ether_out++;
  return (ATOM_T);
} /* ether_send */

[gemini-code-assist]

There are several critical issues in the recvPacket implementation:

1. Incorrect pcap_next_ex Return Value Handling: pcap_next_ex can return PCAP_ERROR_BREAK (-2) or other non-1 values. The current switch statement only handles 0 and PCAP_ERROR. If it returns -2, the code falls through and attempts to dereference header and packet, which are not initialized/set in those cases, leading to a NULL pointer dereference and crash.
2. Unaligned Memory Access: Casting packet to const uint16_t * and dereferencing it as packet[i] can cause unaligned memory access (and a SIGBUS crash) on architectures that do not support unaligned accesses (e.g., SPARC, ARM).
3. Out-of-bounds Read: If hlen is odd, (hlen + 1) / 2 reads one byte past the allocated packet buffer.
4. Type Safety: Changing packet to const u_char * avoids the need for a (void *)&packet cast in pcap_next_ex.

We should explicitly check if the return value is 1 before proceeding, use const u_char * for the packet pointer, and copy bytes safely.

static int recvPacket() {
  bpf_u_int32 hlen;
  const u_char *packet = NULL;
  struct pcap_pkthdr *header = NULL;

  int res = pcap_next_ex(pcap_handle, &header, &packet);
  if (res != 1) {
    if (res == PCAP_ERROR) {
      pcap_perror(pcap_handle, "recvPacket");
    }
    return (0);
  }
  hlen = header->len;
  if (hlen > ether_bsize || hlen == 0) return (0);

#ifdef BYTESWAP
  for (int i = 0; i < hlen / 2; i++) {
    GETBASEWORD((DLword *)ether_buf, i) = (packet[2 * i] << 8) | packet[2 * i + 1];
  }
  if (hlen % 2) {
    GETBASEWORD((DLword *)ether_buf, hlen / 2) = packet[hlen - 1] << 8;
  }
#else
  memcpy(ether_buf, packet, hlen);
#endif

  ether_bsize = 0;
  ((DLETHERCSB *)IOPage->dlethernet)->DLFIRSTICB = hlen;
  return (hlen);
}

[gemini-code-assist]

There are two major issues in this block:

1. Unchecked pcap_findalldevs Return Value: pcap_findalldevs can fail and return -1. If it fails, alldevs is not guaranteed to be valid or NULL. We should check if pcap_rval == 0 before iterating over alldevs and calling pcap_freealldevs.
2. NULL Pointer Dereference: In the loop iterating over dev->addresses, the code dereferences a->addr without checking if it is NULL (e.g., a->addr->sa_family). According to libpcap documentation, a->addr can be NULL for some interfaces or address families. Dereferencing a NULL pointer here will cause a crash. We must check if (a->addr != NULL) before accessing a->addr->sa_family.

  /* select an ethernet interface if one was not provided */
  if (strlen(ether_ifname) == 0) {
    pcap_if_t *alldevs = NULL;
    pcap_if_t *dev = NULL;
    pcap_rval = pcap_findalldevs(&alldevs, errbuf);
    if (pcap_rval == 0) {
      for (pcap_if_t *d = alldevs; d; d = d->next) {
        if ((d->flags & PCAP_IF_UP) && (d->flags & PCAP_IF_RUNNING) && !(d->flags & PCAP_IF_LOOPBACK)) {
          dev = d;
          break;
        }
      }
      /* did we find one */
      if (dev) {
        strlcpy(ether_ifname, dev->name, sizeof(ether_ifname));
        for (pcap_addr_t *a = dev->addresses; a; a = a->next) {
          if (a->addr == NULL) continue;
#if defined(MAIKO_OS_SOLARIS)        
          /* Solaris defines both AF_LINK and AF_PACKET but neither give us
           * the desired result
           */
#elif defined(AF_LINK)
          /* this is BSD-like, macOS? */
          if (a->addr->sa_family == AF_LINK) {
            memcpy(ether_host, LLADDR(((struct sockaddr_dl *)(a->addr))), sizeof(ether_host));
            break;
          }
#elif defined(AF_PACKET)
          /* this is Linux-like */
          if (a->addr->sa_family == AF_PACKET) {
            memcpy(ether_host, ((struct sockaddr_ll *)(a->addr))->sll_addr, sizeof(ether_host));
            break;
          }
#else
#warning Neither AF_LINK nor AF_PACKET address families defined
#endif
        }
      }
      pcap_freealldevs(alldevs);
    } else {
      fprintf(stderr, "pcap_findalldevs failed: %s\n", errbuf);
    }
  }

[gemini-code-assist]

If pcap_get_selectable_fd fails or is not supported on the platform, it returns -1. Calling FD_SET(-1, &LispReadFds) results in undefined behavior (out-of-bounds write to the fd_set bitmask), which can corrupt memory and cause a crash. We must check if ether_fd is non-negative before calling FD_SET.

Suggested change

	pcap_rval = pcap_setnonblock(pcap_handle, 1, errbuf);
	ether_fd = pcap_get_selectable_fd(pcap_handle);
	FD_SET(ether_fd, &LispReadFds);
	pcap_rval = pcap_setnonblock(pcap_handle, 1, errbuf);
	ether_fd = pcap_get_selectable_fd(pcap_handle);
	if (ether_fd < 0) {
	fprintf(stderr, "pcap_get_selectable_fd failed\n");
	pcap_close(pcap_handle);
	return;
	}
	FD_SET(ether_fd, &LispReadFds);

[gemini-code-assist]

There is a stack buffer overflow vulnerability in read_Xoption when parsing the ldex.EtherNet resource. The ifname buffer is declared with a size of 32 bytes: char ifname[32];. However, sscanf uses %s without any width limit: fields = sscanf(tmp, "%x:%x:%x:%x:%x:%x%%%s", ...). If the input string contains more than 31 characters after the % character, it will overflow ifname on the stack. We must limit the string width to %31s to prevent stack buffer overflow.

#if defined(MAIKO_ENABLE_ETHERNET) && defined(USE_PCAP)
  if (XrmGetResource(rDB, "ldex.EtherNet", "Ldex.EtherNet", str_type, &value) == True) {
    int b0, b1, b2, b3, b4, b5, fields;
    char ifname[32];
    (void)strncpy(tmp, value.addr, value.size);
    fields = sscanf(tmp, "%x:%x:%x:%x:%x:%x%%%31s",  &b0, &b1, &b2, &b3, &b4, &b5, ifname);

[gemini-code-assist]

pcap_create returns NULL on failure and fills errbuf. On success, it returns a valid handle, but errbuf might still contain non-empty warning messages. Checking strlen(errbuf) > 0 to detect failure is incorrect because it will abort initialization even if pcap_create succeeded with a warning. We should check if (pcap_handle == NULL) to detect failure, and only print warnings if pcap_handle is non-NULL but errbuf is not empty.

Suggested change

	pcap_handle = pcap_create(ether_ifname, errbuf);
	if (strlen(errbuf) > 0) {
	fprintf(stderr, "%s\n", errbuf);
	return;
	}
	pcap_handle = pcap_create(ether_ifname, errbuf);
	if (pcap_handle == NULL) {
	fprintf(stderr, "pcap_create failed: %s\n", errbuf);
	return;
	}
	if (strlen(errbuf) > 0) {
	fprintf(stderr, "pcap_create warning: %s\n", errbuf);
	}

[gemini-code-assist]

pcap_setfilter must only be called on an activated pcap_t handle. Calling it before pcap_activate (line 389) will fail and return PCAP_ERROR_NOT_ACTIVATED. Since pcap_setfilter is called again with the correct filter after activation (line 406), the pre-activation call is both invalid and redundant and should be removed.

Suggested change

	pcap_rval = pcap_set_immediate_mode(pcap_handle, 1);
	pcap_rval = pcap_set_buffer_size(pcap_handle, 65536);
	pcap_rval = pcap_set_snaplen(pcap_handle, 1518);
	pcap_rval = pcap_setfilter(pcap_handle, &filter_match_none);
	pcap_rval = pcap_activate(pcap_handle);
	/* set up properties on the pcap_handle and then activate it */
	pcap_rval = pcap_set_immediate_mode(pcap_handle, 1);
	pcap_rval = pcap_set_buffer_size(pcap_handle, 65536);
	pcap_rval = pcap_set_snaplen(pcap_handle, 1518);
	pcap_rval = pcap_activate(pcap_handle);

[pamoroso]

On Linux Mint 22.1 Cinnamon it builds and runs fine when networking is not active.

[nbriggs]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new raw Ethernet networking implementation using the libpcap library, refactors display event processing, and updates interrupt state and signal handling. The code review identified several critical issues that need to be addressed, including potential pointer corruption in src/xc.c when setting PENDINGINTERRUPT68k, packet truncation on little-endian systems due to a low MAXETHERPACKETWORDS limit, and an out-of-bounds read vulnerability in src/xrdopt.c from unsafe strncpy usage. Additionally, there is a type mismatch for ether_ifname, fragile handling of pcap_next_ex returns, a potential crash if no suitable network interface is found, and a buffer overflow risk if the selectable file descriptor exceeds FD_SETSIZE.

[gemini-code-assist]

PENDINGINTERRUPT68k is a pointer to a LispPTR (which represents a Lisp object reference). Bitwise ORing ATOM_T into it (*PENDINGINTERRUPT68k |= extended_frame ? ATOM_T : 0;) will corrupt the pointer if it was already non-zero, leading to memory corruption or crashes. It should be set directly using an if statement.

      if (extended_frame) {
        *PENDINGINTERRUPT68k = ATOM_T;
      }

[gemini-code-assist]

MAXETHERPACKETWORDS is defined as 300 (600 bytes). On little-endian (BYTESWAP) systems, any Ethernet packet larger than 600 bytes will be silently truncated to 600 bytes because of the cap if (wordCount > MAXETHERPACKETWORDS) wordCount = MAXETHERPACKETWORDS;. Since standard Ethernet MTU is 1500 bytes (750 words), this will break standard-sized IP or large XNS packets. We should increase MAXETHERPACKETWORDS to at least 1000 to accommodate full-sized Ethernet frames.

Suggested change

	#define MAXETHERPACKETWORDS 300
	#define MAXETHERPACKETWORDS 1000

[gemini-code-assist]

strncpy does not guarantee null-termination if value.size is greater than or equal to sizeof(tmp). If value.addr is not null-terminated or value.size is large, sscanf(tmp, ...) will read out of bounds. We should safely copy and explicitly null-terminate tmp.

    size_t len = (value.size < sizeof(tmp)) ? value.size : (sizeof(tmp) - 1);
    memcpy(tmp, value.addr, len);
    tmp[len] = '\0';
    fields = sscanf(tmp, "%x:%x:%x:%x:%x:%x%%%31s",  &b0, &b1, &b2, &b3, &b4, &b5, ifname);

[gemini-code-assist]

There is a type mismatch for ether_ifname. It is declared as extern u_char ether_ifname[32] here, but declared as extern char ether_ifname[32] in other files (e.g., main.c, xrdopt.c, and ether_pcap.c). Since it holds an interface name (a standard C string), it should be declared as char instead of u_char to avoid compiler warnings and potential undefined behavior.

Suggested change

	extern u_char ether_ifname[32];
	extern char ether_ifname[32];

[gemini-code-assist]

The switch statement on pcap_next_ex is fragile. If pcap_next_ex returns any other value than 0, PCAP_ERROR, or PCAP_ERROR_BREAK (for example, if it returns a different negative value or warning), it will fall through to the packet processing logic, which expects a valid packet. It is much safer and cleaner to only proceed if pcap_next_ex returns exactly 1 (success). Additionally, declaring packet as const u_char * avoids the need for a (void *) cast.

  const u_char *packet = NULL;
  struct pcap_pkthdr *header = NULL;

  if (pcap_next_ex(pcap_handle, &header, &packet) != 1) {
    return (0);
  }

[gemini-code-assist]

If pcap_findalldevs succeeds but no suitable interface is found (i.e., dev remains NULL), ether_ifname remains empty (""). The code then proceeds to call pcap_create("", errbuf), which will fail. It is better to check if ether_ifname is empty after the pcap_findalldevs block and return early with an error message.

    pcap_freealldevs(alldevs);
    if (strlen(ether_ifname) == 0) {
      fprintf(stderr, "No suitable network interface found\n");
      return;
    }
  }

[gemini-code-assist]

If pcap_get_selectable_fd returns a file descriptor >= FD_SETSIZE (1024), calling FD_SET will cause a buffer overflow on LispReadFds. We should check if (ether_fd >= FD_SETSIZE) and handle it gracefully.

Suggested change

	ether_fd = pcap_get_selectable_fd(pcap_handle);
	if (ether_fd == -1) {
	fprintf(stderr, "Couldn't get selectable fd for pcap\n");
	pcap_close(pcap_handle);
	return;
	}
	FD_SET(ether_fd, &LispReadFds);
	ether_fd = pcap_get_selectable_fd(pcap_handle);
	if (ether_fd == -1 || ether_fd >= FD_SETSIZE) {
	fprintf(stderr, "Couldn't get valid selectable fd for pcap\n");
	pcap_close(pcap_handle);
	ether_fd = -1;
	return;
	}
	FD_SET(ether_fd, &LispReadFds);

[nbriggs]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new PCAP-based raw ethernet implementation for XNS and PUP networking, refactors build configurations across CMake and Makefiles to support multiple network backends, and cleans up interrupt and event processing. Feedback on these changes highlights several critical and moderate issues: an uninitialized fd_set in src/ether_sunos.c that could cause undefined behavior; a hardcoded PCAP filter in src/ether_pcap.c that incorrectly blocks PUP packets; ignored return values for pcap_inject and an unused variable devname in the same file; and the use of uint8_t in inc/etherdefs.h and src/ether_common.c without including <stdint.h>, which could lead to compilation errors.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

The local fd_set rfds is declared on the stack but never initialized with FD_ZERO(&rfds) before calling FD_SET(ether_fd, &rfds). This leaves rfds containing random stack garbage. When select is subsequently called with rfds, it will monitor arbitrary file descriptors, leading to undefined behavior, potential crashes, or blocking on unrelated descriptors.

  if (ether_fd < 0) return (NIL);

  FD_ZERO(&rfds);
  FD_SET(ether_fd, &rfds);

[gemini-code-assist]

The variable devname is declared but never used in init_ether. It should be removed to clean up the code.

[gemini-code-assist]

The definition of ether_addr_equal uses uint8_t, but <stdint.h> is not included in src/ether_common.c. To prevent compilation errors on platforms where uint8_t is not defined, use u_char to match the prototype in inc/etherdefs.h and the rest of the ethernet codebase.

Suggested change

	int ether_addr_equal(const uint8_t addr1[6], const uint8_t addr2[6])
	{
	return (0 == memcmp(addr1, addr2, 6));
	int ether_addr_equal(const u_char addr1[6], const u_char addr2[6])
	{
	return (0 == memcmp(addr1, addr2, 6));

[pamoroso]

Still nothing to report at commit b50f976.

[nbriggs]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new raw Ethernet networking implementation using the libpcap library (PCAP), refactoring existing network configurations to unify them under a common MAIKO_ENABLE_ETHERNET flag. It also cleans up event processing by introducing process_Display_events and improves string copying safety in src/xrdopt.c. The review feedback recommends removing an unused variable xfd in src/xinit.c, using const u_char * for the packet pointer in src/ether_pcap.c to avoid unnecessary casts, and adding length checks on AF_LINK and AF_PACKET address structures before copying MAC addresses to prevent potential out-of-bounds reads.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[pamoroso]

Still looking good at commit f7284f2.

[nbriggs]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces raw Ethernet access using the libpcap library (PCAP network implementation) alongside refactoring of the build system, interrupt handling, and I/O event processing. It also improves X resource option parsing safety with a new XrmValueCopy helper. The review feedback suggests robustifying the automatic network interface selection in the PCAP implementation, correcting a potential integer division/overflow calculation in the timer interrupt frequency, and clarifying an error message in the X resource parsing code to refer to the correct resource name.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[pamoroso]

Nothing unusual to report at commit c579084.

[pamoroso]

Still nothing unusual at commit b0706d4.

[pamoroso]

Still looking good at commit 9725309.

[pamoroso]

Still looking good at commit f8f4ddf.

[pamoroso]

Nothing unusual to report at 74cea17.