-
Notifications
You must be signed in to change notification settings - Fork 0
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
Hl add dhcp dissection #4
Conversation
A few minor comments, generally looks good. |
@hl33ta should this target |
src/lib/protocols/dhcp.c
Outdated
} else if(id == 15 /* Domain Name */) { | ||
char *name = (char*)&dhcp->options[i+2]; | ||
int j = 0; | ||
|
||
j = ndpi_min(len, sizeof(flow->protos.dhcp.domain_name)-1); | ||
strncpy((char*)flow->protos.dhcp.domain_name, name, j); | ||
flow->protos.dhcp.domain_name[j] = '\0'; | ||
} else if(id == 50) /* Requested IP */ { | ||
if (len > sizeof(flow->protos.dhcp.requested_ip)) | ||
memcpy(&flow->protos.dhcp.requested_ip, (char*)&dhcp->options[i+2], sizeof(flow->protos.dhcp.requested_ip)); | ||
else | ||
memcpy(&flow->protos.dhcp.requested_ip, (char*)&dhcp->options[i+2], len); | ||
} else if(id == 51) /* Lease Time */ { | ||
if (len > sizeof(flow->protos.dhcp.lease_time)) | ||
memcpy(&flow->protos.dhcp.lease_time, (char*)&dhcp->options[i+2], sizeof(flow->protos.dhcp.lease_time)); | ||
else | ||
memcpy(&flow->protos.dhcp.lease_time, (char*)&dhcp->options[i+2], len); | ||
} else if(id == 54) /* Server Identifier */ { | ||
if (len > sizeof(flow->protos.dhcp.server_ident)) | ||
memcpy(&flow->protos.dhcp.server_ident, (char*)&dhcp->options[i+2], sizeof(flow->protos.dhcp.server_ident)); | ||
else | ||
memcpy(&flow->protos.dhcp.server_ident, (char*)&dhcp->options[i+2], len); | ||
} else if(id == 58) /* Renewal Time */ { | ||
if (len > sizeof(flow->protos.dhcp.renew_time)) | ||
memcpy(&flow->protos.dhcp.renew_time, (char*)&dhcp->options[i+2], sizeof(flow->protos.dhcp.renew_time)); | ||
else | ||
memcpy(&flow->protos.dhcp.renew_time, (char*)&dhcp->options[i+2], len); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the below approach as it includes less code. It should be easier to maintain and less prone to error.
} else if(id == 15 /* Domain Name */) { | |
char *name = (char*)&dhcp->options[i+2]; | |
int j = 0; | |
j = ndpi_min(len, sizeof(flow->protos.dhcp.domain_name)-1); | |
strncpy((char*)flow->protos.dhcp.domain_name, name, j); | |
flow->protos.dhcp.domain_name[j] = '\0'; | |
} else if(id == 50) /* Requested IP */ { | |
if (len > sizeof(flow->protos.dhcp.requested_ip)) | |
memcpy(&flow->protos.dhcp.requested_ip, (char*)&dhcp->options[i+2], sizeof(flow->protos.dhcp.requested_ip)); | |
else | |
memcpy(&flow->protos.dhcp.requested_ip, (char*)&dhcp->options[i+2], len); | |
} else if(id == 51) /* Lease Time */ { | |
if (len > sizeof(flow->protos.dhcp.lease_time)) | |
memcpy(&flow->protos.dhcp.lease_time, (char*)&dhcp->options[i+2], sizeof(flow->protos.dhcp.lease_time)); | |
else | |
memcpy(&flow->protos.dhcp.lease_time, (char*)&dhcp->options[i+2], len); | |
} else if(id == 54) /* Server Identifier */ { | |
if (len > sizeof(flow->protos.dhcp.server_ident)) | |
memcpy(&flow->protos.dhcp.server_ident, (char*)&dhcp->options[i+2], sizeof(flow->protos.dhcp.server_ident)); | |
else | |
memcpy(&flow->protos.dhcp.server_ident, (char*)&dhcp->options[i+2], len); | |
} else if(id == 58) /* Renewal Time */ { | |
if (len > sizeof(flow->protos.dhcp.renew_time)) | |
memcpy(&flow->protos.dhcp.renew_time, (char*)&dhcp->options[i+2], sizeof(flow->protos.dhcp.renew_time)); | |
else | |
memcpy(&flow->protos.dhcp.renew_time, (char*)&dhcp->options[i+2], len); | |
} | |
} else if(id == 15 /* Domain Name */) { | |
char *name = (char*)&dhcp->options[i+2]; | |
int j = 0; | |
j = ndpi_min(len, sizeof(flow->protos.dhcp.domain_name)-1); | |
strncpy((char*)flow->protos.dhcp.domain_name, name, j); | |
flow->protos.dhcp.domain_name[j] = '\0'; | |
} else if(id == 50) /* Requested IP */ { | |
memcpy(&flow->protos.dhcp.requested_ip, (char*)&dhcp->options[i+2], ndpi_min(sizeof(flow->protos.dhcp.requested_ip), len)); | |
} else if(id == 51) /* Lease Time */ { | |
memcpy(&flow->protos.dhcp.lease_time, (char*)&dhcp->options[i+2], ndpi_min(sizeof(flow->protos.dhcp.lease_time),len)); | |
} else if(id == 54) /* Server Identifier */ { | |
memcpy(&flow->protos.dhcp.server_ident, (char*)&dhcp->options[i+2], ndpi_min(sizeof(flow->protos.dhcp.server_ident), len)); | |
} else if(id == 58) /* Renewal Time */ { | |
memcpy(&flow->protos.dhcp.renew_time, (char*)&dhcp->options[i+2], ndpi_min(sizeof(flow->protos.dhcp.renew_time), len)); | |
} | |
A final comment intended to improve readability and ease future maintenance. Otherwise LGTM. |
src/lib/protocols/dhcp.c
Outdated
@@ -114,6 +113,7 @@ void ndpi_search_dhcp_udp(struct ndpi_detection_module_struct *ndpi_struct, | |||
|
|||
if(msg_type <= 8) { | |||
foundValidMsgType = 1; | |||
flow->protos.dhcp.valid = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this conflict with use of this field as a flag below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically no it does the same thing as something like this would do flow->protos.dhcp.valid |= 0x01 would do because it happens before the other assignments. but i think replacing the above with flow->protos.dhcp.valid |= 0x01 makes the most sense. good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to consider this in #6 as well ?
adding additional dhcp info to export from ndpi