[Asterisk-Dev] Is anyone thinking anymore?

[Rob Gagnon]

I was the one that did all those patches.  Yes, I know some of them look
fairly un-needed.  As Mark has already noted, I too wondered about changing
the so-called "obviously safe" strcpy calls, but opted for the
future-proofing of the code, should someone accidentally change the length
of either the bufffer, or the static string being copied into the buffer.

IE: strncpy(40bytestr, "N/A", strlen(40bytestr) - 1);

Most of changes were legitimate fixes (for example):
IE:  char *somefunction(char *outbuffer, struct ofsomekind *input) {
           strcpy(outbuffer, input->someotherstr);
     }
changed to:
    char *somefunction(char *outbuffer, size_t outbuffersize, struct
ofsomekind *input) {
        strncpy(outbuffer, input->someotherstr, outbuffersize - 1);
        outbuffer[outbuffersize - 1] = '\0';
     }

where it is obvious that the "outbuffer" is now protected from an overrun.

however, the protection was meant for the future.  If someone were to copy
pieces of a function out of one function, create another, and forget to
protect the string you get weird things sometimes (as has happened before,
and been fixed).

In one snippet of code, you could tell someone did a copy and paste of the
guts of some function, made another function, but forgot how to pass the
size of a buffer:
     somefunction(char *buffer, char *someotherthing) {
        snprintf(buffer, sizeof(buffer), "formatstring %s\n",
someotherthing);
   }

here, you could see how the snprintf() was probably copied from some place
else, but of course, now, sizeof(buffer) is 4 instead of what the programmer
meant.

Also caught, were many places, where strncpy() was already in place, but the
dest parameter in the call was not previously nulled out.  IE:  if you use
strncpy, you better make sure that last character is '\0' somehow.  Either
memset() the thing, or initialize the string with char thing[40] ="";

As you already know, the compiler will null out the whole string for you if
you have the ="" in the definition.  If not, you get garbage, and need
something like thing[sizeof(thing)-1] = '\0'; everywhere in the code, which
is messy.

The "pedantic" changes were done to be thorough, attempting to eliminate all
strcpy's in place of the more protected strncpy.  Now if someone were to
make "ast_strncpy()" in utils.c (hint, hint), it would be easier to find
places under which to do the replacements, where the -1, the ending null
character, etc. could all be taken careof without the need for repeated
code.

Rob





----- Original Message ----- 
From: "Karl Brose" <khb at brose.com>
To: <asterisk-dev at lists.digium.com>
Sent: Sunday, July 25, 2004 1:35 AM
Subject: [Asterisk-Dev] Is anyone thinking anymore?


>
> In recent CVS updates we can observe code changes (strangely called
> "source audits") such as those quoted below.
> The old code is perfectly legal, stable, safe, technically sound, and
> readable.
>
> Yet, some people in this community insist on infecting good code with
> nonsense, as in this example.
>
> Folks, just because it's wise to limit string lengths when copying in
> some situations to avoid overflows,
> it doesn't make any sense to just mechanically apply these things to
> every situation without thought and call it "source audits" or what ever.
> chan_sip.c is now littered with this crap.
>
> Is anyone thinking anymore?
>
>
>
>         struct sip_user *user;
> -       char ilimits[40];
> -       char olimits[40];
> +       char ilimits[40] = "";
> +       char olimits[40] = "";
>         char iused[40];
>         char oused[40];
>         if (argc != 3)
> @@ -5359,11 +5375,11 @@
>                 if (user->incominglimit)
>                         snprintf(ilimits, sizeof(ilimits), "%d",
> user->incominglimit);
>                 else
> -                       strcpy(ilimits, "N/A");
> +                       strncpy(ilimits, "N/A", sizeof(ilimits) - 1);
>                 if (user->outgoinglimit)
>                         snprintf(olimits, sizeof(olimits), "%d",
> user->outgoinglimit);
>                 else
> -                       strcpy(olimits, "N/A");
> +                       strncpy(olimits, "N/A", sizeof(olimits) - 1);
>                 snprintf(iused, sizeof(iused), "%d", user->inUse);
>                 snprintf(oused, sizeof(oused), "%d", user->outUse);
>
> _______________________________________________
> Asterisk-Dev mailing list
> Asterisk-Dev at lists.digium.com
> http://lists.digium.com/mailman/listinfo/asterisk-dev
> To UNSUBSCRIBE or update options visit:
>    http://lists.digium.com/mailman/listinfo/asterisk-dev