[Qemu-devel] Re: Re: completely OT: C Q/As : let's feed the troll

[Charlie Gordon]

> As there is no such thing as a "boolean value" in C, no standard C
> function returns a boolean value.  Some do, however, return an integer
> that may be successfully used in a boolean context (albeit possibly
> negated).

In C there is no boolean type, but there are boolean values : results of
boolean operators as you pointed out.
I agree that most standard C functions do not return booleans values, which
is the cause of numerous bugs by unsuspecting but bold programmers, not
necessarily newbies. look at these examples:

if (isspace(*p) == isspace(*q)) ...   // stupid optimisation!
if (isdir(path1) & isdir(path2)) ...     // lack of C experience or plain
typo

These are horrible things to write, but I've seen much worse examples of non
portable code like this, that do work in a lot of environments, and will
fail without warning on others.
One would need to define an integer type that can only be compared to 0.
strcmp provides even more horrible semantics: C newbies routinely compare
the result to -1 !
Do you know any tool that can detect such horrors ?  I agree with teaching
offenders a lesson, that's what compile time warnings and errors are for.
If you let bugs like this crawl into production software, end users will
become victims, and maintainers will do the hard work...  Original
authors/sinners will have already moved on to other projects, and commit
more trash.

 My point is this : C is difficult enough as a language, try not to add more
traps unnecessarily.

> And just because something doesn't
> generate an error doesn't mean it shouldn't generate a warning.
>

I agree completely, as a matter of fact, I always compile with almost all
warnings on, and make gcc treat them as errors.

> > /* from libast/types.h */
> > ...
> > #undef false
> > #undef False
> > #undef FALSE
> > #undef true
> > #undef True
> > #undef TRUE
> >
> > typedef enum {
> > #ifndef __cplusplus
> >   false = 0,
> > #endif
> >   False = 0,
> >   FALSE = 0,
> > #ifndef __cplusplus
> >   true = 1,
> > #endif
> >   True = 1,
> >   TRUE = 1
> > } spif_bool_t;
> > ...
>
> First off, I'm creating a well-defined API.  I use an enum to create
> an actual type I can return, manipulate, check against, blah blah
> blah.  I undefine those macros to prevent situations where some
> platforms may expand one or more of the LHS values to something that's
> no longer valid for the enum while others may not.
>
> Secondly, libast headers are API headers, and are really not all that
> different from system headers.  If a programmer wishes to use them,
> (s)he should #include them BEFORE any custom headers (s)he may also be
> using.  Nothing is stopping a programmer from redefining those macros
> as (s)he sees fit.  But for my purposes, the correct choice is clear.

In the case of FALSE/TRUE, you know damn well you are not "creating" the
API, you are potentially redefining it, the fact that you first undef those
symbols proves my point.  I cannot understand why you define upto 3
different symbols for each boolean value !
In my original post, I was pointing out how such a redefinition has subtle
side-effects that you probably didn't anticipate.

For completeness, TRUE and FALSE are defined by numerous API headers.
Apache for one has a bogus definition in several header files :
apache/ap_ctx.h:
#ifndef FALSE
#define FALSE 0
#define TRUE  !FALSE
#endif

The missing parentheses should strike as an obvious mistake.
Further thinking may convince you otherwise...
Wrong ! here is a contrived counterexample :

    printf("this should print 98 : TRUE[\"ab\"]=%d\n", TRUE["ab"]);

will print 0 instead of 98 with that definition.

C IS DIFFICULT ! There is always one more bug.

> > While your code is amazingly consistent and readable (except for
> > line length),
>
> LOL  You've got me there; I use GNU Emacs under X11, so my maximum
> line length is largely dependent on how big the Emacs window was in
> which I wrote that code. :-)
>
> Wrapping code at 80 chars can result in some really ugly messes, so I
> gave up on that.  I just haven't found a really good alternative yet.

Good for you !  I can read C code much faster when it doesn't extend beyond
80 columns.
You get ugly messes because you do too much on one line, use too many
verbose macros...

    self->items = SPIF_CAST_C(spif_obj_t *) REALLOC(self->items,
SPIF_SIZEOF_TYPE(obj) * (--(self->len)));

Since REALLOC, and MALLOC always need a cast in C++, why not pass the type ?
Since realloc has despicable behaviour when running out of memory, why not
pass the address of the pointer ?
Then wrap the ugly mess in a macro (you know about that)...
This is much safer, more concise, and fits in 80 columns, and you could even
check the boolean return value :

    REALLOC(&self->items, --self->len, spif_obj_t);

> > you can learn from this very thread and fix it : you use strncpy 10
> > times. 7 of those are incorrect and at least 5 will lead to
> > potential buffer overflow and inefficiencies,
>
> My analysis of the code disagrees, but I'd like to look at any
> evidence you may have.

Lets go:

socket.c line 717
    addr->sun_path[0] = 0;
    strncat(addr->sun_path, SPIF_STR_STR(spif_url_get_path(self)),
sizeof(addr->sun_path));

strncat will copy upto sizeof(addr->sun_path) bytes from the source, and
tack a '\0'.
the destination happens to be empty (why use strncat ?), so this code can
only overflow addr->sun_path by 1 byte, it would be much worse if actual
concatenation took place.

file.c line 51:
    if (len) {
        strncpy(ftemplate, buff, len);
        ftemplate[len - 1] = 0;
    }

this is just inefficient and cumbersome. pstrcpy is what you want.  also len
is a misleading name for the buffer size.

mem.c line 78:

    strncpy(p->file, filename, LIBAST_FNAME_LEN);
    p->file[LIBAST_FNAME_LEN] = 0;

inefficiency again, luckily the buffer size is LIBAST_FNAME_LEN + 1, pstrcpy
fixes this.

mem.c line 135

   strncpy(p->file, filename, LIBAST_FNAME_LEN);

no extra null setting here ! code is inconsistent, I guess you rely on magic
side effects : the ptr_t structure is not allocated here and most likely the
last byte of the file buffer has already been set to 0.  This is unsafe.
pstrcpy fixes this.

strings.c line 99. line 120:

    tmpstr = (char *) MALLOC(cnt + 1);
    strncpy(tmpstr, str, cnt);
    tmpstr[cnt] = 0;
    return (tmpstr);

inefficiency !  pstrcpy fixes this.

conf.c line 485:

                  strncpy(newbuff + j, getenv("HOME"), max - j);
                  cnt1 = strlen(getenv("HOME")) - 1;
                  cnt2 = max - j - 1;
                  j += MIN(cnt1, cnt2);

given CONFIG_BUFF is defined as 20480, (max - j) is likely very large. So
inefficient!
It is also bogus as newbuff is not guarantied to be null terminated.
getting $HOME twice is childish.
relying on MIN to fix the unsigned overflow in case $HOME is the empty
string is nothing to be proud of.
Similar code is repeated 3 more times in the same function with similar
problems.

conf.c line 741:
        if (n > 0 && n <= maxpathlen) {
            /* Compose the /path/file combo */
            strncpy(full_path, path, n);
            if (full_path[n - 1] != '/') {
                full_path[n++] = '/';
            }
            full_path[n] = '\0';
            strcat(full_path, name);

strncpy is OK here, but not needed as memcpy can be used directly.
strcpy(full_path + n, name) would be more efficient as well.

I'll grant you that the bugs are very remote, but that makes them completely
unlikely to be caught
before production.

Just don't use strncpy, steal pstrcpy from here and use it, and propagate to
message !


Cheers,

Charlie Gordon.


PS: please let's stop polluting qemu, address further remarks to me
directly.