12:28 p.m.
On 6/19/23 16:36, Vincent Mailhol wrote:
If /sys/block can not be opened, get_devices() returns NULL.
cmdline() does not check this result and below code snippet:
scanned = get_devices();
devices = (gchar **) scanned->data;
results in a segmentation fault.
Add a check on scanned.
Relevant logs:
Unable to open /sys/block: No such file or directory
[ 0.777352] ldmtool[164]: segfault at 0 ip 0000563a225cd6a5 sp 00007ffe54965a60
error 4 in ldmtool[563a225cb000+3000]
[ 0.778278] Code: 18 64 48 33 1c 25 28 00 00 00 75 5e 48 83 c4 28 5b 5d 41 5c 41 5d
41 5e 41 5f c3 66 2e 0f 1f 84 00 00 00 00 00 e8 db fd ff ff <4c> 8b 20 48 89 44 24
08 4c 89 e7 e8 0b e1 ff ff 45 31 c0 4c 89 e1
Fixes: 25d9635e4ee5 ("Add ldmtool")
Signed-off-by: Vincent Mailhol <mailhol.vincent(a)wanadoo.fr>
---
This thread did not yet show-up in
https://listman.redhat.com/archives/libguestfs/2023-June/subject.html
not sure why.
For this reason, I couln't add a link reference.
---
src/ldmtool.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/ldmtool.c b/src/ldmtool.c
index 6957c1a..87aaccc 100644
--- a/src/ldmtool.c
+++ b/src/ldmtool.c
@@ -746,6 +746,8 @@ cmdline(LDM * const ldm, gchar **devices,
GArray * scanned = NULL;
if (!devices) {
scanned = get_devices();
+ if (!scanned)
+ goto error;
devices = (gchar **) scanned->data;
}
(I'm reviewing this against commit 5014da5b9071, in repository
<https://github.com/mdbooth/libldm.git>.)
This fix is almost right, but not entirely. Consider what we have under
the "error" label:
error:
if (scanned) g_array_unref(scanned);
if (jb) g_object_unref(jb);
return FALSE;
When we jump to "error" on the new branch, "scanned" is is NULL, so
that
is safe. However, the
JsonBuilder *jb = NULL;
line has not been reached yet, and that's a problem, because
g_object_unref() will receive an indeterminate value (undefined
behavior). (In fact, just *evaluating* "jb" as the controlling
expression of the "if" that is supposed to protect g_object_unref() is
*already* undefined behavior.)
According to C99 6.8 "Statements and blocks" paragraph 3,
A /block/ allows a set of declarations and statements to be grouped
into one syntactic unit. The initializers of objects that have
automatic storage duration [...] are evaluated and the values are
stored in the objects (including storing an indeterminate value in
objects without an initializer) each time the declaration is reached
in the order of execution, as if it were a statement, and within
each declaration in the order that declarators appear.
Furthermore, C99 6.8.4.2 "The switch statement" paragraph 7 provides the
following example:
In the artificial program fragment
switch (expr)
{
int i = 4;
f(i);
case 0:
i = 17;
/* falls through into default code */
default:
printf("%d\n", i);
}
the object whose identifier is *i* exists with automatic storage
duration (within the block) but is never initialized, and thus if
the controlling expression has a nonzero value, the call to the
*printf* function will access an indeterminate value. Similarly, the
call to the function *f* cannot be reached.
Thus, you can do one of two things (at least):
- *In addition* to the current contents of the patch, move the
declaration of "jb", together with its NULL initializer, to the top of
the function (just below that of "scanned"). This makes sure that the
new jump to "error" will still "pass" a well-defined "jb"
value (NULL),
which the logic at the "error" label handles well.
- Alternatively: replace "goto error" with just "return FALSE" in
your
patch. At that point, there is nothing to release yet.
Laszlo