Improve allocations #649
Merged
Improve allocations #649
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
force-pushed
the
alloc
branch
from
a5edea0
to
865ac8e
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Regarding the "high" report from CodeQL ( Like what? That's exactly what we do. This analyzer be kidding us :) |
alejandro-colomar
force-pushed
the
alloc
branch
5 times, most recently
from
f37f147
to
ea048a4
Compare
|
Rebased to master. $ git range-diff gh/master..gh/alloc master..alloc
1: 85b5ccaa < -: -------- Remove superfluous casts
2: b277bf98 < -: -------- ttytype(): Fix race
3: 41cc3c4c = 1: 03aa642e Rely on realloc(NULL, ...) being equivalent to malloc(...)
4: 6b5cb73a = 2: b9355fd6 malloc(3) already sets errno to ENOMEM
5: 2bc8c628 = 3: fd163a61 Use reallocf(3) instead of its pattern
6: 502221f2 = 4: 8fa33624 Use reallocarray(3) instead of its pattern
7: 8011b006 = 5: 60e1bee6 Use calloc(3) instead of its pattern
8: b0fade29 = 6: ebc30525 Add safer allocation functions
9: 3e936455 = 7: 31471ecb Use xcalloc(3) instead of its pattern
10: 9630f967 = 8: 918a3957 Use the new header for xstrdup()
11: ed5af2c5 = 9: 1676ebad Use *array() allocation functions where appropriate
12: aa91c6a3 = 10: f87ad6ef Use reallocarrayf() instead of its pattern
13: 759afd46 = 11: 6066d8fd Use xreallocarray() instead of its pattern
14: c4820476 = 12: e7416270 Add safer allocation macros
15: 06ea8fb8 = 13: 6eaf62e8 Use safer allocation macros
16: f37f1470 = 14: ea048a41 Fix use-after-free of pointer after realloc(3) |
|
Is this PR ready for review or are you planning to add more commits? I'd prefer to review everything in a row. |
Yes, I think it's ready. No plans :) |
ikerexxe
requested changes
Feb 16, 2023
There was a problem hiding this comment.
Two major comments apart from the ones inline. The first one is related to the malloc.* files naming. As they contain more functionality apart from malloc I wonder if we should rename it to something more generic.
The second one is related to the new macros provided in e741627. Wouldn't it be better to add this functionality to the functions themselves? We are already adding an abstraction and I'd prefer to avoid having a second one.
By the way, I didn't review properly the last three commits because I wanted to tackle my previous concern first.
lib/malloc.h
Outdated
|
|
||
| #define REALLOCARRAY(ptr, n, type) \ | ||
| ({ \ | ||
| __auto_type p_ = (ptr); \ |
There was a problem hiding this comment.
Is __auto_type standard C?
There was a problem hiding this comment.
No. It's GNU C, and available in GCC and Clang. ISO C23 will likely add auto with the same meaning, AFAIK (and hopefully deprecate the old meaning of auto).
There was a problem hiding this comment.
BTW, another GNU extension used here (also available at least in GCC and Clang) is ({ ... }): https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html.
This is a very basic feature for writing safe macros, and any good compiler should support it, but AFAIK it's not yet in the standard.
alejandro-colomar
force-pushed
the
alloc
branch
from
ea048a4
to
c86a6b7
Compare
|
Changes:
$ git range-diff gh/alloc...alloc
-: -------- > 1: ca9e309d Fix VPATH build
-: -------- > 2: a187ad8e agetpass.c: Use SPDX tags
-: -------- > 3: 7e213cfb Add stpeprintf()
-: -------- > 4: 46610792 Use stpeprintf() where appropriate
-: -------- > 5: 8a9285aa Remove unnecessary NUL terminators
-: -------- > 6: e0e9e57a Add mempcpy(3)
-: -------- > 7: 709e6b44 Add stpecpy()
-: -------- > 8: 5956cea1 Use stpecpy() where appropriate
1: 03aa642e = 9: 231aa1cc Rely on realloc(NULL, ...) being equivalent to malloc(...)
2: b9355fd6 = 10: 820e65a4 malloc(3) already sets errno to ENOMEM
3: fd163a61 = 11: 27681f5e Use reallocf(3) instead of its pattern
4: 8fa33624 = 12: 1ca3a789 Use reallocarray(3) instead of its pattern
5: 60e1bee6 = 13: 56e751b2 Use calloc(3) instead of its pattern
6: ebc30525 ! 14: 7c4f8c73 Add safer allocation functions
@@ libmisc/Makefile.am: libmisc_la_SOURCES = \
loginprompt.c \
mail.c \
+ malloc.c \
+ mempcpy.c \
motd.c \
myname.c \
- obscure.c \
@@ libmisc/Makefile.am: libmisc_la_SOURCES = \
xgetgrnam.c \
xgetgrgid.c \
7: 31471ecb = 15: 82dd2db7 Use xcalloc(3) instead of its pattern
8: 918a3957 = 16: 52c661cd Use the new header for xstrdup()
9: 1676ebad = 17: d9da1bd0 Use *array() allocation functions where appropriate
10: f87ad6ef = 18: 6fec8ba7 Use reallocarrayf() instead of its pattern
11: 6066d8fd = 19: 15e1695c Use xreallocarray() instead of its pattern
12: e7416270 = 20: 09518b7b Add safer allocation macros
13: 6eaf62e8 ! 21: 87aeb91d Use safer allocation macros
@@ libmisc/idmapping.c
+
+#include "malloc.h"
#include "prototypes.h"
+ #include "stpeprintf.h"
#include "idmapping.h"
- #if HAVE_SYS_CAPABILITY_H
@@ libmisc/idmapping.c: struct map_range *get_map_ranges(int ranges, int argc, char **argv)
return NULL;
}
@@ libmisc/idmapping.c: void write_mapping(int proc_dir_fd, int ranges, const struc
bufsize = ranges * ((ULONG_DIGITS + 1) * 3);
- pos = buf = xmalloc(bufsize);
+ pos = buf = XMALLOCARRAY(bufsize, char);
+ end = buf + bufsize;
/* Build the mapping command */
- mapping = mappings;
## libmisc/list.c ##
@@
@@ src/groupmod.c: static void prepare_failure_reports (void)
#endif
info_passwd.name = group_name;
-- info_group.audit_msg = xmalloc (512);
-+ info_group.audit_msg = XMALLOCARRAY (512, char);
+- gr = xmalloc (512);
++ gr = XMALLOCARRAY(512, char);
+ info_group.audit_msg = gr;
+ gr_end = gr + 512;
#ifdef SHADOWGRP
-- info_gshadow.audit_msg = xmalloc (512);
-+ info_gshadow.audit_msg = XMALLOCARRAY (512, char);
+- sgr = xmalloc (512);
++ sgr = XMALLOCARRAY(512, char);
+ info_gshadow.audit_msg = sgr;
+ sgr_end = sgr + 512;
#endif
-- info_passwd.audit_msg = xmalloc (512);
-+ info_passwd.audit_msg = XMALLOCARRAY (512, char);
+- pw = xmalloc (512);
++ pw = XMALLOCARRAY(512, char);
+ info_passwd.audit_msg = pw;
+ pw_end = pw + 512;
- (void) snprintf (info_group.audit_msg, 512,
- "changing %s; ", gr_dbname ());
## src/groups.c ##
@@
14: ea048a41 = 22: c86a6b73 Fix use-after-free of pointer after realloc(3) |
alejandro-colomar
force-pushed
the
alloc
branch
from
c86a6b7
to
5d215a3
Compare
|
Changes:
$ git range-diff c86a6b73...alloc
1: d9da1bd0 ! 1: 648fc0ad Use *array() allocation functions where appropriate
@@ lib/subordinateio.c: static bool append_range(struct subid_range **ranges, const
} else {
struct subid_range *alloced;
- alloced = realloc(*ranges, (n + 1) * (sizeof(struct subid_range)));
-+ alloced = reallocarray(*ranges, n+1, sizeof(struct subid_range));
++ alloced = reallocarray(*ranges, n + 1, sizeof(struct subid_range));
if (!alloced)
return false;
*ranges = alloced;
2: 6fec8ba7 = 2: 8710f813 Use reallocarrayf() instead of its pattern
3: 15e1695c = 3: 9fffea5d Use xreallocarray() instead of its pattern
4: 09518b7b = 4: a6d04d28 Add safer allocation macros
5: 87aeb91d ! 5: 02ef2cc4 Use safer allocation macros
@@ lib/subordinateio.c: static bool have_range(struct commonio_db *db,
return false;
} else {
struct subid_range *alloced;
-- alloced = reallocarray(*ranges, n+1, sizeof(struct subid_range));
+- alloced = reallocarray(*ranges, n + 1, sizeof(struct subid_range));
+ alloced = REALLOCARRAY(*ranges, n + 1, struct subid_range);
if (!alloced)
return false;
6: c86a6b73 = 6: 5d215a3a Fix use-after-free of pointer after realloc(3) |
I thought about calling it
I put most of the functionality in the functions, as they are safer, and then only put in macros what can't be put in functions, which is the cast and
Sure, makes sense. |
wangliu-iscas
pushed a commit
to plctlab/patchwork-gcc
that referenced
this pull request
Feb 17, 2023
Link: <https://inbox.sourceware.org/gcc/3098fd18-9dbf-b4e9-bae5-62ec6fea74cd@opteya.com/T/> Link: <shadow-maint/shadow#649 (comment)> Cc: Andreas Schwab <schwab@linux-m68k.org> Cc: David Malcolm <dmalcolm@redhat.com> Cc: Florian Weimer <fweimer@redhat.com> Cc: Iker Pedrosa <ipedrosa@redhat.com> Cc: Jens Gustedt <jens.gustedt@inria.fr> Cc: Jonathan Wakely <jwakely.gcc@gmail.com> Cc: Mark Wielaard <mark@klomp.org> Cc: Martin Uecker <uecker@tugraz.at> Cc: Michael Kerrisk <mtk.manpages@gmail.com> Cc: Paul Eggert <eggert@cs.ucla.edu> Cc: Sam James <sam@gentoo.org> Cc: Siddhesh Poyarekar <siddhesh@gotplt.org> Cc: Yann Droneaud <ydroneaud@opteya.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
force-pushed
the
alloc
branch
from
5d215a3
to
891ce8f
Compare
|
Changes:
|
alejandro-colomar
force-pushed
the
alloc
branch
from
891ce8f
to
18655c0
Compare
|
Changes:
|
| inline char * | ||
| xstrdup(const char *str) | ||
| { | ||
| return strcpy(XMALLOCARRAY(strlen(str) + 1, char), str); |
Check failure
Code scanning / CodeQL
Unbounded write
ikerexxe
reviewed
Feb 20, 2023
Yes 😄
C++ templates are so handy...
Reviewed. |
This is guaranteed by ISO C. Now that we require ISO C (and even POSIX) to compile, we can simplify this code. Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
hallyn
reviewed
Feb 22, 2023
|
Huh. I don't know where I was looking before, but now I see:
SUSv2 requires malloc(), calloc(), and realloc() to set errno to ENOMEM upon failure.
( https://manpages.ubuntu.com/manpages/jammy/en/man3/malloc.3.html )
So all's good, thanks.
|
Thank you! BTW, that text is a bit old for me :D It was largely rewritten in |
hallyn
reviewed
Feb 23, 2023
Mostly looks good to me, thanks - I still need to look through 12-14, but have to run now. |
hallyn
reviewed
Feb 23, 2023
hallyn
reviewed
Feb 23, 2023
| #define REALLOC(ptr, type) REALLOCARRAY(ptr, 1, type) | ||
| #define REALLOCF(ptr, type) REALLOCARRAYF(ptr, 1, type) | ||
|
|
||
| #define REALLOCARRAY(ptr, n, type) \ |
There was a problem hiding this comment.
Once these are merged, it might be worth having a CI step that looks for non-modified users so we can catch them before they go in.
hallyn
reviewed
Feb 23, 2023
hallyn
approved these changes
Feb 23, 2023
alejandro-colomar
force-pushed
the
alloc
branch
from
7339875
to
edf3a6a
Compare
|
Changes:
|
In addition, don't set local variables just before return. Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We'll expand the contents in a following commit, so let's move the file to a more generic name, have a dedicated header, and update includes. Signed-off-by: Alejandro Colomar <alx@kernel.org> Use the new header for xstrdup() Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This prevents overflow from multiplication. Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This macros have several benefits over the standard functions:
- The type of the allocated object (not the pointer) is specified as an
argument, which improves readability:
- It is directly obvious what is the type of the object just by
reading the macro call.
- It allows grepping for all allocations of a given type.
This is admittedly similar to using sizeof() to get the size of the
object, but we'll see why this is better.
- In the case of reallocation macros, an extra check is performed to
make sure that the previous pointer was compatible with the allocated
type, which can avoid some mistakes.
- The cast is performed automatically, with a pointer type derived from
the type of the object. This is the best point of this macro, since
it does an automatic cast, where there's no chance of typos.
Usually, programmers have to decide whether to cast or not the result
of malloc(3). Casts usually hide warnings, so are to be avoided.
However, these functions already return a void *, so a cast doesn't
really add much danger. Moreover, a cast can even add warnings in
this exceptional case, if the type of the cast is different than the
type of the assigned pointer. Performing a manual cast is still not
perfect, since there are chances that a mistake will be done, and
even ignoring accidents, they clutter code, hurting readability.
And now we have a cast that is synced with sizeof.
- Whenever the type of the object changes, since we perform an explicit
cast to the old type, there will be a warning due to type mismatch in
the assignment, so we'll be able to see all lines that are affected
by such a change. This is especially important, since changing the
type of a variable and missing to update an allocation call far away
from the declaration is easy, and the consequences can be quite bad.
Cc: Valentin V. Bartenev <vbartenev@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Use of these macros, apart from the benefits mentioned in the commit that adds the macros, has some other good side effects: - Consistency in getting the size of the object from sizeof(type), instead of a mix of sizeof(type) sometimes and sizeof(*p) other times. - More readable code: no casts, and no sizeof(), so also shorter lines that we don't need to cut. - Consistency in using array allocation calls for allocations of arrays of objects, even when the object size is 1. Cc: Valentin V. Bartenev <vbartenev@gmail.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
We can't use a pointer that was input to realloc(3), nor any pointers that point to reallocated memory, without making sure that the memory wasn't moved. If we do, the Behavior is Undefined. Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
force-pushed
the
alloc
branch
from
edf3a6a
to
11d9230
Compare
|
Changes:
|
hallyn
reviewed
Feb 25, 2023
| @@ -76,7 +76,7 @@ static uid_t getnamuid(const char *name) { | |||
| } | |||
|
|
|||
| static int alloc_uid(uid_t **uids, uid_t id) { | |||
| *uids = malloc(sizeof(uid_t)); | |||
| *uids = MALLOC(uid_t); | |||
There was a problem hiding this comment.
Oh, this is actually breaking build of libsubid_zzz during tests.
Not sure yet whether just adding #include "alloc.h" at the top suffices.
There was a problem hiding this comment.
Yes, that should work.
There was a problem hiding this comment.
I'm not sure if that library links to libmisc, though, which might be a problem if not.
skissane
pushed a commit
to skissane/man-pages
that referenced
this pull request
Feb 26, 2023
mallocarray() is safer than malloc(3), since it checks for overflow; it
should be preferred almost always (with the exception of non-arrays
maybe).
The macros like MALLOCARRAY() --and MALLOC()-- that perform automatic
casting and sizeof() are also safer than calling the functions directly:
- The type of the allocated object (not the pointer) is specified as an
argument, which improves readability:
- It is directly obvious what is the type of the object just by
reading the macro call.
- It allows grepping for all allocations of a given type.
This is admittedly similar to using sizeof() to get the size of the
object, but we'll see why this is better.
- In the case of reallocation macros, an extra check is performed to
make sure that the previous pointer was compatible with the allocated
type, which can avoid some mistakes.
- The cast is performed automatically, with a pointer type derived from
the type of the object. This is the best point of this macro, since
it does an automatic cast, where there's no chance of typos.
Usually, programmers have to decide whether to cast or not the result
of malloc(3). Casts usually hide warnings, so are to be avoided.
However, these functions already return a void *, so a cast doesn't
really add much danger. Moreover, a cast can even add warnings in
this exceptional case, if the type of the cast is different than the
type of the assigned pointer. Performing a manual cast is still not
perfect, since there are chances that a mistake will be done, and
even ignoring accidents, they clutter code, hurting readability.
And now we have a cast that is synced with sizeof.
- Whenever the type of the object changes, since we perform an explicit
cast to the old type, there will be a warning due to type mismatch in
the assignment, so we'll be able to see all lines that are affected
by such a change. This is especially important, since changing the
type of a variable and missing to update an allocation call far away
from the declaration is easy, and the consequences can be quite bad
Apart from those benefits, there are other minor style benefits:
- Consistency in getting the size of the object from sizeof(type),
instead of a mix of sizeof(type) sometimes and sizeof(*p) other
times.
- More readable code: no casts, and no sizeof(), so also shorter lines
that we don't need to cut.
- Consistency in using array allocation calls for allocations of arrays
of objects, even when the object size is 1.
Link: <shadow-maint/shadow#649>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Cc: "Valentin V. Bartenev" <vbartenev@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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.
No description provided.