Simple Shell Program - Printable Version

Simple Shell Program - Printable Version

+- Sinisterly (https://sinister.li)
+-- Forum: Coding (https://sinister.li/Forum-Coding)
+--- Forum: C, C++, & Obj-C (https://sinister.li/Forum-C-C-Obj-C)
+--- Thread: Simple Shell Program (/Thread-Simple-Shell-Program)

Pages: 1 2

RE: Simple Shell Program - bitm0de - 11-30-2016

You don't need '&' before the function name in your function pointer array because they will decay to the appropriate pointer types.

But why do you free args twice here?

Code:
do
{

    printf("%c%s%c%c ", '$', login, '_', '>');
    args = getArgs(&args_size);
    status = execute(args);

    free_a(args, args_size);
    free(args);

}
while (status);

free_a(args, args_size);
free(args);
free(login);

There's not much reason to be using the heap here either way for those allocations that stay within main. sizeof(char) is guaranteed to be 1 too, and why not use a macro if you're going to use sizeof() operator to check the size of things? More efficient than using a function that you haven't even marked as inline (sizeof is a compile-time operator that you place within a runtime function, and you can't guarantee that it'll be inlined unless compiler optimization is enabled and deals with that for you):

Code:
int num_builtins()
{
    return sizeof(builtin_str) / sizeof(char *);
}

sizeof() returns size_t though; you could have done:

Code:
size_t num_builtins()
{
    return sizeof(builtin_str) / sizeof(*builtin_str);
}

Or a generic macro:

Code:
#define SIZEOFA(arr) (sizeof(arr) / sizeof(*arr))

Lots of other performance improvements can be made.

RE: Simple Shell Program - insidious - 11-30-2016

(11-30-2016, 08:02 AM)bitm0de Wrote: You don't need '&' before the function name in your function pointer array because they will decay to the appropriate pointer types.

But why do you free args twice here?

Code:
do { printf("%c%s%c%c ", '$', login, '_', '>'); args = getArgs(&args_size); status = execute(args); free_a(args, args_size); free(args); } while (status); free_a(args, args_size); free(args); free(login);

There's not much reason to be using the heap here either way for those allocations that stay within main. sizeof(char) is guaranteed to be 1 too, and why not use a macro if you're going to use sizeof() operator to check the size of things? More efficient than using a function that you haven't even marked as inline (sizeof is a compile-time operator that you place within a runtime function, and you can't guarantee that it'll be inlined unless compiler optimization is enabled and deals with that for you):

Code:
int num_builtins() { return sizeof(builtin_str) / sizeof(char *); }

sizeof() returns size_t though; you could have done:

Code:
size_t num_builtins() { return sizeof(builtin_str) / sizeof(*builtin_str); }

Or a generic macro:

Code:
#define SIZEOFA(arr) (sizeof(arr) / sizeof(*arr))

Lots of other performance improvements can be made.

Hey thanks for the feedback. It's deadweek/finals right now and I'm working on a more intensive final project so I don't have time to correct much of the things you have pointed out here, but I will surely get to it after the next week+ahalf is over.

To answer some of your questions:

Quote:But why do you free args twice here?

I don't remember exactly why I freed args twice here, I would have to revisit this code. iirc I was debugging with valgrind and was getting memory leaks, and that fixed it. To my knowledge running this program, it does not trigger any invalid frees.

I use the Heap here to avoid some possible buffer overflows in the shell. Using the heap allows me to do a variety of things, namely, dynamic allocation of arguments. So, if a user of this shell enters some ridiculously long string characters the program does not fail.
The implementation of this dynamic reallocation is in readWord/getArgs iirc

I will take a longer, harder look at this in the near future.
Thanks,

RE: Simple Shell Program - bitm0de - 11-30-2016

I know the heap is more versatile and larger than the stack, some places in your code it makes sense but for example:

Code:
int main(){
  ...
  char *login;

  //max login length is 32 bytes, 33 for good luck
  login = malloc(33 *sizeof(char));
  memset(login, 0, 33);
  //get the login 
  retLogin(&login);

  do{
    
    printf("%c%s%c%c ", '$', login, '_', '>');
    ...
  }while(status);

  ...
  free(login);

  return EXIT_SUCCESS;
}

With login it doesn't really make sense. You allocate 33 bytes, zero-initialize that, then pass it to a function and printf() the result in the loop.

Your retLogin function (tmp) is a pointer to your pointer:

Code:
//getlogin is deprecated on ArchLinux (& newer distro's in general)
//works fine on the servers, for the purposes of this proj
int retLogin(char **tmp){
  char *getlog = *tmp; 
  getlog = getlogin();  
  *tmp = getlog;

  if(!getlog){
    die("getlogin() error");
    free(getlog);
    return -1;
  }else
    return 0;
}

Code:
char *getlog = *tmp; 
getlog = getlogin();

The first assignment of getlog to the (char *) dereferenced from tmp is overwritten in the next line where it's assigned the return of getlogin(). Then the (char *) dereferenced from tmp which is essentially the pointer which login points to is assigned to the pointer returned by getlogin() again.

Code:
*tmp = getlog;

HOWEVER - getlogin() returns a pointer to a STATIC buffer, so now that your login pointer doesn't any longer point to the memory you've allocated with malloc(), you attempt to free this static buffer which isn't even on the heap, and you leak memory from the call to malloc().

Read the docs for getlogin(): https://linux.die.net/man/3/getlogin

Quote:The string is statically allocated and might be overwritten on subsequent calls to this function or to cuserid().

Essentially, this is what the function does internally:

Code:
function getlogin()
{
  static char buf[BUF_SIZE];
  ... get login ...
  return buf;
}

^ This is more likely where the memory leak comes from. The problem is that you allocate memory on the heap, pass a pointer to the pointer (that holds the address of the first byte of that allocated memory on the heap) to a function that modifies the address to some memory on the stack, then you try to free it as a subsequent call to the malloc() which not only doesn't free the memory on the heap but introduces undefined behavior!

Summary:
- You allocate memory on the heap for your login pointer
- The function you pass a pointer to this pointer to does some redundant pointer assignments before assigning your pointer to a location on the stack
- Return to caller
- You attempt to free the memory from the stack (not what's on the heap from the initial call to malloc())
- ** Memory leak ** + ** Undefined behavior **