0

I have a small program which converts 12 hour time to 24 hour time. 

#include <math.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <assert.h>
#include <limits.h>
#include <stdbool.h>

int get_tokens(char* buf, char *fields[], char *sep){

    char* ptr= (char*)malloc((10*sizeof(char))+1);
        strncpy(ptr, buf, 10);
        *(ptr+10)='\0';
    int num_f=0;

    while((fields[num_f] = strtok(ptr,sep)) != NULL ){
        ptr = NULL;
        num_f++;
    }
    return num_f;
}


char* timeConversion(char* s) {
    char *fields[3];
    int num_f=0;
    char *ptr = (char*) malloc(100*sizeof(char));
    int hour=0;

    get_tokens(s, fields, ":");
    if(strstr(s,"PM")){
        hour=atoi(fields[0])+12;
    }
    else{
      hour=atoi(fields[0]);
    } 

    snprintf(ptr, 9, "%d:%s:%s" ,hour,fields[1],fields[2]);
    return ptr;
}

int main() {
    char* s = (char *)malloc(100 * sizeof(char));
    scanf("%s", s);
    char* result = timeConversion(s);
    printf("%s\n", result);
    return 0;
}

I see a "stack smash" immediately after the timeConversion function returns. I know that the logic of the code works but am not able to figure out the stack smash.

Abhishek Jain
  • 3
  • 4
  • There are at least 3 things wrong with `scanf("%s", s);`. – melpomene Mar 18 '18 at 10:53
  • Don't cast `malloc()`, and multiplying by `sizeof(char)` is pointless because it's `1`. – melpomene Mar 18 '18 at 10:54
  • Don't use `strncpy`. It is an abomination. – melpomene Mar 18 '18 at 10:55
  • `(10*sizeof(char))+1` makes no logical sense. If `sizeof(char)` weren't guaranteed to be 1, this would allocate too little memory. – melpomene Mar 18 '18 at 10:55
  • 1
    @CIsForCookies No, `&s` would be wrong. 1) is indeed the potential buffer overflow (no length limit). 2) is not checking the return value to make sure `scanf` successfully read a string. 3) is using `scanf` for user input at all. `scanf` `%s` in particular is not line based; it will skip over any initial whitespace, then read a sequence of non-whitespace characters, which is annoying to deal with both by the programmer (the rest of the line (including the final newline) stays in the input buffer) and the user (if you just hit Enter, the program seemingly hangs). – melpomene Mar 18 '18 at 11:00
  • Why do you say `strncpy` is an abomination. You only incur any overhead if storage for the destination is much longer than the number of bytes copied, and then it's a simple call to `memset`. Lesser of better alternatives in that case, sure, an abomination, well... [strncpy.c](https://sourceware.org/git/?p=glibc.git;a=blob;f=string/strncpy.c;h=bb2abe30e59ee633b7aeec2846093f57afd0d17c;hb=refs/heads/master) – David C. Rankin Mar 18 '18 at 11:17
  • @melpomene what are you supposed to use instead of strncpy? – Zebrafish Mar 18 '18 at 11:25
  • @DavidC.Rankin When I say abomination, I'm referring to the semantics. It's not a string function, and it does not perform a limited string copy, which is what its name implies. It should definitely not be called `strncpy`, and I would argue that it should not exist as an abstraction in the first place. Its use is too specialized for a place in the standard library. – melpomene Mar 18 '18 at 11:46
  • 1
    @Zebrafish If you actually need what `strncpy` does, a combination of `memcpy` and `memset`. But if you just want to perform a bounded string copy (which is not what `strncpy` does, despite its name), the easiest way is `snprintf`. – melpomene Mar 18 '18 at 11:48
  • I agree whole heartedly with the `snprintf` call. Thanks for the explanation. – David C. Rankin Mar 18 '18 at 11:49
  • Did you find the reason for the error? As VTT says your fields pointers will be overrun, but only if strtok returns over 3 times. When I try 10:30PM it runs fine. What input are you giving it? – Zebrafish Mar 18 '18 at 11:52
  • I am giving the following as input: 10:10:10PM. – Abhishek Jain Mar 20 '18 at 04:37

1 Answers1

0

Function get_tokens writes into stack-allocated buffer with 3 pointers without checking for buffer capacity constraint. fields[num_f] = strtok will potentially cause a stack buffer overrun. And at the same time fields items could be used uninitialized.

There is also a memory leak since you never free allocated memory.

user7860670
  • 35,849
  • 4
  • 58
  • 84