Skip to content
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.

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

Added changes to permit pppoe interfaces #38

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
53 changes: 39 additions & 14 deletions src/ifvc.c
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,10 @@ void rebuildIfVc () {
**
*/
void buildIfVc(void) {
struct ifreq IfVc[ sizeof( IfDescVc ) / sizeof( IfDescVc[ 0 ] ) ];
struct ifreq IfVc[ MAX_IF * 2 ]; /* allocate in double in order to get as much information as possible */
Copy link
Owner

Choose a reason for hiding this comment

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

Why *2? Maximal number is maximal. So either defined constant is not so large or there is another hidden problem?

struct ifreq *IfEp;
struct IfDesc *dp;
struct SubnetList *allowednet, *currsubnet;
struct Config *config = getCommonConfig();

int Sock;
Expand Down Expand Up @@ -246,6 +248,19 @@ void buildIfVc(void) {
if (IfNext < IfPt + 1)
IfNext = IfPt + 1;

/* don't retrieve any further info if MAX_IF is reached
*/
if ( IfDescEp >= &IfDescVc[ MAX_IF ] ) {
my_log(LOG_DEBUG, 0, "Too many interfaces, skipping all since %s", IfPt->ifr_name);
break;
}

/* don't retrieve any info from non-IPv4 interfaces
*/
if ( IfPt->ifr_addr.sa_family != AF_INET ) {
my_log(LOG_DEBUG, 0, "Interface is not AF_INET, skipping %s (family %d)", IfPt->ifr_name, IfPt->ifr_addr.sa_family);
continue;
Copy link
Owner

Choose a reason for hiding this comment

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

Skipping interfaces without IPv4 address would mean igmpproxy itself would not see this interface. Why such change is needed? Previously these interfaces was normally processed just marked as non-IPv4.

}
strncpy( IfDescEp->Name, IfPt->ifr_name, sizeof( IfDescEp->Name ) );

// Currently don't set any allowed nets...
Expand All @@ -254,14 +269,6 @@ void buildIfVc(void) {
// Set the index to -1 by default.
IfDescEp->index = (unsigned int)-1;

/* don't retrieve more info for non-IP interfaces
*/
if ( IfPt->ifr_addr.sa_family != AF_INET ) {
IfDescEp->InAdr.s_addr = 0; /* mark as non-IP interface */
IfDescEp++;
continue;
}

// Get the interface adress...
IfDescEp->InAdr.s_addr = s_addr_from_sockaddr(&IfPt->ifr_addr);
addr = IfDescEp->InAdr.s_addr;
Expand All @@ -274,6 +281,21 @@ void buildIfVc(void) {
mask = s_addr_from_sockaddr(&IfReq.ifr_addr); // Do not use ifr_netmask as it is not available on freebsd
subnet = addr & mask;

dp = getIfByName(IfPt->ifr_name, 1);
Copy link
Owner

Choose a reason for hiding this comment

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

What is the purpose of function parameter which is always constant?

Copy link
Author

Choose a reason for hiding this comment

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

It is to be able to take advantage of the function so that it also works in the same way as it did in the past.

Copy link
Owner

Choose a reason for hiding this comment

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

But that function is not called from other places. Such change introduce a dead code as parameter is never false.

if (dp != NULL && dp->allowednets != NULL) {
allowednet = (struct SubnetList *)malloc(sizeof(struct SubnetList));
if (allowednet == NULL) my_log(LOG_ERR, 0, "Out of memory !");
allowednet->next = NULL;
allowednet->subnet_mask = mask;
allowednet->subnet_addr = subnet;
currsubnet = dp->allowednets;
while (currsubnet->next != NULL)
currsubnet = currsubnet->next;
currsubnet->next = allowednet;
continue;
Copy link
Owner

Choose a reason for hiding this comment

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

This completely skip code which is below. Why?

Looks like this is a hack which just hide the fact that there is a probably bug in code below and try to invisibly skip it. This change is non-intuitive and introduce (potentially) dead code. Also probably may introduce another bugs as parts like IfDescEp++; are not executed...

}


/* get if flags
**
** typical flags:
Expand Down Expand Up @@ -333,13 +355,16 @@ void buildIfVc(void) {
** - NULL if no interface 'IfName' exists
**
*/
struct IfDesc *getIfByName( const char *IfName ) {
struct IfDesc *getIfByName( const char *IfName, int iponly ) {
struct IfDesc *Dp;

for ( Dp = IfDescVc; Dp < IfDescEp; Dp++ )
if ( ! strcmp( IfName, Dp->Name ) )
return Dp;

for ( Dp = IfDescVc; Dp < IfDescEp; Dp++ ) {
if (iponly && Dp->InAdr.s_addr == 0)
continue;
if ( ! strcmp( IfName, Dp->Name ) )
return Dp;
}

return NULL;
}

Expand Down
2 changes: 1 addition & 1 deletion src/igmpproxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ extern int upStreamIfIdx[MAX_UPS_VIFS];
*/
void rebuildIfVc( void );
void buildIfVc( void );
struct IfDesc *getIfByName( const char *IfName );
struct IfDesc *getIfByName( const char *IfName, int iponly );
struct IfDesc *getIfByIx( unsigned Ix );
struct IfDesc *getIfByAddress( uint32_t Ix );
struct IfDesc *getIfByVifIndex( unsigned vifindex );
Expand Down