-
Notifications
You must be signed in to change notification settings - Fork 225
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
Simplify a lot of NIH stuff, and fix several bugs #991
Open
alejandro-colomar
wants to merge
30
commits into
shadow-maint:master
Choose a base branch
from
alejandro-colomar:reduce
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alejandro-colomar
changed the title
Simplify a lot of open-coded stuff, and fix a forever loop
Simplify a lot of NIH stuff, and fix a forever loop
May 13, 2024
alejandro-colomar
changed the title
Simplify a lot of NIH stuff, and fix a forever loop
Simplify a lot of NIH stuff, and fix several bugs
May 13, 2024
I've also documented strndup(3) within string_copying(7) as a function that duplicates an input null-padded character sequence into an output newly allocated string: commit 6af636f134d82b8d3838074fa6e0dce5efc356a4 (HEAD -> contrib, alx/contrib)
Author: Alejandro Colomar <[email protected]>
Date: Tue May 14 19:36:53 2024 +0200
string_copying.7: Document strndup(3)
Signed-off-by: Alejandro Colomar <[email protected]>
diff --git a/man/man7/string_copying.7 b/man/man7/string_copying.7
index 281fff8be..0be53d1cf 100644
--- a/man/man7/string_copying.7
+++ b/man/man7/string_copying.7
@@ -61,6 +61,9 @@ .SS Null-padded character sequences
// Catenate a null-padded character sequence into a string.
.BI "char *strncat(char *restrict " dst ", const char " src "[restrict ." ssize ],
.BI " size_t " ssize );
+.P
+// Duplicate a null-padded character sequence into a string.
+.BI "char *strndup(const char " src [. ssize "], size_t " ssize );
.fi
.\" ----- SYNOPSIS :: Known-length character sequences --------------------/
.SS Known-length character sequences
@@ -154,6 +157,11 @@ .SS Terms (and abbreviations)
(or one after the last character in a character sequence)
after the call,
so that the programmer can use it to chain such calls.
+.\" ----- DESCRIPTION :: Terms (and abbreviations) :: duplicate -------/
+.TP
+.I duplicate
+Allocate a new buffer
+where a copy is placed.
.\" ----- DESCRIPTION :: Copy, catenate, and chain-copy ---------------/
.SS Copy, catenate, and chain-copy
Originally,
@@ -252,6 +260,8 @@ .SS Null-padded character sequences
and then you can treat it as a known-length character sequence;
or use
.BR strncat (3)
+or
+.BR strndup (3)
directly.
.\" ----- DESCRIPTION :: Known-length character sequences -----------------/
.SS Known-length character sequences
@@ -342,7 +352,11 @@ .SS String vs character sequence
has an even more misleading name than the functions above.
List of functions:
.IP \[bu] 3
+.PD 0
.BR strncat (3)
+.IP \[bu]
+.BR strndup (3)
+.PD
.P
Other functions operate on an input character sequence
to create an output character sequence.
@@ -453,6 +467,15 @@ .SS Functions
.IP
.I \%stpcpy(mempcpy(dst,\ src,\ strnlen(src,\ NITEMS(src))),\ \[dq]\[dq])
is a faster alternative to this function.
+.\" ----- DESCRIPTION :: Functions :: strndup(3) ----------------------/
+.TP
+.BR strndup (3)
+Duplicate the input character sequence,
+contained in a null-padded fixed-size buffer,
+into a newly allocated destination string.
+.IP
+The string must be freed with
+.BR free (3).
.\" ----- DESCRIPTION :: Functions :: mempcpy(3) ----------------------/
.TP
.BR mempcpy (3)
@@ -508,6 +531,9 @@ .SH RETURN VALUE
.I dst
pointer,
which is useless.
+.TP
+.BR strndup (3)
+The newly allocated string.
.\" ----- ERRORS ------------------------------------------------------/
.SH ERRORS
Most of these functions don't set
@@ -526,6 +552,13 @@ .SH ERRORS
.B E2BIG
The string has been truncated.
.RE
+.TP
+.BR strndup (3)
+.RS
+.TP
+.B ENOMEM
+Insufficient memory available to allocate duplicate string.
+.RE
.\" ----- NOTES :: strscpy(9) -----------------------------------------/
.SH NOTES
The Linux kernel has an internal function for copying strings,
@@ -689,6 +722,15 @@ .SH EXAMPLES
len = strlen(buf);
puts(buf);
.EE
+.\" ----- EXAMPLES :: strndup(3) --------------------------------------/
+.TP
+.BR strndup (3)
+.EX
+buf = strndup(u->ut_user, NITEMS(u->ut_user));
+len = strlen(buf);
+puts(buf);
+free(buf);
+.EE
.\" ----- EXAMPLES :: mempcpy(3) --------------------------------------/
.TP
.BR mempcpy (3) |
alejandro-colomar
force-pushed
the
reduce
branch
2 times, most recently
from
May 17, 2024 23:27
4faf560
to
9a07175
Compare
@ikerexxe , if you want me to divide this into smaller PRs, let me know more or less the size you want to handle in each. ;) |
alejandro-colomar
force-pushed
the
reduce
branch
7 times, most recently
from
May 20, 2024 16:09
df88606
to
291b3e3
Compare
@ikerexxe The codeq failure is spurious (see the resolved reports above). Feel free to review and merge when you find some time. |
alejandro-colomar
force-pushed
the
reduce
branch
4 times, most recently
from
May 21, 2024 14:03
a81ac44
to
55119d1
Compare
Fixes: e6c2e43 ("Hardcoding Prog to known value") Signed-off-by: Alejandro Colomar <[email protected]>
wait(2) accepts NULL if the status won't be read. Simplify. Signed-off-by: Alejandro Colomar <[email protected]>
The functions that set these strings --do_rlogin() and login_prompt()-- make sure to terminate them with a NUL. Fixes: 3704745 ("* lib/defines.h: Define USER_NAME_MAX_LENGTH, based on utmp and [...]") Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
…tring Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
See time(2): BUGS Error returns from this system call are indistinguishable from successful reports that the time is a few seconds before the Epoch, so the C library wrapper function never sets errno as a re‐ sult of this call. The tloc argument is obsolescent and should always be NULL in new code. When tloc is NULL, the call cannot fail. Fixes: 45c6603 ("[svn-upgrade] Integrating new upstream version, shadow (19990709)") Signed-off-by: Alejandro Colomar <[email protected]>
This should have gone into the #else'd branch in 8451bed, and should have been removed in 3e602b5. Fixes: 8451bed ("[svn-upgrade] Integrating new upstream version, shadow (4.0.13)") Fixes: 3e602b5 ("Remove HAVE_STRFTIME ifdefs") Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
…ENOMEM We don't need to set ENOMEM on failure of those functions. Signed-off-by: Alejandro Colomar <[email protected]>
In the files where #include <string.h> is missing, add it, and sort the includes. Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Before this patch, the function looped while (s != NULL && *s != '\0'). However, nothing was modifying that string if REALLOC() failed, so the loop was forever. Fixes: 8e167d2 ("[svn-upgrade] Integrating new upstream version, shadow (4.0.8)") Signed-off-by: Alejandro Colomar <[email protected]>
Fixes: efbbcad ("Use safer allocation macros") Signed-off-by: Alejandro Colomar <[email protected]>
We are setting `sgrent.sg_adm[1] = NULL;`, so we need 2 elements. Fixes: 87b56b1 ("* NEWS, src/groupmems.c, man/groupmems.8.xml: Added support for [...]") Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
This is like ZUSTR2STP(), but returns dst instead of dst + strlen(dst). Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
XSTRNDUP() is similar to ZUSTR2STP() but allocating the destination buffer. This means that we must change the buffers to be allocated in the call instead of automatic storage, which itself means we must free(3) the memory after using. The benefits of this refactor are: - The allocation size is always correct, and needs no comments, since it's now automatically calculated by the macro. - XSTRNDUP() is probably more familiar, since - strndup(3) is a standard libc function, - XSTRNDUP() is the obvious wrapper that - exit()s on ENOMEM, - calculates the size based on the input array. - We can remove ZUSTR2STP(). Or actually, we can use it internally just for implementing XSTRNDUP(), but stop using it in other code. Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
ZUSTR2STR() is now only used by XSTRNDUP(), where 'dst' is a recently allocated buffer (so, not an array). Thus, we can remove the static_assert(3), which is now useless. ZUSTR2STP() is unused. ZUSTR2STR() is much simpler by implementing it as a STRNCAT() wrapper. Also, this macro can be documented by comparing them to strndup(3) and strncat(3). That's much easier to understand than a fully descriptive manual page. Signed-off-by: Alejandro Colomar <[email protected]>
…p(3) The program was happily ignoring ENOMEM errors. Fixes: 7f9e196 ("* NEWS, src/newusers.c, src/Makefile.am: Added support for") Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was looking at some code about REALLOC(), due to my recent implementation of these macros in neomutt(1), just to do some sanity checks.
While doing that, I found that a few cases were still open-coding the REALLOCF() variant, so I changed that. Then I found nearby some other nasty open-coded versions of strcspn(3) and strsep(3), which I also adapted (in several steps). Then I found another case of REALLOCF().
And finally, while doing some of those simplifications, they made obvious that there was a forever loop in a mis-handled error from REALLOC().
Sorry for the churn, but it helped find a bug (edit: two bugs (edit: three bugs)), so I guess it's worth it.
I hope I didn't do any mistakes in the simplifications... :-)
Revisions:
v2
v3
v3b
v4
v5
v6
v6b
v7
v7b
v7c
v8
v9
v10
v11
v12
v13
v13b
v13c
v14
v14b
v15
v15b
v15c