Skip to content

Ensure VERSION is NUL-terminated #213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
chriskuehl merged 1 commit into from
Dec 2, 2020
Merged

Ensure VERSION is NUL-terminated #213

chriskuehl merged 1 commit into from
Dec 2, 2020
+3 −3

Conversation

@suve
Copy link
Contributor

@suve suve commented Nov 30, 2020

The VERSION variable is referenced in two places inside dumb-init.c: in the print_help() function, and in parse_command(), when the --version option is passed:

void print_help(char *argv[]) {
    fprintf(stderr,
        "dumb-init v%s"
        "Usage: %s [option] command [[arg] ...]\n"
// (snip)
        "Full help is available online at https://github.com/Yelp/dumb-init\n",
        VERSION,
        argv[0]
    );
}

// inside parse_command()
    case 'V':
        fprintf(stderr, "dumb-init v%s", VERSION);
        exit(0);

In both of these cases, VERSION is used as an argument to fprintf(), to be formatted with %s. The man page for fprintf(3) says this about the %s conversion specifier:

The const char * argument is expected to be a pointer to an array of character type (pointer to a string). Characters from the array are written up to (but not including) a terminating null byte ('\0');

However, the way VERSION is currently generated, it does NOT contain the terminating null byte.

unsigned char VERSION[] = {
  0x31, 0x2e, 0x32, 0x2e, 0x32, 0x0a
};

This patch changes the way VERSION.h is generated, to ensure that VERSION is a NUL-terminated string.

@asottile
Copy link
Contributor

asottile commented Nov 30, 2020 •
edited
Loading

a less intrusive patch would be to use VERSION_len:

        fprintf(stderr, "dumb-init v%.*s", VERSION_len, VERSION);

@suve
Copy link
Contributor Author

suve commented Dec 1, 2020 •
edited
Loading

It's an interesting idea, though it has one drawback - if someone needs to add a new piece of code that uses VERSION, they'll have to remember that it's not NUL-terminated and requires special handling.

As a third option, how about keeping VERSION as unsigned char[], with an additional 0-byte?

@asottile
Copy link
Contributor

asottile commented Dec 1, 2020 •
edited
Loading

I would prefer the one line patch as I indicated, this tool has had very little maintenance and I doubt that a new use for VERSION will come up

@suve
Use "%*s" when printing VERSION
7bb2fef 
The "%s" conversion specifier expects a NUL-terminated string.
However, the VERSION variable does not contain a NUL-terminator,
so formatting it using "%s" may lead to printing whatever happens
to be in memory next to VERSION.

Using "%*s" allows to specify how many characters to print,
thus making sure we don't go off the array.
@suve suve force-pushed the ensure-VERSION-is-NUL-terminated branch from 040188d to 7bb2fef Compare December 1, 2020 19:05
@suve
Copy link
Contributor Author

suve commented Dec 1, 2020

Sure. Rewrote the patch and force-pushed the branch.

asottile
asottile approved these changes Dec 1, 2020
View reviewed changes
Copy link
Contributor

@asottile asottile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chriskuehl chriskuehl merged commit 6a0fcbc into Yelp:master Dec 2, 2020
@chriskuehl
Copy link
Contributor

chriskuehl commented Dec 2, 2020

Thanks for the fix!

I think I won't release a new version just for this fix since as far as I understand this isn't causing issues in practice (due to automatic null byte padding around the constant?) but let me know if I'm misunderstanding the impact.

@suve
Copy link
Contributor Author

suve commented Dec 2, 2020

Well, the only reason I spotted and patched this is because it did, in fact, cause issues in practice. ;)

In the Fedora Linux distribution, we had issues with building the dumb-init package. It turned out that on the s390x architecture, while the program built fine, the test suite failed - on this particular arch, the VERSION variable was not immediately followed by a null byte, which in turn caused the "is help message ok?" test to fail. (rhbz1863456)

@chriskuehl
Copy link
Contributor

chriskuehl commented Dec 3, 2020

Thanks for letting me know, I've released a v1.2.3 which contains this fix.

@suve suve mentioned this pull request Dec 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@asottile asottile asottile approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@suve @asottile @chriskuehl