Skip to content

Fixed libcap with "any" device setting #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fixed libcap with "any" device setting #5

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions in_pcap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,24 @@
#include <stdio.h>

#include <pcap.h>
#include <string.h>

#include "params.h"
#include "in.h"

static pcap_t *p;

static char *device;

int in_init(void)
{
char *device;
char error[PCAP_ERRBUF_SIZE];
struct bpf_program filter;

#ifdef SCANLOGD_DEVICE
device = SCANLOGD_DEVICE;
#else
if (!(device = pcap_lookupdev(error))) {
fprintf(stderr, "pcap_lookupdev: %s\n", error);
return 1;
}
device = "any";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not continue using pcap_lookupdev()? This change looks unrelated to fixing support for the "any" setting, when that is specified in SCANLOGD_DEVICE.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see you wrote in the PR description:

I have also changed the default libpcap interface to "any".

I think if we do this, it should be a separate commit.

BTW, your current commit message is over-long - I suggest that first lines (titles) of commit messages be limited to 72 chars, and subsequent lines (if any) wrapped at 75 (so that it's 79 max after git log indents them by 4).

Copy link
Author

@janw-cz janw-cz Oct 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pi@pi:~/scanlogd $ make libpcap
gcc -Wall -O2 -fomit-frame-pointer -c scanlogd.c
gcc -Wall -O2 -fomit-frame-pointer -I/usr/include/pcap -c in_pcap.c
in_pcap.c: In function ‘in_init’:
in_pcap.c:21:2: warning: ‘pcap_lookupdev’ is deprecated: use 'pcap_findalldevs' and use the first device [-Wdeprecated-declarations]
21 | if (!(device = pcap_lookupdev(error))) {
| ^~
In file included from in_pcap.c:5:
/usr/include/pcap/pcap.h:394:16: note: declared here
394 | PCAP_API char *pcap_lookupdev(char *)
| ^~~~~~~~~~~~~~
gcc -s scanlogd.o in_pcap.o -lpcap -o scanlogd

Copy link
Author

@janw-cz janw-cz Oct 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think that making the "any" as default should work for most users. It is much more useful than picking a random first interface.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if we change the default, that also needs to be reflected in the comment in params.h.

#endif

if (!(p = pcap_open_live(device, sizeof(struct header),
Expand Down Expand Up @@ -64,6 +63,9 @@ void in_run(void (*process_packet)(struct header *packet, int size))
hw_size = 14;
}

if(device == NULL || strcmp(device, "any") == 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent coding style with the rest of the file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you be more specific about what's wrong with this coding style?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The missing space between if and its opening brace is wrong.

Also, while there isn't an exact example like this in scanlogd in particular, other checks generally use !function(...) instead of function(...) == 0. I understand that for strcmp in particular the latter is regarded by many as a more descriptive way to write it, so I don't mind.

hw_size += 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the most important aspect: skipping the 2 bytes unconditionally probably breaks proper behavior with certain older versions of libpcap - ideally, we should investigate that and make this skipping conditional upon libpcap version (if our understanding is correct, or do something different if the understanding is not confirmed).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just tested - the skipping of 2 bytes is needed even with libpcap 0.9.4. So we're good making this change unconditionally, and not skipping was probably always a scanlogd bug.


while (1)
if ((packet = (char *)pcap_next(p, &header))) {
packet += hw_size;
Expand Down