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

shadow: update to 4.16.0. #48813

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

dataCobra
Copy link
Contributor

@dataCobra dataCobra commented Feb 18, 2024

Testing the changes

  • I tested the changes in this PR: NOT YET

Local build testing

  • I built this PR locally for my native architecture, (x86_64-glibc)
  • I built this PR locally for these architectures (if supported. mark crossbuilds):
    • x86_64-musl
    • i686

I welcome everyone to test this version. Maybe also on a new installation.

@dataCobra
Copy link
Contributor Author

The file /etc/default/useradd is no longer created by default. Instead now the patched useradd binary is aware of the defaults that we provided before with the old version in /etc/default/useradd.

@dkwo
Copy link
Contributor

dkwo commented Feb 19, 2024

Some distros are also carrying this patch https://git.alpinelinux.org/aports/tree/community/shadow/fix-undefined-reference.patch Maybe it's useful?

@dkwo
Copy link
Contributor

dkwo commented Feb 19, 2024

As a reference, /usr/bin/lastlog and its manpage are now gone, and there are new /usr/bin/getsubids and its manpage

@dkwo
Copy link
Contributor

dkwo commented Feb 19, 2024

also the file login.defs seems outdated.
distros like arch and chimera patch it instead of replacing it.

@dataCobra
Copy link
Contributor Author

dataCobra commented Feb 20, 2024

Some distros are also carrying this patch https://git.alpinelinux.org/aports/tree/community/shadow/fix-undefined-reference.patch Maybe it's useful?

I've added the patch and everything still builds fine.

As a reference, /usr/bin/lastlog and its manpage are now gone, and there are new /usr/bin/getsubids and its manpage

Forgot to add the new configuration argument to add lastlog. Fixed with the new push.

also the file login.defs seems outdated. distros like arch and chimera patch it instead of replacing it.

I've checked what Arch did and added and modified patch 2 and 3 so they fit our needs. The file will now be installed from shadow itself and patched beforehand. I removed the fixed file we provided.

@dkwo
Copy link
Contributor

dkwo commented Feb 20, 2024

about the patches from arch linux: they may need more adaptating to our needs. for example, yescrypt needs a build option if i remember right.

@dataCobra
Copy link
Contributor Author

I've added yescrypt to be build for shadow to make sure we got a secure password hash.

@dataCobra
Copy link
Contributor Author

CC: @Gottox

Could you as a maintainer please check this as well? 🙂

@dkwo
Copy link
Contributor

dkwo commented Feb 20, 2024

what is the reason for removing xstrdup.patch instead of updating it?

@dataCobra
Copy link
Contributor Author

what is the reason for removing xstrdup.patch instead of updating it?

What do you want to update?

The function does no longer exist and the file is also removed.

In lib/alloc.c a comment says:

/* Replacements for malloc and strdup with error checking.  Too trivial
   to be worth copyrighting :-).  I did that because a lot of code used
   malloc and strdup without checking for NULL pointer, and I like some
   message better than a core dump...  --marekm

   Yeh, but.  Remember that bailing out might leave the system in some
   bizarre state.  You really want to put in error checking, then add
   some back-out failure recovery code. -- jfh */

As I understand the patch is no longer needed.

@dkwo
Copy link
Contributor

dkwo commented Feb 22, 2024

@dataCobra
Copy link
Contributor Author

@dkwo
Copy link
Contributor

dkwo commented Feb 24, 2024

  • does it make sense to disable RUSEROK for all libc through a patch, instead of selectively in pre_configure?
  • same for groups(1): instead of in do_build, can this be done in a patch?
  • is the use of a license file still needed?
  • i think we should have only one patch for login.defs, which can be void-specific

@dataCobra
Copy link
Contributor Author

dataCobra commented Feb 25, 2024

  • does it make sense to disable RUSEROK for all libc through a patch, instead of selectively in pre_configure?

I've added the patch you recommended from chimera linux.

  • same for groups(1): instead of in do_build, can this be done in a patch?

We could do that in a patch, but I feel like the way in the template is more convenient and easier to update.

  • is the use of a license file still needed?

I'll check that.

  • i think we should have only one patch for login.defs, which can be void-specific

Agree. For the moment I've only modified the patches a bit. But refactoring and cleaning up the files in the patches folder after we've finished the decision which patches to include must be done. Otherwise it might be hard to update the package in the future.

@dkwo
Copy link
Contributor

dkwo commented Feb 25, 2024

We could do that in a patch, but I feel like the way in the template is more convenient and easier to update.

alternatively, could it be moved to either post_configure or pre_build?

@dkwo
Copy link
Contributor

dkwo commented Mar 6, 2024

btw, 4.14.6 is out, soon maybe 4.15

@alejandro-colomar
Copy link

Some distros are also carrying this patch https://git.alpinelinux.org/aports/tree/community/shadow/fix-undefined-reference.patch Maybe it's useful?

That patch shouldn't be necesary. Please don't apply it unless you find a reason to. And if you do (find a build error that requires the patch), please report it as an upstream bug.

Would you mind asking the Alpine maintainer if they can comment on that patch? I'm interested in fixing upstream if there's something broken, but I'd like to learn what's broken, because that patch looks like a red herring.

