?

Log in

No account? Create an account
For the love of whatever deity you happen to like.... - CERisE's Testing for L

> Recent Entries
> Archive
> Friends
> Profile

August 26th, 2005


Previous Entry Share Next Entry
03:15 pm - For the love of whatever deity you happen to like....
DO NOT USE sprintf()! NEVER, EVER, EVER, EVER, EVER USE sprintf()! USE snprintf()! It'll protect you from buffer overflows!

Furthermore, do not use uninitialized pointers!
I was just looking at code like this:
char* getTmpPath(){
   char* tmpPath;
   char* envDir;
   
   envDir=getenv("SOME_ENV_VAR");

   if(!envDir){
      envDir="tmp";
   }

   sprintf(tmpPath, "%s/%s", getOutDirPath(), envDir);
   return tmpPath;
}



The guy who wrote this code was directed to NOT use malloc for tmpPath, use sprintf() instead of strcat(), and not bother checking for an ending slash.

To this guy's discredit: He didn't initialize his char*'s. He used strcat(), not strncat(). He had no idea why you shouldn't use sprintf() or strcat(). He also poo-poohed my suggestion to check for an ending slash on getOutDirPath() despite the fact that there's no check for it in the code.

I don't recall exactly, but I think the initialization for envDir was "/tmp" as well. =/

To make the lead happy, I suggested:
char* getTmpPath(){
   static char tmpPath[256];
   char* envDir;
   
   envDir=getenv("SOME_ENV_VAR");

   if(!envDir){
      envDir="tmp";
   }

   snprintf(tmpPath, 256, "%s/%s", getOutDirPath(), envdir);
   return tmpPath;
}



This SHOULD have been written:
static char* tmpPath=NULL;
static int tmpLen;

char* getTmpPath(){
   if(tmpPath==NULL){
      char* envDir=getenv("SOME_ENV_VAR");
      int elen;
      char* outDir=getOutDirPath();
      char slash=0;

      if(envDir==NULL){
         envDir="tmp";
         elen=3;
      }else{
         elen=strlen(envDir);
      }

      if(outDir==NULL){
         outDir="/";
         tmpLen=1;
      }else{
         tmpLen=strlen(outDir);
         if(tmpLen==0 || outDir[tmpLen-1]!='/'){
            slash++;
         }
      }

      tmpPath=(char*)malloc(tmpLen+elen+slash+1);
      if(tmpPath!=NULL){
         strncpy(tmpPath, outDir, tmpLen);
         if(slash!=0){
            tmpPath[tmpLen++]='/';
         }
         strncpy(tmpPath+tmpLen, envDir, elen);
         tmpLen+=elen;
         tmpPath[tmpLen++]='\0';
      }else{
         perror("malloc");
      }
   }
   return tmpPath;
}



Completely untested, but I suspect that'll work just fine, so long as someone remembers to free(tmpPath) at the end.

EDITS:
1. A check to determine that olen>0 before accessing olen-1
2. I didn't update tmpLen.
3. olen was eliminated in favor of tmpLen.
4. LJ cuts by popular demand.

(8 comments | Leave a comment)

Comments:


[User Picture]
From:level_head
Date:August 27th, 2005 02:13 am (UTC)
(Link)
*chuckle*

A "pointer": LJ cuts can address the Source of Difficulty.

My C is rough, and from many years ago; I have tended to work in either higher-level languages or in assembly. There are my millions of lines of code. (And writing generators, responsible for -- literally -- many billions of lines of source code. Perhaps trillions by now.)

Still, memory leaks and mismanagement are critical and common failings of low-time programmers.

===|==============/ Level Head
[User Picture]
From:testing4l
Date:August 27th, 2005 02:38 am (UTC)
(Link)
Low time or low quality? My suspicion tends towards the latter. Especially when he'd never heard of snprintf(). I don't know when exactly the rest of the world got the impression that programming this way is OK.

These are people who should know better. I'd have half a mind to waltz into the aforementioned lead's office on Monday and flay her alive. Letting people get away with that code is immoral. Telling them to write it in the first place, doubly so.

Out of curiousity, what HLLs have you been playing with?
[User Picture]
From:level_head
Date:August 27th, 2005 08:07 am (UTC)
(Link)
DataFlex, Sensible Solution, ODBS, Dephi and the best of all: Clarion.

Others in small involvments. And "EasyLanguge" these days, which is a specialized language -- scripting system really -- for financial modeling.

===|==============/ Level Head
[User Picture]
From:aaronlehmann
Date:August 27th, 2005 02:52 am (UTC)
(Link)
Why strncpy instead of memcpy?
[User Picture]
From:testing4l
Date:August 27th, 2005 05:59 am (UTC)
(Link)
Good question!

The main reason is the additional safety blanket of terminating on a null char as well as running through the count. Logically, there's no way it should matter, but it was what I found myself writing.

In principle, I'd be willing to concede that memcpy should be the quicker of the two functions, since it lacks that check, although that may well be a naive view of things. I've never quite trusted libraries since noting that atoi() in glibc is really just a call to strtol().
[User Picture]
From:sakiroa
Date:August 28th, 2005 03:42 am (UTC)
(Link)


I am so sorry.
[User Picture]
From:testing4l
Date:August 31st, 2005 10:27 pm (UTC)
(Link)
You know, I probably made that face, but more disturbed.
[User Picture]
From:philled2thebrim
Date:August 31st, 2005 05:37 am (UTC)
(Link)
totally unrelated, but i thought it would be a good idea to give you my dad's number. (477-7561)

> Go to Top
LiveJournal.com