Details
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 15725 Build 15747: arc lint + arc unit
Event Timeline
sys/netinet/ip_output.c | ||
---|---|---|
1247 | Shouldn't we calculate min(inp->inp_options->m_len, sopt->sopt_valsize) prior to doing the malloc()? I'm a bit unsure what will happen if userland program calls this option with 4T buffer.. |
sys/netinet/ip_output.c | ||
---|---|---|
1247 | Yes, we should, will post fix in a few days. |
sys/netinet/ip_output.c | ||
---|---|---|
1247 | options and len have the same scope. Why do we declare them in different places? |
The latest diff is based on feedback from @jhb via @sbruno :
The output length is an in-out parameter to getsockopt() so that you can determine the size of variable-sized variables (such as this one) with this type of pattern:
socklen_t len; void *buf; len = 0; getsockopt(s, IPPROTO_IP, IP_OPTIONS, NULL, &len); buf = malloc(len); getsockopt(s, IPPROTO_IP, IP_OPTIONS, buf, &len);
(In theory you'd really need to use a loop with realloc until you obtain a consistent snapshot.)
However, for this to work, the kernel code that calls sooptcopyout has to always pass the full size. sooptcopyout ensures it doesn't overflow the user's supplied buffer, but depends on the length it is passed to determine the output value of 'len'. The ulmin breaks this since it will return a len of 0 for the first call always.
I think the right way to fix this is to use m_dup() with M_NOWAIT while holding the INP lock and fail with ENOMEM if m_dup() fails:
INP_RLOCK(inp); if (inp->inp_options) { struct mbuf *options; options = m_dup(inp->inp_options, M_NOWAIT); INP_RUNLOCK(inp); if (options != NULL) { error = sooptcopyout(sopt, mtod(options, char *), options->m_len); m_free(options); } else error = ENOEM; } else { INP_RUNLOCK(inp); sopt->optvalsize = 0; }
(Someone else may prefer m_freem() to m_free() in case inp_options can be an arbitrary chain.)
I chose to always acquire INP_RLOCK because I would have had to test it again under the lock anyway before calling m_dup() to determine ENOMEM vs no valid options, and getsockopt(IP_OPTIONS) isn't a critical path such that it warrants doing an unlocked check before doing a check under the lock.
I think this looks fine of course since it is what I suggested (:-P), but would prefer to have someone else from transport sign off on it as well.