Bug in getopt (C) for non-global use?

user_387182 0
Viewed 146

I'm writing a code that parses a number of commands from a text file, and I found what I think it is a bug in getopt. Just wondering if anyone has seen this before?

Basically I have a loop where in each iteration I read a string, I transform it with 'strtok' to something that can be fed to getopt and check for the options provided. It works in some cases:

$ ./test_short
cmd -ia
  -- option i
  -- option a


cmd -a
  -- option a


cmd -i
  -- option i

But it fails with, for example, this:

$ ./test_short
cmd -ia
  -- option i
  -- option a


cmd -i -a
cmd: invalid option -- '-'
  -- option unknown
  -- option a
  -- option a

The code can be cloned from https://github.com/angel-devicente/test-getopt, but for completeness I put the short option below as well.

Any idea if this is a real bug or there is some other global variable that I'm forgetting to reset?

Thanks,

#include <stdio.h>     /* for printf */
#include <stdlib.h>    /* for exit */
#include <string.h>
#include <unistd.h>

#define MAX_ARGS 10
#define MAX_C 100


int main(int argc, char **argv) {
  int c;
  char       cmd[MAX_C+1];

  int argc_i;
  char *argv_i[MAX_ARGS];
  
  
  while (1) {
    // Read string, for example: cmd -i -a
    fgets(cmd,MAX_C,stdin);
    cmd[strlen(cmd) - 1] = 0;
    if (strlen(cmd) <= 1) break;

    // Transform into argc_i, argv_i, to feed to getopt
    argc_i = 0;
    char *p2 = strtok(cmd, " ");
    while (p2 && argc_i < MAX_ARGS)
      {
    argv_i[(argc_i)++] = p2;
    p2 = strtok(NULL, " ");
      }
    argv_i[argc_i] = 0;


    // Reset getopt
    optind  = 1;
    int option_index = 0;

    
    while (1) {
      c = getopt(argc_i, argv_i, "ia");

      if (c == -1)
    break;

      switch (c) {
      case 'a':
    printf("  -- option a\n");
    break;

      case 'i':
    printf("  -- option i\n");
    break;

      case '?':
    printf("  -- option unknown\n");
    break;

      default:
    printf("?? getopt returned character code 0%o ??\n", c);
      }
    }

    printf("\n\n");

    if (optind < argc) {
      printf("non-option ARGV-elements: ");
      while (optind < argc)
    printf("%s ", argv[optind++]);
      printf("\n");
    }
  }

  exit(EXIT_SUCCESS);
}
getopt linux c
1 Answers

This looks like an intentional deviation from the POSIX specification of getopt.

To start off, I changed this line in your code:

argv_i[(argc_i)++] = p2;

To this:

argv_i[(argc_i)++] = strdup(p2);

And added this to the end of the while (1) loop:

for (i=0;i<argc_i;i++) {
    free(argv_i[i]);
}

And added this to the top:

memset(argv_i,0,sizeof(argv_i));

Then I ran through valgrind:

==54748== Memcheck, a memory error detector
==54748== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==54748== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==54748== Command: ./x1
==54748== 
optind=1
cmd -ia
arg 0: cmd
arg 1: -ia
  -- option i
  -- option a


cmd -i -a
==54748== Invalid read of size 1
==54748==    at 0x4F192A1: _getopt_internal_r (in /usr/lib64/libc-2.17.so)
==54748==    by 0x4F1A56A: _getopt_internal (in /usr/lib64/libc-2.17.so)
==54748==    by 0x4F1A5B7: getopt (in /usr/lib64/libc-2.17.so)
==54748==    by 0x400A2E: main (x1.c:48)
==54748==  Address 0x5205093 is 3 bytes inside a block of size 4 free'd
==54748==    at 0x4C2AF9D: free (vg_replace_malloc.c:540)
==54748==    by 0x400B25: main (x1.c:89)
==54748==  Block was alloc'd at
==54748==    at 0x4C29EA3: malloc (vg_replace_malloc.c:309)
==54748==    by 0x4EC3AA9: strdup (in /usr/lib64/libc-2.17.so)
==54748==    by 0x400991: main (x1.c:31)
==54748== 
  -- option i
  -- option a

Since there's no reference to the malloc'ed memory in the test program, that means the getopt implementation is keeping around a pointer to it.

Now let's take a look at a few lines from https://github.com/lattera/glibc/blob/master/posix/getopt.c including _getopt_internal_r:

