1
\$\begingroup\$

I write this program that checks if a file is executable, and if so, to check if it is an ELF binary or a shell script.

#include <unistd.h>
#include <stdio.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <string.h>

int Search_in_File(char *fname, char *str);

int main(int argc, char **argv)
{
    if(argc < 2){
        printf("The path to the file is\n");
        return 1;
    }

    struct stat fileStat;

    if(stat(argv[1],&fileStat) < 0){
        printf("error\n");
        return 1;
    }

    if(S_ISREG(fileStat.st_mode)){
        if(fileStat.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH))
        {
            printf("The file is executable ");
            Search_in_File(argv[1], "#!");
            Search_in_File(argv[1], "ELF");
        }
        else
        {
            printf("The file is not executable.\n");
        }
    }
    else
        printf("Not a file\n");
    return 0;
}

int Search_in_File(char *fname, char *str) {
    FILE *fp;
    char temp[512];

    if((fp = fopen(fname, "r")) == NULL) {
        return(-1);
    }

    while(fgets(temp, 512, fp) != NULL) {
        if((strstr(temp, str)) != NULL) {
            if(strcmp(str, "#!")==0)
                printf("shell type.");
            else if(strcmp(str, "ELF")==0)
                printf(" ELF.");
        }
    }
    printf("\n");
    if(fp) {
        fclose(fp);
    }
    return(0);
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
if(argc < 2){
    printf("The path to the file is\n");
    return 1;
}

The error message is unfinished (appropriately enough, missing is missing), and it should go to stderr, not stdout.

if(stat(argv[1],&fileStat) < 0){
    printf("error\n");
    return 1;
}

Again we're writing to the wrong stream. And we could make a more informative message, e.g. perror(argv[1]).

        Search_in_File(argv[1], "#!");
        Search_in_File(argv[1], "ELF");

These two calls each open the file to read it. It is probably better to open the file just once. Why do we ignore the return value each time?

while(fgets(temp, 512, fp) != NULL) {
    if((strstr(temp, str)) != NULL) {
        ⋮
    }
}

This code will fail to match when the search string straddles a 512-byte boundary. But why are we searching the whole file anyway? Those magic strings are only meaningful as the first characters, and do not tell us anything if they occur later in the file. We could do this much more simply:

if (fread(temp, 4, 1, fp) == 1
    && strncmp(temp, str, strlen(str)) == 0) {
    ⋮
}
        if(strcmp(str, "#!")==0)
            printf("shell type.");
        else if(strcmp(str, "ELF")==0)
            printf(" ELF.");

Instead of string-matching the str argument, why not just pass an additional argument specifying the string to print? Then there's only one place to modify when you add a new test.

if(fp) {
    fclose(fp);
}

At this point, we know that fp is non-null, since we already returned if it isn't.

Oh, and an executable that begins with #! could be any type of interpreted executable (e.g. Python or Sed script), not necessarily a shell script. Not sure whether that's what's intended (is it your description or the program that needs adjusting?).

\$\endgroup\$
2
  • \$\begingroup\$ thank you very much, helpful. But can a new solution, another code, another example? \$\endgroup\$
    – Mark
    Mar 22, 2021 at 19:58
  • 1
    \$\begingroup\$ Yes, you could post your new solution - just ask a new question. I'll look at it if I'm not too busy. \$\endgroup\$ Mar 22, 2021 at 21:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.