possible array out of bounds access in sys/netinet/sctp_output.c
Alexander Best
i've been playing with clang tot and noticed the following error:
/usr/local/bin/clang -c -O3 -pipe -fno-inline-functions -fno-strict-aliasing -march=core2 -std=c99 -g -fdiagnostics-show-option -fformat-extensions -Wall -Wcast-qual -Winline -Wmissing-include-dirs -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wredundant-decls -Wstrict-prototypes -Wundef -Wno-pointer-sign -nostdinc -I. -I/usr/git-freebsd-head/sys -I/usr/git-freebsd-head/sys/contrib/altq -D_KERNEL -DHAVE_KERNEL_OPTION_HEADERS -include opt_global.h -fno-omit-frame-pointer -mno-aes -mno-avx -mcmodel=kernel -mno-red-zone -mno-mmx -msoft-float -fno-asynchronous-unwind-tables -ffreestanding -Wno-error=tautological-compare -Wno-error=shift-count-negative -Wno-error=shift-count-overflow -Wno-error=shift-overflow -Wno-error=conversion -Wno-error=empty-body -Wno-error=gnu-designator -Wno-error=format -Wno-error=format-invalid-specifier -Wno-error=format-extra-args -Werror /usr/git-freebsd-head/sys/netinet/sctp_output.c
clang: warning: argument unused during compilation: '-fformat-extensions'
/usr/git-freebsd-head/sys/netinet/sctp_output.c:4685:2: error: array index 1 is past the end of the array (which contains 1 element) [-Werror,-Warray-bounds]
sup_addr->addr_type[1] = htons(SCTP_IPV6_ADDRESS);
^ ~
/usr/git-freebsd-head/sys/netinet/sctp_header.h:84:2: note: array 'addr_type' declared here
uint16_t addr_type[SCTP_ARRAY_MIN_LEN]; /* array of supported address
^
1 error generated.
*** Error code 1
Stop in /usr/obj/usr/git-freebsd-head/sys/GENERIC.
*** Error code 1
Stop in /usr/git-freebsd-head.
*** Error code 1
Stop in /usr/git-freebsd-head.
this is from a GENERIC kernel build (so INET + INET6) for amd64. is this a
false positive, or is length(sup_addr->addr_type) really == 1, thus making
sup_addr->addr_type[1] an illegal access?
cheers.
alex
_______________________________________________
freebsd...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-curre...@freebsd.org"
Jilles Tjoelker
> i've been playing with clang tot and noticed the following error:
> /usr/local/bin/clang -c -O3 -pipe -fno-inline-functions -fno-strict-aliasing -march=core2 -std=c99 -g -fdiagnostics-show-option -fformat-extensions -Wall -Wcast-qual -Winline -Wmissing-include-dirs -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wredundant-decls -Wstrict-prototypes -Wundef -Wno-pointer-sign -nostdinc -I. -I/usr/git-freebsd-head/sys -I/usr/git-freebsd-head/sys/contrib/altq -D_KERNEL -DHAVE_KERNEL_OPTION_HEADERS -include opt_global.h -fno-omit-frame-pointer -mno-aes -mno-avx -mcmodel=kernel -mno-red-zone -mno-mmx -msoft-float -fno-asynchronous-unwind-tables -ffreestanding -Wno-error=tautological-compare -Wno-error=shift-count-negative -Wno-error=shift-count-overflow -Wno-error=shift-overflow -Wno-error=conversion -Wno-error=empty-body -Wno-error=gnu-designator -Wno-error=format -Wno-error=format-invalid-specifier -Wno-error=format-extra-args -Werror /usr/git-freebsd-head/sys/netinet/sctp_output.c
> clang: warning: argument unused during compilation: '-fformat-extensions'
> /usr/git-freebsd-head/sys/netinet/sctp_output.c:4685:2: error: array index 1 is past the end of the array (which contains 1 element) [-Werror,-Warray-bounds]
> sup_addr->addr_type[1] = htons(SCTP_IPV6_ADDRESS);
> ^ ~
> /usr/git-freebsd-head/sys/netinet/sctp_header.h:84:2: note: array 'addr_type' declared here
> uint16_t addr_type[SCTP_ARRAY_MIN_LEN]; /* array of supported address
> ^
> 1 error generated.
> *** Error code 1
>
> Stop in /usr/obj/usr/git-freebsd-head/sys/GENERIC.
> *** Error code 1
>
> Stop in /usr/git-freebsd-head.
> *** Error code 1
>
> Stop in /usr/git-freebsd-head.
> this is from a GENERIC kernel build (so INET + INET6) for amd64. is this a
> false positive, or is length(sup_addr->addr_type) really == 1, thus making
> sup_addr->addr_type[1] an illegal access?
This is the fairly common construct of a variable-length array at the
end of a struct. With C89, this was not allowed but defining one element
and allocating more elements worked in most implementations. C99
recognized this need and created a way to do it, which looks like
uint16_t addr_type[];. This adds any necessary padding and allows access
to however many elements have been allocated. Also, if it is not at the
end of a struct it is an error.
Using this new construct requires code changes because some code such as
fairly close to the error message relies on the size of the one element
already in the struct.
--
Jilles Tjoelker
_______________________________________________
freeb...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net...@freebsd.org"
Michael Tüxen
Hi Jilles,
you are completely right. It is a false positive.
the reason why we don't use addr_type[] is that the same code is used
on different plattforms and (at least at one point of time), using
addr_type[] didn't work there.
However, reconsidering the code right now, I guess one could change to code
in a way to avoid the warning. I'll put that on my ToDo list. But it is only
to avoid the warning, there is no real problem as said earlier.
Best regards
Michael
Bruce Cran
> the reason why we don't use addr_type[] is that the same code is used
> on different plattforms and (at least at one point of time), using
> addr_type[] didn't work there.
Unfortunately I don't think even the Windows 8 Driver Kit will support
much more than stdint.h out of C99.
--
Bruce Cran
Michael Tüxen
On Nov 27, 2011, at 5:56 PM, Davide Italiano wrote:
>> To unsubscribe, send any mail to "freebsd-curre...@freebsd.org"
>>
>
>
> I looked at sctp_send_initiate() and it seems that independently from
> the number of types supported (IPV6/IPV4 or both) two elements are
> allocated in the array sup_addr->addr_type[0] . In case only a type is
> supported one of the elements is simply padding.
> (http://fxr.watson.org/fxr/source/netinet/sctp_output.c#L4670) .
>
> So, this should fix the issue, but maybe I'm wrong so feel free to correct me.
>
> http://davit.altervista.org/sctp_header_types.diff
>
> I defined a new macro mainly because SCTP_ARRAY_MIN_LEN is used in
> another place, i.e. in the field name of struct sctp_host_name_param,
> defined in sctp_header.h). Thanks to arundel@ for testing.
The problem with your fix is that the size of the structure is used in
the code and there are also changes required.
When looking at the code to avoid the warning I realized that the
supported address types parameters will have a wrong length if
the kernel is build with either INET or INET6, but not both.
I've committed a fix in
http://svn.freebsd.org/changeset/base/228031
Best regards
Michael
>
> Regards
>
> Davide