/* Index in ARGV of the next element to be scanned.
   This is used for communication to and from the caller
   and for communication between successive calls to 'getopt'.
   On entry to 'getopt', zero means this is the first call; initialize.
   When 'getopt' returns -1, this is the index of the first of the
   non-option elements that the caller should itself scan.
   Otherwise, 'optind' communicates from one call to the next
   how much of ARGV has been scanned so far.  */

/* 1003.2 says this must be 1 before any call.  */
int optind = 1;
...
static struct _getopt_data getopt_data;
...
int
_getopt_internal_r (int argc, char **argv, const char *optstring,
            const struct option *longopts, int *longind,
            int long_only, struct _getopt_data *d, int posixly_correct)
{
  int print_errors = d->opterr;

  if (argc < 1)
    return -1;

  d->optarg = NULL;

  if (d->optind == 0 || !d->__initialized)
    optstring = _getopt_initialize (argc, argv, optstring, d, posixly_correct);

...

int
_getopt_internal (int argc, char **argv, const char *optstring,
          const struct option *longopts, int *longind, int long_only,
          int posixly_correct)
{
  int result;

  getopt_data.optind = optind;
  getopt_data.opterr = opterr;

  result = _getopt_internal_r (argc, argv, optstring, longopts,
                   longind, long_only, &getopt_data,
                   posixly_correct);

  optind = getopt_data.optind;
  optarg = getopt_data.optarg;
  optopt = getopt_data.optopt;

  return result;
}

#define GETOPT_ENTRY(NAME, POSIXLY_CORRECT)         \
  int                               \
  NAME (int argc, char *const *argv, const char *optstring) \
  {                             \
    return _getopt_internal (argc, (char **)argv, optstring,    \
                 0, 0, 0, POSIXLY_CORRECT);     \
  }

#ifdef _LIBC
GETOPT_ENTRY(getopt, 0)
GETOPT_ENTRY(__posix_getopt, 1)
#else
GETOPT_ENTRY(getopt, 1)
#endif

Here we can see that optind is initialized to 1 and an internal static struct which is uninitialized. There is also a comment that says "On entry to 'getopt', zero means this is the first call; initialize." which indicated a different initialization condition.

getopt calls getopt_internal, which subsequently assigns optind to a corresponding member of the static struct, then passes it to _getopt_internal_r . The _getopt_internal_r function then checks if either the optind member of the struct is 0 or if the __initialized member is 0, and if so an initialization function is called, defined as follows:

static const char *
_getopt_initialize (int argc _GL_UNUSED,
            char **argv _GL_UNUSED, const char *optstring,
            struct _getopt_data *d, int posixly_correct)
{
  /* Start processing options with ARGV-element 1 (since ARGV-element 0
     is the program name); the sequence of previously skipped
     non-option ARGV-elements is empty.  */
  if (d->optind == 0)
    d->optind = 1;

  d->__first_nonopt = d->__last_nonopt = d->optind;
  d->__nextchar = NULL;

  /* Determine how to handle the ordering of options and nonoptions.  */
  if (optstring[0] == '-')
    {
      d->__ordering = RETURN_IN_ORDER;
      ++optstring;
    }
  else if (optstring[0] == '+')
    {
      d->__ordering = REQUIRE_ORDER;
      ++optstring;
    }
  else if (posixly_correct || !!getenv ("POSIXLY_CORRECT"))
    d->__ordering = REQUIRE_ORDER;
  else
    d->__ordering = PERMUTE;

  d->__initialized = 1;
  return optstring;
}

We can see here that the optind member will be set to 1 if it's currently 0. Also, notice in particular this line:

d->__nextchar = NULL;

If we then look at https://github.com/lattera/glibc/blob/master/posix/getopt_int.h where this field is defined, we see the following:

  /* The next char to be scanned in the option-element
     in which the last option character we returned was found.
     This allows us to pick up the scan where we left off.
     If this is zero, or a null string, it means resume the scan
     by advancing to the next ARGV-element.  */
  char *__nextchar;

So this variable keeps track of where the processing left off. But going back to these lines in _getopt_internal_r

  if (d->optind == 0 || !d->__initialized)
    optstring = _getopt_initialize (argc, argv, optstring, d, posixly_correct);

The reset will only happen if optind is 0.

So in this implementation, setting optind to 0 will reset processing. This however is not portable, as POSIX specifies that doing so is unspecified behavior.

This means that either this code should change to take optind == 1 as an initialization condition, or the documentation should be updated to reflect this deviation from the POSIX standard and state that it is a deviation.

user_16871190
Related