Note that the calls to strncat() did not account for the
NUL terminating byte, and for very long queries could have
led to a buffer overrun.
Signed-off-by: Dan Cross <patchdev@fat-dragon.org>
In the course of removing calls to realloc(), change
the menu parsing and use logic to use ptr_vector's
directly.
This also fixes some detects menu issues in parsing
and avoids e.g. writing to a bad pointer (or should;
of course it needs testing...).
Finally, free menu state on return from the menu_system
function. There was a comment here to do that, but it
didn't appear to be done.
Signed-off-by: Dan Cross <patchdev@fat-dragon.org>
The poorly named `strncpy` was originally written to
copy data into fixed-sized, disk-resident data structures
in an early version of the research Unix kernel. Thus, it
has peculiar semantics: it takes source and destination
pointer arguments and a length and will *always* modify
exactly `length` bytes in the destination buffer. If
the length of the source (which is presumed to be a
NUL-terminated C-stylestring) is `length` or more chars
long, then the result will not be NUL terminated. If it
is less than `length` bytes long, then the result will be
padded with zeros up to `length`.
This is all well and good for storing a file name into a
fixed-width directory entry in 6th edition Unix, but it's
not useful as a general-purpose string utility.
Replaced with calls to strlcpy(), which always properly
terminates the destination but doesn't have the additional
zeroing behavior. Since the buffers that we're copying
into were allocated with malloz(), and thus are guaranteed
to be filled with zeros, we're not leaking data, but not
double-zeroing either.
A few other things were changed. Lengths of destination
buffers are now given via `sizeof` instead of manifest
constants. One call to `memcpy` took the length from the
size of the source argument, thus possibly writing beyond
the end of the destination buffer. Changed to a call to
strlcpy() with length the sizeof destination.
Signed-off-by: Dan Cross <patchdev@fat-dragon.org>
In bluewave.c mostly. There are a few places left where sprintf()
is called directly; these should be recast in terms of a stralloc
or possibly strlcat.
One small whitespace change in www_files.c.
Signed-off-by: Dan Cross <patchdev@fat-dragon.org>
Replace most remaining uses of sprintf() into a `buffer`
variable followed by realloc() and strcat() with direct
use of stralloc.
Signed-off-by: Dan Cross <patchdev@fat-dragon.org>
Specifically, change the www_last10 HTML rendering logic
to use stralloc and strftime(). This eliminates a lot of
duplication.
It would be easier to test this with a unit test if the
logic of reading the last10 entries from a file were
separated from the HTML rendering logic. An area for
future enhancement.
Also start in on www_email.c, which is the last bastion
of significant realloc() use for page generation. An
explicit goal is to get rid of unsafe string handling
functions such as strcpy, strcat, sprintf, etc.
Signed-off-by: Dan Cross <patchdev@fat-dragon.org>
strcat()'ing a string onto the result of file2str()
will result in a buffer overflow, since file2str()
only allocates enough memory to hold the contents of
the file (plus a NUL terminator). This happend in
`bluewave.c`.
Instead, use `file2stralloc` to read the contents of
that file into a stralloc, which we can stralloc_cats
onto without fear of overflow.
Signed-off-by: Dan Cross <patchdev@fat-dragon.org>
Make `blog_load` return a ptr_vector which is
consumed by the code that uses blog entries.
Greatly clean up WWW page generation by using
stralloc and strftime and the ptr_vector
infrastructure.
Needs to be tested. :-)
Signed-off-by: Dan Cross <patchdev@fat-dragon.org>
The pointer vector maintaining `content` in `editor` already
keeps track of the number of lines and makes it available via
a call to `ptr_vector_len` (or one could look at th `len`
member of the ptr_vector struct...this is C, not some fancy
object oriented language with data hiding). Delete the `lines`
local variable and just use ptr_vector_len where necessary.
Sorry; I should have done that in the first sweep through that
code. My bad!
Signed-off-by: Dan Cross <patchdev@fat-dragon.org>
strcalloc_starts() should have tested the return value
of `memcmp` against 0 for equality. Fixed and added a
test case.
As an aside, one might wonder how bugs like that are
creeping into well-tested code imported from other
projects? The answer, specific to stralloc, is that
the original code was very specific to qmail, and used
a number of additional functions specific to qmail.
Rather than import half of qmail, the version imported
into Magicka has been reworked to, instead, use
standard C functions. The process of modifying the
code gave rise to the opportunity for bugs to creep in.
Now that a unit testing framework is in place, we can
test things in isolation more easily and hopefully
catch such things BEFORE they are published to the
master repository.
Signed-off-by: Dan Cross <patchdev@fat-dragon.org>
Replace unsafe string operations (strcpy, strcat,
sprintf, vsprintf) with safe equivalents:
1. The one use of strcpy into an allocated buffer was
replaced with strdup.
2. The one use of strcat was replaced with a call to
memmove and explicitly setting the NUL terminating
byte.
3. sprintf()/vsprintf() calls were replaced with calls
to snprintf()/vsnprintf(), respectively.
Added a Makefile to build the library as, er, a library
and run the test suite.
Signed-off-by: Dan Cross <patchdev@fat-dragon.org>
CuTest is a relatively simple unit testing framework for
C code. It is distributed under the zlib license; this
is an import of the pristine sources.
Signed-off-by: Dan Cross <patchdev@fat-dragon.org>
strcpy()/strcat() are inherently dangerous, even when
used with great care. strlcpy() and strlcat() are
much safer replacements, and are available from OpenBSD
under a very liberal license. Import them and start
using them.
Between pointer vectors, malloz, stralloc and now
strlcpy/strlcat, Magicka has much safer, simpler and
more performant infrastructure for dealing with
strings and dynamic collections of various kinds.
Signed-off-by: Dan Cross <patchdev@fat-dragon.org>
Lots of code in Magicka is involved in dynamic string manipulation.
`stralloc` isn't a bad library for this sort of thing.
Note that this is complements, but doesn't replace, existing string
utilities.
Signed-off-by: Dan Cross <patchdev@fat-dragon.org>
Recast more code in terms of the ptr_vector abstraction.
The mail_menu.c code also made a lot of unnecessary copies
of strings. For example, there was this code sequence:
for (i = z; i < lines - 1; i++) {
free(content[i]);
content[i] = strdup(content[i + 1]);
}
free(content[i]);
lines--;
content = (char **)realloc(content, sizeof(char *) * lines);
Here, `content` represents an array of lines of text.
This code is removing an element from somewhere in that
array (possibly in the middle), and then shifting the
remaining elements over one position.
But observe the calls to `free` and `strdup` in the loop
body: the content is already dynamically allocated. We
free whatever was in the selected position, and then make
*another copy* of the data in the next position to put
into the now-available slot in the array: repeat for the
remainder of the array's elements.
Instead, we could change this code to just shift things
down:
free(content[z]);
for (i = z; i < (lines - 1); ++i)
content[i] = content[i + 1];
--lines;
ncontent = realloc(content, sizeof(char *) * lines);
assert(ncontent == NULL);
content = ncontent;
However, the ptr_vector abstraction provides us a function,
`ptr_vector_del` that deletes an element from the array and
returns the pointer, so we can rewrite this as simply:
free(ptr_vector_del(&content, z));
No additional malloc()/free() required, which means less
pressure on the memory allocator and less copying of data.
Signed-off-by: Dan Cross <patchdev@fat-dragon.org>
Dan Cross
Andrew Pamment