Skip to content
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.

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

lib/posix-sysinfo: Fix off-by-1 error in sethostname #1307

Merged
merged 2 commits into from Feb 7, 2024

Conversation

andreittr
Copy link
Contributor

Description of changes

This change corrects an an off-by-1 error in sethostname when checking the length of the new hostname, leading to a missing terminating NUL byte in the nodename field of struct utsname, contrary to the expected Linux ABI. The suboptimal strncpy call is also replaced with a more appropriate memcpy call, since (1) we know the length of the input string, which (2) does not need to be NUL-terminated.

Additionally, declarations of gethostname and sethostname are added to nolibc's unistd.h, making these functions available to core unikraft as well as microlibraries.

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): N/A
  • Platform(s): N/A
  • Application(s): N/A

Additional configuration

CONFIG_LIBPOSIX_SYSINFO=y

Test snippet:

int main(void)
{
	char name[4096];
	int r;

	r = gethostname(name, 4096);
	assert(!r);
	puts(name);

	r = sethostname("123456789012345678901234567890123456789012345678901234567890abcde", 65);
	if (!r) {
		r = gethostname(name, 4096);
		assert(!r);
		puts(name);
	} else {
		puts("Errored out");
	}

	r = sethostname("123456789012345678901234567890123456789012345678901234567890abcde", 64);
	assert(!r);

	r = gethostname(name, 4096);
	assert(!r);
	puts(name);

	return 0;
}

On staging it incorrectly sets a hostname without a NUL byte, resulting in the Unikraft version leaking into the hostname:

SeaBIOS (version 1.16.3-1.fc39)
Booting from ROM..Powered by
o.   .o       _ _               __ _
Oo   Oo  ___ (_) | __ __  __ _ ' _) :_
oO   oO ' _ `| | |/ /  _)' _` | |_|  _)
oOo oOO| | | | |   (| | | (_) |  _) :_
 OoOoO ._, ._:_:_,\_._,  .__,_:_, \___)
               Telesto 0.16.1~4b12ebc89
unikraft
123456789012345678901234567890123456789012345678901234567890abcde5-Telesto
123456789012345678901234567890123456789012345678901234567890abcd

After the fix, it should correctly error out of the first sethostname call:

SeaBIOS (version 1.16.3-1.fc39)
Booting from ROM..Powered by
o.   .o       _ _               __ _
Oo   Oo  ___ (_) | __ __  __ _ ' _) :_
oO   oO ' _ `| | |/ /  _)' _` | |_|  _)
oOo oOO| | | | |   (| | | (_) |  _) :_
 OoOoO ._, ._:_:_,\_._,  .__,_:_, \___)
               Telesto 0.16.1~c704be7ea
unikraft
Errored out
123456789012345678901234567890123456789012345678901234567890abcd

This change adds declarations of gethostname and sethostname to nolibc's
unistd.h when CONFIG_LIBPOSIX_SYSINFO is enabled.
Additionally, the sysinfo declarations are moved after shared includes.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
This change fixes an off-by-1 error in sethostname when checking the
length of the new hostname, leading to a missing terminating NUL byte in
the `nodename` field of `struct utsname`, contrary to the expected Linux
ABI.
Additionally, strncpy is replaced with a more appropriate memcpy call.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
@andreittr andreittr requested a review from a team as a code owner February 5, 2024 15:37
@github-actions github-actions bot added area/lib Internal Unikraft Microlibrary lang/c Issues or PRs to do with C/C++ lib/nolibc Only neccessary subset of libc functionality lib/posix-sysinfo labels Feb 5, 2024
@razvand razvand added this to the v0.16.2 (Telesto) milestone Feb 6, 2024
@razvand razvand requested review from adinamariav and mariasfiraiala and removed request for a team February 6, 2024 21:56
Copy link
Contributor

@mariasfiraiala mariasfiraiala left a comment

Choose a reason for hiding this comment

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

Looks good to me, however, please fix the NUL typo in commit's c704be7 message.

Copy link

@adinamariav adinamariav left a comment

Choose a reason for hiding this comment

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

Aside from what @mariasfiraiala stated here, looks good.

Reviewed-by: Adina-Maria Vaman adinamariav22@gmail.com

@andreittr
Copy link
Contributor Author

Looks good to me, however, please fix the NUL typo in commit's c704be7 message.

@mariasfiraiala Oh that's on purpose, I meant that as NUL, ASCII abbreviation for char code 0 (aka "the null char", but that "null" is lowercase), instead of NULL, like the null pointer.
I know it's confusing, blame the telecom people behind ascii and their aversion to 4 letter names 😄 .

Copy link
Contributor

@mariasfiraiala mariasfiraiala left a comment

Choose a reason for hiding this comment

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

Reviewed-by: Maria Sfiraiala maria.sfiraiala@gmail.com

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

@razvand razvand added the merge Label to trigger merge action label Feb 7, 2024
@unikraft-bot unikraft-bot changed the base branch from staging to staging-1307 February 7, 2024 12:23
@unikraft-bot unikraft-bot merged commit 3217ccb into unikraft:staging-1307 Feb 7, 2024
13 checks passed
unikraft-bot pushed a commit that referenced this pull request Feb 7, 2024
This change adds declarations of gethostname and sethostname to nolibc's
unistd.h when CONFIG_LIBPOSIX_SYSINFO is enabled.
Additionally, the sysinfo declarations are moved after shared includes.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
GitHub-Closes: #1307
unikraft-bot pushed a commit that referenced this pull request Feb 7, 2024
This change fixes an off-by-1 error in sethostname when checking the
length of the new hostname, leading to a missing terminating NUL byte in
the `nodename` field of `struct utsname`, contrary to the expected Linux
ABI.
Additionally, strncpy is replaced with a more appropriate memcpy call.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
GitHub-Closes: #1307
@unikraft-bot unikraft-bot added ci/merged Merged by CI and removed merge Label to trigger merge action labels Feb 7, 2024
@andreittr andreittr deleted the ttr/sethostname branch February 7, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lib Internal Unikraft Microlibrary ci/merged Merged by CI lang/c Issues or PRs to do with C/C++ lib/nolibc Only neccessary subset of libc functionality lib/posix-sysinfo
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants