Skip to content

Fix NULL reference in option parser#149

Merged
legionus merged 1 commit into
legionus:masterfrom
000lbh:fix-null
Nov 24, 2025
Merged

Fix NULL reference in option parser#149
legionus merged 1 commit into
legionus:masterfrom
000lbh:fix-null

Conversation

@000lbh

@000lbh 000lbh commented Nov 18, 2025

Copy link
Copy Markdown
Contributor

If an option that needs an argument is given, but the argument is actually missing, the optind will increase by 1 and return, and if the option is the last option, a NULL reference occurs at setfont.c:290. This bug is fixed by returning '!' instead of '?' to main and process this condition separately.

If an option that needs an argument is given, but the argument is
actually missing, the optind will increase by 1 and return, and if
the option is the last option, a NULL reference occurs at
setfont.c:290. This bug is fixed by returning '!' instead of '?' to
main and process this condition separately.

Signed-off-by: Bohai Li <lbhlbhlbh2002@icloud.com>
@legionus

Copy link
Copy Markdown
Owner

@000lbh Yeah, you're right. But why not make it simpler and reduce optind if an argument is required?

--- a/src/setfont.c
+++ b/src/setfont.c
@@ -159,6 +159,7 @@ kbd_getopt(int argc, char **argv, const struct kbd_option *opts)
 
 required_argument:
        kbd_warning(0, "option '%s' requires an argument", name);
+       optind--;
        return '?';
 }
 

This will eliminate the need to create a new magic return code.

@000lbh

000lbh commented Nov 23, 2025

Copy link
Copy Markdown
Contributor Author

Because this simple method will print two lines of warning: option %s requires an argument and invalid option %s. I think this is confusing, because here option %s is valid but with wrong number of arguments. May I add usage(EX_USAGE, opthelp) function call in function kbd_getopt after required_argument label instead to solve this problem, and this method will not create the new magic return code

Edited:
opthelp is local variable in main, so cannot be accessed in kbd_getopt. Maybe we need to make this variable global and static(to avoid symbol conflicts).

@legionus

Copy link
Copy Markdown
Owner

Because this simple method will print two lines of warning: option %s requires an argument and invalid option %s. I think this is confusing, because here option %s is valid but with wrong number of arguments.

I completely forgot about the second message. Yes, if it can be avoided, then it should be avoided. Your changes totally make sense. I'll apply it tomorrow.

@legionus legionus merged commit c547363 into legionus:master Nov 24, 2025
@000lbh 000lbh deleted the fix-null branch November 25, 2025 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants