Although I originally limited myself to cleaning up code in xine-lib 1.2 branch, I’ve started lately to care a bit more about the 1.1 branch too. The reason for this is that the last few security issues we had were already fixed in the 1.2 series by my
calloc() changes (suggested to me by our great Gentoo Security team more than an year ago), which are now backported.
While I took a pause in the development of the new qt/mpeg4 demuxer (which I’m sure will be quite more solid than the previous one) I’ve resumed removing
xine_xmalloc calls from the code, hopefully soon it will be gone from xine-lib itself at least. I’ll have to check the frontends to see if they do that too.
The interesting part of this job is to actually compare how the size of the code changes when you make some changes around. For instance, when I change a
xine_xmalloc() call with a
calloc() call (because the latter zeroes out the memory area, while
malloc() gives it pristine), the size of the code increase, as the call has two parameters rather than one (in a lot of cases calloc is called with 1 as first parameter). This shouldn’t be a problem considering that
xine_xmalloc() also calls
calloc(), so it’s likely to execute less instructions at the end anyway. On the other hand, I don’t like increasing codesize if I can, so I started doing related cleaning so that
strndup() is used instead of allocating the space and then filling it with
strcpy(), or using
asprintf() rather than allocating and then using
sprintf(), which allowed me to stay about at the same codesize even though
xine_xmalloc() was replaced.
One thing that scares me is that a lot of time the size of the memory area to allocate is first computed in an integer variable. A signed integer variable. You can guess that it isn’t really so safe to do that as you can easily get an overflow on those variables, and then who knows what would happen. Unfortunately I can’t find way for gcc to warn me whenever an alloc function is called with signed parameters. I always try to use
size_t when necessary though.
And instead, a common problem that really needs to be addressed in some way is the fact that a lot of places tend to initialise to zero a memory area right after allocating it. Even when they used
xine_xmalloc() (that always zeroed out the area) and now when they use
calloc() (which also does that). The mild cases of uselessness use(d)
memset() to do their job, which wouldn’t have been that bad, but a few places iterates over the allocated memory area to set it by hand. Which is very much a waste of time and code.
Regarding gcc warnings, -Wstrict-overflow=5 might spit out warnings for risky signed arithmetic, but won’t consider pointer arithmetic.Warnings for risky pointer arithmetic have been added to gcc cvs, and will be present (iiuc) in the next release of each of the 4.2 and 4.3 series.