Opened 12 years ago

Closed 9 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 12 years ago.
53.png (37.6 KB ) - added by Saad Talaat 12 years ago.
getopt.patch (264 bytes ) - added by Saad Talaat 12 years ago.
Fix patch
506.patch (4.1 KB ) - added by Luka Strižić 9 years ago.
opreset = 1 fix

Download all attachments as: .zip

Change History (13)

by Saad Talaat, 12 years ago

Attachment: 45.png added

by Saad Talaat, 12 years ago

Attachment: 53.png added

comment:1 by Saad Talaat, 12 years ago

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 by Saad Talaat, 12 years ago

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.

by Saad Talaat, 12 years ago

Attachment: getopt.patch added

Fix patch

comment:3 by Vojtech Horky, 12 years ago

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 by Vojtech Horky, 12 years ago

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 by Vojtech Horky, 10 years ago

Keywords: first-patch added

comment:6 by Michal Koutny, 9 years ago

See also: #618

comment:7 by Luka Strižić, 9 years ago

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.

by Luka Strižić, 9 years ago

Attachment: 506.patch added

opreset = 1 fix

comment:8 by Luka Strižić, 9 years ago

The a) fix is attached.

comment:9 by Jakub Jermář, 9 years ago

Milestone: 0.7.0
Resolution: fixed
Status: newclosed

Fixed in mainline,2324.

Note: See TracTickets for help on using tickets.