Parsing ARP cache in C

How can I make this better?

#include 
#include 
#include 
#include 
#include 
#include 
#include 

#define ARP_CACHE       "/proc/net/arp"
#define ARP_BUFFER_LEN  1024
#define ARP_DELIM       " "

int readLine(int Fd, char *Buffer)
{
    if (Fd < 0)
    {
        return -1;
    }

    char ch;
    size_t Read = 0;

    while (read(Fd, (Buffer + Read), 1))
    {
        ch = Buffer[Read];
        if (ch == '\n')
        {
            break;
        }
        Read++;
    }

    if (Read)
    {
        Buffer[Read] = 0;
        return 0;
    }

    return -1;

}

char * getField(char * Line_Arg, int Field)
{
    char * Line = malloc(strlen(Line_Arg)), *ptr;
    memcpy(Line, Line_Arg, strlen(Line_Arg));
    ptr = Line;

    char * s; 
    s = strtok(Line, ARP_DELIM);
    while (Field && s)
    {
        s = strtok(NULL, ARP_DELIM);
        Field--;
    };

    char * ret;
    if (s)
    {
        ret = (char*)malloc(strlen(s) + 1);
        memset(ret, 0, strlen(s) + 1);
        memcpy(ret, s, strlen(s));
    }
    free(ptr);

    return s ? ret : NULL;
}

int main()
{
    int Fd = open(ARP_CACHE, O_RDONLY);

    if (Fd < 0)
    {
        fprintf(stdout, "Arp Cache: Failed to open file \"%s\"\n", ARP_CACHE);
        return 1;
    }

    char Buffer[ARP_BUFFER_LEN];

    /* Ignore first line */
    int Ret = readLine(Fd, &Buffer[0]);

    Ret = readLine(Fd, &Buffer[0]);
    int count = 0;

    while (Ret == 0)
    {
        char * Line;
        Line = &Buffer[0];

        /* Get Ip, Mac, Interface */ 
        char * Ip       = getField(Line, 0);
        char * Mac      = getField(Line, 3);
        char * IfaceStr = getField(Line, 5);

        fprintf(stdout, "%03d: Mac Address of [%s] on [%s] is \"%s\"\n",
                ++count, Ip, IfaceStr, Mac);

        free(Ip);
        free(Mac);
        free(IfaceStr);

        Ret = readLine(Fd, &Buffer[0]);
    }
    close(Fd);
    return Ret;
}

Concept

I don't believe that there is a cross-platform way to read the ARP cache. From the use of /proc filesystem, I deduce that you are targeting Linux. In Linux, you could read /proc/net/arp as you have done, or run the command ip neigh, which does something similar to your program. (On OS X, you could run arp -a -n instead.)

A comment worth a thousand words

You should include a comment like this in your program:

/**
 * /proc/net/arp looks like this:
 *
 * IP address       HW type     Flags       HW address            Mask     Device
 * 192.168.12.31    0x1         0x2         00:09:6b:00:02:03     *        eth0
 * 192.168.12.70    0x1         0x2         00:01:02:38:4c:85     *        eth0
 */

That picture tells me everything I need to know about what you are trying to accomplish — you need hardly explain further. Conversely, not having that picture makes me work a lot harder to reverse-engineer your code.

Minutiae

• &Buffer[0] is usually written as Buffer.

-
You repeat the readLine() call — once before entering the loop, once at the end of the loop. This is a good opportunity to make use of C's support for side-effects.

while (0 == (Ret = readLine(Fd, Buffer))) {
    …
}

-
Since ARP_CACHE is a compile-time constant string, you can just write

fprintf(stdout, "Arp Cache: Failed to open file \"" ARP_CACHE "\"\n");

instead of doing a %s substitution at runtime.

• Reading one byte at a time is wasteful.

• Print errors to standard error; don't contaminate standard output.

• I suggest naming your variables to be consistent with the /proc/net/arp header fields. For example, device instead of IfaceStr. Also, be consistent with capitalization: count starts with lowercase, which is more common.

Big-picture issues

-
The function int readLine(int Fd, char *Buffer) is kind of susceptible to buffer overflow. I can deduce that fact from the function signature: you pass a pointer to a buffer without also passing the size, so it seems unlikely that readLine would know how to stop when the buffer fills up. You could hard-code it to use ARP_BUFFER_LEN as a limit, but it would be unfortunate to cripple an otherwise generic function like that. It would be better to pass in the buffer size explicitly. That's a general pattern you'll see in C APIs: a pointer to a buffer is frequently accompanied by the buffer's size.

In practice, /proc/net/arp should never contain a line long enough to overflow a 1024-byte buffer, so you're safe. Still, you should follow idiomatic C conventions.

• Once you modify readLine() to take a size parameter, you'll find that you've just reinvented fgets().

-
In C, passing strings from a function to its caller is usually avoided, since it involves malloc(), which introduces the potential for memory leaks. Rather, have the caller pass a buffer and size, like how fgets() and scanf() work.

The only good reason to return a string that was allocated using malloc() would be to support arbitrary-length results. At first glance, your getField() accomplishes that, as it allocates a buffer as large as strlen(Line_Arg). (You forgot to add a byte to accommodate the terminating NUL character, by the way.) But, that turns out not to be the case, since Line_Arg itself is not a string of arbitrary length. It's either less than ARP_BUFFER_LEN bytes long (if you got "lucky") or a buffer overflow (as discussed in (1) above).

• You seem to be working very hard at I/O and string processing. Why not just use fscanf()?

The whole program, then, can be simplified to

#include 

/**
 * Macros to turn a numeric macro into a string literal.  See
 * https://gcc.gnu.org/onlinedocs/cpp/Stringification.html
 */
#define xstr(s) str(s)
#define str(s) #s

#define ARP_CACHE       "/proc/net/arp"
#define ARP_STRING_LEN  1023
#define ARP_BUFFER_LEN  (ARP_STRING_LEN + 1)

/* Format for fscanf() to read the 1st, 4th, and 6th space-delimited fields */
#define ARP_LINE_FORMAT "%" xstr(ARP_STRING_LEN) "s %*s %*s " \
                        "%" xstr(ARP_STRING_LEN) "s %*s " \
                        "%" xstr(ARP_STRING_LEN) "s"

int main()
{
    FILE *arpCache = fopen(ARP_CACHE, "r");
    if (!arpCache)
    {
        perror("Arp Cache: Failed to open file \"" ARP_CACHE "\"");
        return 1;
    }

    /* Ignore the first line, which contains the header */
    char header[ARP_BUFFER_LEN];
    if (!fgets(header, sizeof(header), arpCache))
    {
        return 1;
    }

    char ipAddr[ARP_BUFFER_LEN], hwAddr[ARP_BUFFER_LEN], device[ARP_BUFFER_LEN];
    int count = 0;
    while (3 == fscanf(arpCache, ARP_LINE_FORMAT, ipAddr, hwAddr, device))
    {
        printf("%03d: Mac Address of [%s] on [%s] is \"%s\"\n",
                ++count, ipAddr, device, hwAddr);
    }
    fclose(arpCache);
    return 0;
}

StackExchange Code Review Q#58097, answer score: 17