Fashionable, but unable to tell fact from fiction (testing4l) wrote,
Fashionable, but unable to tell fact from fiction
testing4l

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.
Subscribe
  • Post a new comment

    Error

    default userpic

    Your IP address will be recorded 

    When you submit the form an invisible reCAPTCHA check will be performed.
    You must follow the Privacy Policy and Google Terms of use.
  • 8 comments