Comment on lines 4 to 8
inline char *
xstrdup(const char *str)
{
+ if(str == NULL) return NULL;
return strcpy (xmalloc (strlen (str) + 1), str);
+ if (str == NULL) return NULL;
return strcpy(XMALLOC(strlen(str) + 1, char), str);

Choose a reason for hiding this comment

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

These xfoo() functions are designed to exit/abort on errors reported by the corresponding foo() function.

Thus, xstrdup() is strdup(3) plus a check of an output NULL from strdup(3) to error out. The precise reason to have these functions is so that they can never return NULL.

Adding null checks to a NULL input is tangential to the purpose of these functions, and defeats their purpose. Now the function can return NULL, which needs to be checked by the caller.

So, if you really want to check for programmer errors, you'd need to exit on a NULL input, not just pass it through.

But you probably shouldn't do that at all. strdup(3) doesn't handle NULL input, and I don't see a reason why xstrdup() should.

Copy link
Contributor

Choose a reason for hiding this comment

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

This patch goes back a long time (introduced in Void in 2012, shadow 4.1.5), so many things may have changed. If you say it's not needed, I tend to trust you. Maybe @q66 can comment as to why Chimera still keeps it?

Choose a reason for hiding this comment

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

Hmmm, I'd prefer to know why it originated, before confirming that it should be removed, but most likely. Let's hear @q66 . :)

@dkwo
Copy link
Contributor

dkwo commented Mar 9, 2024

@alejandro-colomar Thanks a lot for taking a look. Do you think it would be possible to make stuff that conflicts with coreutils and util-linux (e.g. groups) optional via a configure? right now most distros have to patch it (and their man) out (see the patch taken from arch). Ditto for respecting usr/bin and usr/sbin config options.

For the Alpine patch, maybe can you take a look at this https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/50121 pull request? the first thread has a discussion about why this was done.

@alejandro-colomar
Copy link

alejandro-colomar commented Mar 9, 2024

Hi!

@alejandro-colomar Thanks a lot for taking a look.

=)

Do you think it would be possible to make stuff that conflicts with coreutils and util-linux (e.g. groups) optional via a configure?

I'm neutral to that, but others seem to not like the idea. (Now I see you're the same one that reported the issue shadow-maint/shadow#842.) How about opening an issue, not asking to make it conditional, but rather reporting the conflict with other projects? Maybe that's more convincing. You could document which distros use shadow's groups, and which distros use others' groups. Maybe we could merge the efforts from those other projects and shadow into a single groups implementation. While some competition is good, it might be good to merge at some point.

right now most distros have to patch it (and their man) out (see the patch taken from arch). Ditto for respecting usr/bin and usr/sbin config options.

You know what? I would wipe out the entire autotools-based build system, which has been more problematic than anything else. I would write a hand-written GNUmakefile that allows more flexibility. But some distro maintainers (cough, Gentoo, cough) opposed strongly.

Please, please, report a bug in shadow. That will add up to the current issues with the build system. :)

For the Alpine patch, maybe can you take a look at this https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/50121 pull request? the first thread has a discussion about why this was done.

Yup, I found that MR a moment ago, and sent an email https://lists.sr.ht/~hallyn/shadow/%3CZeyg8ClVMNeRifua%40debian%3E, https://lists.alpinelinux.org/~alpine/aports/%3CZeyg8ClVMNeRifua%40debian%3E.

@dataCobra
Copy link
Contributor Author

Thank you both for all the input and information.

I'm currently working on an update to 4.15.0.

@dataCobra dataCobra marked this pull request as draft March 20, 2024 06:31
@dataCobra dataCobra force-pushed the shadow branch 3 times, most recently from a05dcf8 to d3597ad Compare March 22, 2024 19:09
@dataCobra dataCobra changed the title shadow: update to 4.14.5. shadow: update to 4.15.0. Mar 22, 2024
@dkwo
Copy link
Contributor

dkwo commented Mar 22, 2024

it may also be possible to drop the ruserok patch

@alejandro-colomar
Copy link

alejandro-colomar commented Mar 22, 2024

it may also be possible to drop the ruserok patch

If you investigate that ruserok thing, you may want to have fun upstream with the "quick hack":

https://github.com/shadow-maint/shadow/blob/ead55e9ba8958504e23e29545f90c4dd925c7462/configure.ac#L162
shadow-maint/shadow@428a207
shadow-maint/shadow@45c6603#diff-bb21aa33a3f69ccb36c68b220f40ad08f29b9cd2c05dfedae7b9e3d5d4d08f6bR196

:)

@dataCobra
Copy link
Contributor Author

@Gottox are you able to help as the maintainer of the package?

If you have some more knowledge/information.

@dkwo
Copy link
Contributor

dkwo commented May 20, 2024

btw, 4.15.1 is out

@dataCobra
Copy link
Contributor Author

Hey, I'm currently not having the time to work on this on my own.

Help from the Maintainer @Gottox would be nice or another person who is capable of helping me out.

@dataCobra dataCobra changed the title shadow: update to 4.15.0. shadow: update to 4.16.0. Jul 14, 2024
@dataCobra
Copy link
Contributor Author

Updated the PR to shadow 4.16.0. But didn't had the time to check the package.

Just updating to make sure everyone who wants to can have a look.
Help and discussion is appreciated.

@alejandro-colomar

This comment was marked as off-topic.

Copy link

Pull Requests become stale 90 days after last activity and are closed 14 days after that. If this pull request is still relevant bump it or assign it.

@github-actions github-actions bot added Stale and removed Stale labels Oct 17, 2024
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.

4 participants