Summary: | [PATCH] Fix some compiler warnings in netinet alias module | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | Base System | Reporter: | nikethmurali | ||||||
Component: | kern | Assignee: | freebsd-bugs (Nobody) <bugs> | ||||||
Status: | Open --- | ||||||||
Severity: | Affects Only Me | CC: | markj | ||||||
Priority: | --- | Keywords: | patch | ||||||
Version: | CURRENT | ||||||||
Hardware: | Any | ||||||||
OS: | Any | ||||||||
Attachments: |
|
- Please ensure that local variables are declared at the beginning of the function, per style(9). - FragmentIn() and ProtoAliasIn() may modify the destination address, and with your patch the modification is discarded. So I think the patch is incorrect. Comment on attachment 215410 [details]
Patch for /sys/netinet/libalias/alias.c
Index: alias.c
===================================================================
--- alias.c (revision 361967)
+++ alias.c (working copy)
@@ -443,7 +443,8 @@
IcmpAliasIn(struct libalias *la, struct ip *pip)
{
struct icmp *ic;
- int dlen, iresult;
+ uint16_t dlen;
+ int iresult;
LIBALIAS_LOCK_ASSERT(la);
@@ -740,7 +741,7 @@
{
struct udphdr *ud;
struct alias_link *lnk;
- int dlen;
+ uint16_t dlen;
LIBALIAS_LOCK_ASSERT(la);
@@ -839,7 +840,8 @@
u_short dest_port;
u_short proxy_server_port;
int proxy_type;
- int dlen, error;
+ uint16_t dlen;
+ int error;
LIBALIAS_LOCK_ASSERT(la);
@@ -944,7 +946,7 @@
{
struct tcphdr *tc;
struct alias_link *lnk;
- int dlen;
+ uint16_t dlen;
LIBALIAS_LOCK_ASSERT(la);
@@ -1069,7 +1071,8 @@
static int
TcpAliasOut(struct libalias *la, struct ip *pip, int maxpacketsize, int create)
{
- int dlen, proxy_type, error;
+ uint16_t dlen;
+ int proxy_type, error;
u_short dest_port;
u_short proxy_server_port;
struct in_addr dest_address;
@@ -1343,9 +1346,10 @@
static int
LibAliasInLocked(struct libalias *la, char *ptr, int maxpacketsize)
{
- struct in_addr alias_addr;
+ struct in_addr alias_addr,ip_dst;
struct ip *pip;
int iresult;
+ u_short ip_sum ;
if (la->packetAliasMode & PKT_ALIAS_REVERSE) {
la->packetAliasMode &= ~PKT_ALIAS_REVERSE;
@@ -1398,14 +1402,24 @@
error = find_handler(IN, IP, la, pip, &ad);
if (error == 0)
iresult = PKT_ALIAS_OK;
- else
+ else {
+ ip_dst = pip->ip_dst;
+ ip_sum = pip->ip_sum;
iresult = ProtoAliasIn(la, pip->ip_src,
- &pip->ip_dst, pip->ip_p, &pip->ip_sum);
+ &ip_dst, pip->ip_p, &ip_sum);
+ pip->ip_dst = ip_dst;
+ pip->ip_sum = ip_sum;
+ }
}
break;
- default:
- iresult = ProtoAliasIn(la, pip->ip_src, &pip->ip_dst,
- pip->ip_p, &pip->ip_sum);
+ default: {
+ ip_dst = pip->ip_dst;
+ ip_sum = pip->ip_sum;
+ iresult = ProtoAliasIn(la, pip->ip_src, &ip_dst,
+ pip->ip_p, &ip_sum);
+ }
+ pip->ip_dst = ip_dst;
+ pip->ip_sum = ip_sum;
break;
}
@@ -1421,8 +1435,12 @@
}
}
} else {
- iresult = FragmentIn(la, pip->ip_src, &pip->ip_dst, pip->ip_id,
- &pip->ip_sum);
+ ip_dst = pip->ip_dst;
+ ip_sum = pip->ip_sum;
+ iresult = FragmentIn(la, pip->ip_src, &ip_dst, pip->ip_id,
+ &ip_sum);
+ pip->ip_dst = ip_dst;
+ pip->ip_sum = ip_sum;
}
getout:
@@ -1479,8 +1497,9 @@
)
{
int iresult;
- struct in_addr addr_save;
+ struct in_addr addr_save,ip_src;
struct ip *pip;
+ u_short ip_sum;
if (la->packetAliasMode & PKT_ALIAS_REVERSE) {
la->packetAliasMode &= ~PKT_ALIAS_REVERSE;
@@ -1555,18 +1574,33 @@
error = find_handler(OUT, IP, la, pip, &ad);
if (error == 0)
iresult = PKT_ALIAS_OK;
- else
- iresult = ProtoAliasOut(la, &pip->ip_src,
- pip->ip_dst, pip->ip_p, &pip->ip_sum, create);
+ else {
+ ip_src = pip->ip_src;
+ ip_sum = pip->ip_sum;
+ iresult = ProtoAliasOut(la, &ip_src,
+ pip->ip_dst, pip->ip_p, &ip_sum, create);
+ pip->ip_src = ip_src;
+ pip->ip_sum = ip_sum;
+ }
}
break;
- default:
- iresult = ProtoAliasOut(la, &pip->ip_src,
- pip->ip_dst, pip->ip_p, &pip->ip_sum, create);
+ default: {
+ ip_src = pip->ip_src;
+ ip_sum = pip->ip_sum;
+ iresult = ProtoAliasOut(la, &ip_src,
+ pip->ip_dst, pip->ip_p, &ip_sum, create);
+ pip->ip_src = ip_src;
+ pip->ip_sum = ip_sum;
+ }
break;
}
} else {
- iresult = FragmentOut(la, &pip->ip_src, &pip->ip_sum);
+ ip_src = pip->ip_src;
+ ip_sum = pip->ip_sum;
+ iresult = FragmentOut(la, &ip_src, &ip_sum);
+ pip->ip_src = ip_src;
+ pip->ip_sum = ip_sum;
+
}
SetDefaultAliasAddress(la, addr_save);
Created attachment 215431 [details]
Alternate patch for sys/netinet/libalias/alias.c
Thanks for looking at my patch and catching what I missed.
I've updated the original patch to not drop updated values after function calls.
------------
As an alternate approach to this, I've attached a patch with a slightly different approach (alias.diff.2). I thought this change would make it easier to read.
Please let me know what you think.
(In reply to nikethmurali from comment #3) A couple more comments: Changing dlen to be unsigned seems dangerous. For example, we have: 450 dlen = ntohs(pip->ip_len) - (pip->ip_hl << 2); 451 if (dlen < ICMP_MINLEN) 452 return (PKT_ALIAS_IGNORED); What happens if a malicious packet defines a header length longer than ip_len? If dlen is unsigned, it will end up being a large number and will pass the subsequent check. Regarding the alignment issue, wouldn't it be simpler to modify each of ProtoAliasIn/Out and FragmentIn/Out to take a struct ip * as input, and have them update fields directly? Then those functions know that the ip address fields are not necessarily self-aligned and the compiler can handle it. Keyword: patch or patch-ready – in lieu of summary line prefix: [patch] * bulk change for the keyword * summary lines may be edited manually (not in bulk). Keyword descriptions and search interface: <https://bugs.freebsd.org/bugzilla/describekeywords.cgi> |
Created attachment 215410 [details] Patch for /sys/netinet/libalias/alias.c Patch contains fixes for warnings found when building kernel 1. /usr/src/sys/netinet/libalias/alias.c:472:12: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare] 2. /usr/src/sys/netinet/libalias/alias.c:1559:35: warning: taking address of packed member 'ip_src' of class or structure 'ip' may result in an unaligned pointer value [-Waddress-of-packed-member] 3./usr/src/sys/netinet/libalias/alias.c:1433:42: error: taking address of packed member 'ip_dst' of class or structure 'ip' may result in an unaligned pointer value