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-environ: Ensure that compiled-in strings are writable #1054

Closed

Conversation

mogasergiu
Copy link
Member

@mogasergiu mogasergiu commented Aug 15, 2023

Previously, the elements of __init_env would be compile-in strings
that would correspond to CONFIG_LIBPOSIX_ENVIRON_ENVPx configurable
strings. This lead to these strings being placed by in the .rodata
section and making them unmodifiable.

Unfortunately, Redis represents an application whose code may want to
modify such data (see
commit 6356cf6808be ("Set process name in ps output to make operations safer.").
Therefore, to appease such actions, make sure that this data is placed
in a modifiable section such as .data by using statically declared
global variables containing these very strings as an indirection.

To make it look somewhat nicer, do so with the help of two basic helper
macros: DECLARE_LIBPOSIX_ENVIRON_ENV_VAR to transparently declare such
variables and LIBPOSIX_ENVIRON_ENV_VAR to transparently access them.

Signed-off-by: Sergiu Moga sergiu@unikraft.io

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): [All]
  • Platform(s): [All]
  • Application(s): [All]

Additional configuration

Description of changes

Copy link
Member

@michpappas michpappas 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. One minor point is the leading underscore in the variable name. Unless there is a strong reason to be there it should be changed to something in compliance with our guidelines.

@mogasergiu
Copy link
Member Author

Looks good to me. One minor point is the leading underscore in the variable name. Unless there is a strong reason to be there it should be changed to something in compliance with our guidelines.

I noticed that @skuenzer likes using those 😝.

@unikraft-bot unikraft-bot added area/lib Internal Unikraft Microlibrary area/plat Unikraft Patform lang/c Issues or PRs to do with C/C++ plat/common Common to all platforms labels Aug 15, 2023
@skuenzer
Copy link
Member

Looks good to me. One minor point is the leading underscore in the variable name. Unless there is a strong reason to be there it should be changed to something in compliance with our guidelines.

I noticed that @skuenzer likes using those 😝.

I have to admit that this is true. I liked them for things to mark them as "do not touch", especially when they are part of a system declared in the file/library. But since we have our convention, we can change it. In this sense @michpappas is right that we should be a good example for other contributors.

@razvand razvand added this to the v0.14.0 (Prometheus) milestone Aug 15, 2023
@mogasergiu mogasergiu force-pushed the smoga/fix-posix-environ-rodata branch from d0b74b9 to 1a354e1 Compare August 15, 2023 12:24
@mogasergiu mogasergiu force-pushed the smoga/fix-posix-environ-rodata branch from 1a354e1 to a7aa288 Compare August 15, 2023 12:25
Copy link
Member

@michpappas michpappas 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: Michalis Pappas michalis@unikraft.io

Previously, the elements of `__init_env` would be compile-in strings
that would correspond to `CONFIG_LIBPOSIX_ENVIRON_ENVPx` configurable
strings. This lead to these strings being placed by in the `.rodata`
section and making them unmodifiable.

Unfortunately, Redis represents an application whose code may want to
modify such data (see
commit 6356cf6808be ("Set process name in ps output to make operations safer.")).
Therefore, to appease such actions, make sure that this data is placed
in a modifiable section such as `.data` by using statically declared
global variables containing these very strings as an indirection.

To make it look somewhat nicer, do so with the help of two basic helper
macros: DECLARE_LIBPOSIX_ENVIRON_ENV_VAR to transparently declare such
variables and LIBPOSIX_ENVIRON_ENV_VAR to transparently access them.

Signed-off-by: Sergiu Moga <sergiu@unikraft.io>
@mogasergiu mogasergiu force-pushed the smoga/fix-posix-environ-rodata branch from a7aa288 to 97918b2 Compare August 15, 2023 14:20
@unikraft-bot
Copy link
Member

⚠️ Checkpatch passed with warnings

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request but it encountered warnings:

SHA commit checkpatch
97918b2 lib/posix-environ: Ensure that compiled-in strings are writable ⚠️

Truncated logs starting from first warning 97918b2:

WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#14: 
commit 6356cf6808be ("Set process name in ps output to make operations safer.")).

total: 0 errors, 1 warnings, 98 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/tmp/build/a53d4c5b/patches/0001-lib-posix-environ-Ensure-that-compiled-in-strings-ar.patch has style problems, please review.

NOTE: Ignored message types: ASSIGN_IN_IF FILE_PATH_CHANGES NEW_TYPEDEFS OBSOLETE

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

View complete logs | Learn more about Unikraft's coding style and contribution guidelines.

Copy link
Member

@skuenzer skuenzer left a comment

Choose a reason for hiding this comment

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

Thanks a lot for these changes!

Approved-by: Simon Kuenzer simon@unikraft.io

@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lib Internal Unikraft Microlibrary area/plat Unikraft Patform ci/merged Merged by CI lang/c Issues or PRs to do with C/C++ plat/common Common to all platforms
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

5 participants