3
\$\begingroup\$

This is a simple packet sniffer that turns on LEDs when there's network activity. It works by picking up filters in a configuration file and listen for activity in those filters. When there's a captured packet, it sets a bit in a bit-mask that corresponds to the filter index in the config file. Then, once all the filters have been polled, it sends the bit-mask to an LED device ( in this case, an Arduino ) and the lights turn on.

I'm just looking for suggestions or any sort of feedback on the code. All comments are welcome.

#include "main.h"

int close_signal = 0;

int main( int argc, char * argv[] ){

    struct filter_node * head = 0;
    FILE * led_device = 0;
    FILE * config_file = 0;
    char * line_buffer = 0;
    char errbuf[ PCAP_ERRBUF_SIZE ];
    char * dev = 0;
    bpf_u_int32 maskp;
    bpf_u_int32 netp;

    unsigned int led_mask = 0;

    printf( "Starting %s..\n", argv[ 0 ] );

    /* Daemonize */ {
        pid_t p_id = fork( );   // start in new process
        if( p_id < 0 ){
            printf( "Couldn't start %s\n", argv[ 0 ] );
            return -1;
        }
        else if( p_id > 0 ) return 0;

        if( setsid( ) < 0 ){        // disconnect from terminal
            if( errno != EPERM ){   // EPERM == already disconnected
                printf( "Couldn't start %s\n", argv[ 0 ] );
                return -1;
            }
        }

        int i = 0;  // close standard io
        for( ; i < 3; i++ ) close( i );

        // only keep stdout, redirected to a file
        if( open( NULL_DEVICE, O_RDONLY             ) < 0 ) return -1;
        if( open( OUTPUT_FILE, O_WRONLY | O_CREAT | O_TRUNC ) < 0 ) return -1;
        if( open( NULL_DEVICE, O_WRONLY             ) < 0 ) return -1;

        // sets 'close' to true so that a cleanup
        // can occur before process exits
        if( signal( SIGTERM, exit_handler ) == SIG_ERR ){
            printf( "Error creating exit handler!\n" );
            return -1;
        }
    }

    printf( "%s is daemonized\nInitializing packet sniffers..\n", argv[ 0 ] );

    led_device = fopen( LED_DEVICE, "rw+" );

    if( !led_device ){
        printf( "Error opening %s\n", LED_DEVICE );
        return -1;
    }

    config_file = fopen( CONFIG_FILE, "r+" );

    if( !config_file ){
        printf( "Error opening %s\n", CONFIG_FILE );
        fclose( led_device );
        return -1;
    }

    struct filter_node * temp = 0;
    int i = 0;

    // for reading lines of the config file
    line_buffer = malloc( LINE_BUFFER_SIZE );

    if( !line_buffer ){
        printf( "Couldn't allocate memory for %s\n", argv[ 0 ] );
        fclose( config_file );
        fclose( led_device );
    }

    // look up netmask of the network
    if( pcap_lookupnet( 0, &netp, &maskp, errbuf ) < 0 ){
        printf( "Error looking up netmask: %s\n", errbuf );
        return -1;
    }

    // each line of the file represents a network filter
    for( ; fgets( line_buffer, LINE_BUFFER_SIZE, config_file ); i++ ){

        printf( "Starting sniffer %d for %s", i, line_buffer );

        // create node in linked list
        temp = malloc( sizeof( struct filter_node ) );

        if( !temp ){
            printf( "Couldn't allocate memory!\n" );
            continue;
        }

        // create a handle for each filter
        temp->cap_handle = pcap_open_live( 0, BUFSIZ, 1, -1, errbuf );
            if( !temp->cap_handle ){
            printf( "pcap_open_live(): %s\n" , errbuf );
            free( temp );
            continue;
        }

        // compile filter
        if( pcap_compile( temp->cap_handle, &temp->fp, line_buffer, 0, netp ) < 0 ){
             printf( "Error compiling expression: %s", line_buffer );
             pcap_close( temp->cap_handle );
             free( temp );
             continue;
        }

        // assign filter to handle
        if( pcap_setfilter( temp->cap_handle, &temp->fp ) < 0 ){
            printf( "Error setting filter: %s", line_buffer );
            pcap_close( temp->cap_handle );
            pcap_freecode( &temp->fp );
            free( temp );
            continue;
        }

        // set to non blocking calls
        // this is so that each filter can be checked
        // in the same instance of time
        if( pcap_setnonblock( temp->cap_handle, 1, errbuf ) < 0 ){
            printf( "Error setting Non-block mode: %s\n", errbuf );
            pcap_close( temp->cap_handle );
            pcap_freecode( &temp->fp );
            free( temp );
            continue;
        }

        temp->index = i;   // assign the index of the node
        temp->next = head; // add to filter list
        head = temp;       // set head of filter list
    }

    fclose( config_file ); // no longer need config file
    free( line_buffer );   // no longer need line buffer

    while( !close_signal ){ // main loop, stopped by signal handler
        for( temp = head, led_mask = 0; temp; temp = temp->next ){ // traverse filter list

            if( pcap_dispatch( temp->cap_handle, // acknowledge all packets
                               -1,
                               capture_callback,
                               ( u_char * ) &temp->index ) > 0 )

                led_mask |= ( 1 << temp->index ); // set bit index in led_mask
            }
            if( led_mask ){ // when all filters have been polled, send the led mask to device
                if( fwrite( &led_mask, 1, 1, led_device ) > 0 ) fflush( led_device );
            }
    }

    // the program was signaled
    // close the led device
    fclose( led_device );

    // free filter, capture handle, and memory of
    // all nodes in the filter list
    while( head ){
        printf( "Closing sniffer %d\n", head->index );
        pcap_freecode( &head->fp );
        temp = head->next;
        free( head );
        head = temp;
    }

    // confirm program has closed correctly
    printf( "%s closed\n", argv[ 0 ] );
    return 0;
}

