Fork us on GitHub Follow us on Facebook Follow us on Twitter

Opened 7 years ago

Closed 5 years ago

#506 closed defect (fixed)

getopt() crashes when called multiple time with invalid options

Reported by: Saad Talaat Owned by: Saad Talaat
Priority: minor Milestone: 0.7.0
Component: helenos/lib/c Version: mainline
Keywords: first-patch Cc:
Blocker for: Depends on:
See also: #618

Description (last modified by Vojtech Horky)

I've been playing around with several commands ls and cat in particular.Anyway if you ls -la for example it will output unknown options, which is fine. then try to ls -h and ls —help. after that just make a normal ls it will output unknown options and kill the bdsh session. I tried to perform the same sequence of commands again to trigger this error but it would occasionally be triggered.

Steps to reproduce:

  1. ls -la (correctly reports bad options)
  2. ls -h
  3. ls --help
  4. ls (crashes the whole bdsh)

Attachments (4)

45.png (32.1 KB) - added by Saad Talaat 7 years ago.
53.png (37.6 KB) - added by Saad Talaat 7 years ago.
getopt.patch (264 bytes) - added by Saad Talaat 7 years ago.
Fix patch
506.patch (4.1 KB) - added by Luka Strižić 5 years ago.
opreset = 1 fix

Download all attachments as: .zip

Change History (13)

Changed 7 years ago by Saad Talaat

Attachment: 45.png added

Changed 7 years ago by Saad Talaat

Attachment: 53.png added

comment:1 Changed 7 years ago by Saad Talaat

the getopt.c doesn't recognize —help as long option after -h has occured however, It only recognizes the first h, and on latter execution of ls it considers the rest of the chars (elp) as an option, since place variable is not reset in getopt_internal, However with resetting place it still recognize e as an option.

comment:2 Changed 7 years ago by Saad Talaat

Issue solved, the problem was with reseting the place pointer as previously mentioned. Problem doesn't only apply to ls of course it applies to all long options. However, fix was only to reset place at the end of getopt_long otherwise when —help is made again the getopt_internal won't recognize the long option in argv, Instead it will re-read the place string.
setting place to EMSG before getopt_long returns fixes the issue.

Patch attached.

Changed 7 years ago by Saad Talaat

Attachment: getopt.patch added

Fix patch

comment:3 Changed 7 years ago by Vojtech Horky

Component: helenos/app/bdshhelenos/lib/c

You are probably close to the root of the problem but the patch does not fix it. With this patch, running ls -xy causes an endless loop, printing unknown option -- x (tried on current mainline).

Changing component to libc because the problem is probably inside getopt. With normal applications this problem is not reproducible because they are always calling getopt once, thus with correct initialization.

A side-note: the code in getopt.c is a mess. It uses K&R style, lot of macros instead of inline functions or has hacks for applications such as rsyncd. We should probably think about replacing this interface completely.

comment:4 Changed 7 years ago by Vojtech Horky

Description: modified (diff)
Summary: bdsh misbehaving on sequential erroneous/invalid options for a cmd/modulegetopt() crashes when called multiple time with invalid options

comment:5 Changed 6 years ago by Vojtech Horky

Keywords: first-patch added

comment:6 Changed 5 years ago by Michal Koutny

See also: #618

comment:7 Changed 5 years ago by Luka Strižić

getopt() is used like this in the code:
1) initialize optind to 0
2) run c = getopt_long() in a loop until c == -1
3) for every c set the relevant flags or execute a function, depending on the value of c

getopt() use in BSD is a bit different from POSIX.
From the BSD getopt docs:
"In order to use getopt() to evaluate multiple sets of arguments, or to evaluate a single set of arguments multiple times, the variable optreset must be set to 1 before the second and each additional set of calls to getopt(), and the variable optind must be reinitialized."
And later:
"The optreset variable was added to make itpossible to call the getopt() function multiple times. This is an extension to the IEEE Std 1003.2 ('POSIX.2') specification."

When passing "-h" or "—help", most commands don't loop until c == -1 in step 2, but simply stop the command execution in step 3, before the -1 getopt() call, so getopt() doesn't reset it's state completely, and optreset isn't initialized in step 1.

Possible solutions include:
a) Use getopt() like the BSD docs suggest, and set optreset=1 in step 1, along with optind=0.
b) Call getopt() until c==-1 in every case, without the "early" returns when handling c.
c) Make optind==0 toggle an optreset=1 in getopt.c
d) Suggestions welcome.

a) and b) require changes in most bdsh commands, but a) is "the right" thing to do, I think. c) requires change in one file, but is a "hack".

Solution to this should also fix #618.

Changed 5 years ago by Luka Strižić

Attachment: 506.patch added

opreset = 1 fix

comment:8 Changed 5 years ago by Luka Strižić

The a) fix is attached.

comment:9 Changed 5 years ago by Jakub Jermář

Milestone: 0.7.0
Resolution: fixed
Status: newclosed

Fixed in mainline,2324.

Note: See TracTickets for help on using tickets.