void capture_callback( u_char * index, const struct pcap_pkthdr* pkthdr, const u_char * packet ){
// nothing to be done here
}

void exit_handler( int sig ){
    close_signal = 1; // closes main loop
}

And if anyone is interested, the entire project is on Google Code.

\$\endgroup\$
2
\$\begingroup\$

Your code looks nice and compiles cleanly (once I filled in the blanks). I have a few comments, the main one being your lack of functions. Putting everything in main is not practicable beyond a certain size and you have exceeded that size. Various functions should be extracted from main, including at least the daemonize code, creating each filter node and the main loop. So main might look something like (simplified):

    daemonize();
    head = configure_filter_chain();
    if (head) {
        int led = open_led();
        while (!close_signal) {
            filter(head, led);
        }
    }

Some detailed comments:

  • use perror to print errors where errno has been set. This prints to stderr, not stdout, so you will need to open a descriptor for that.

  • do you need to open descriptors for unused stdin/stderr (or stdin/stdout - see previous point)? Are they used? If not why not just fclose(stdin)?

  • it is better to open resources only when you need them. For example you opened a file on the LED device long before it is needed.

  • why use buffered i/o on the LED? Wouldn't unbuffered (ie open/write) be more suitable than buffered (fopen/fwrite/fflush)?

  • the content of your big for-loop has a few issues. For a start, it should probably be a function. Also, you allocate memory before it is needed, and hence have to free it at several points. And your error recovery (closing resources obtained) is duplicated. Something like the following would be neater, where create_node allocates a new node and adds it to the linked list:

    static void create_filter_node(const char *line, bpf_u_int32 netp, struct filter_node **head)
    {
        char errbuf[PCAP_ERRBUF_SIZE];
        struct bpf_program fp;
        int compiled = -1;
    
        pcap_t *handle = pcap_open_live(0, BUFSIZ, 1, -1, errbuf);
        if (!handle){
            fprintf(stderr, "..." , errbuf);
        }
        else if ((compiled = pcap_compile(handle, &fp, line, 0, netp)) < 0){
            fprintf(stderr, "..." , line);
        }
        else if (pcap_setfilter(handle, &fp) < 0){
            fprintf(stderr, "..." , line);
        }
        else if (pcap_setnonblock(handle, 1, errbuf) < 0){
            fprintf(stderr, "..." , errbuf);
        }
        else if (create_node(handle, fp, i, head)) {
            return;  // ie. Success!
        }
    
        if (handle) {
            pcap_close(handle);
        }
        if (compiled == 0) {
            pcap_freecode(&fp);
        }
    }
    

    Some people use goto statements to handle this sort of clean-up, instead of the if-else-if chain. In this case that seems unnecessary.

And some pedantic comments

  • Adding spaces inside brackets is unusual. It is more normal to write if (condition) rather than if( condition )

  • dev is unused

  • on failure it is normal to return EXIT_FAILURE (from stdlib) not -1

  • define loop variables in the for-loop if possible, so for (int i=0; ...)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thanks! I'll be going through your points as I rewrite the code. When I finish editing the source, should I edit this question for more review? or, should I post a new review? \$\endgroup\$ – tay10r Apr 29 '13 at 19:11
  • \$\begingroup\$ Personally I'd post a new review to keep it separate. You are more likely to get a follow-up review that way I expect. But the choice is yours :-) BTW, you are likely to get other reviews here, so perhaps wait to see what others have to say before taking my advice as gospel. \$\endgroup\$ – William Morris Apr 29 '13 at 19:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.