Es posible que ya haya leído el artículo reciente sobre el proceso de inicio de PVS-Studio por primera vez utilizando el ejemplo del proyecto GTK 4 y sobre el filtrado principal de advertencias. Ahora es el momento de trabajar con el informe recibido con más detalle. Y como nuestros lectores habituales ya han adivinado, les traigo un artículo que describe los errores encontrados en el código.
El proyecto GTK 4 tiene buena calidad de código
No siempre estoy de humor para escribir más errores en el artículo, como fue el caso, por ejemplo, en el caso de la publicación reciente " Espressif IoT Development Framework: 71 Shots in the Leg ". Esta vez me limitaré a 21 errores en honor al 2021 :). Además, para ser justos, debe tenerse en cuenta la alta calidad del proyecto GTK 4 y que la densidad de errores reales es muy baja.
, , , , Clang static analysis tool, Coverity, AddressSanitizer, UndefinedBehavior Sanitizer. , , - — .
GTK 4 , "GTK: ". 581 ( ). 581 : ? .
, , , . - . , , . , , . .
, :
bool var;
var = true;
if (!var && foo)
, , . ? - var? , , . PVS-Studio "A part of conditional expression is always false: !var". ? .
, , . GTK 4, :
gboolean debug_enabled;
#ifdef G_ENABLE_DEBUG
debug_enabled = TRUE;
#else
debug_enabled = FALSE;
#endif
....
if (!debug_enabled && !keys[i].always_enabled)
: V560 [CWE-570] A part of conditional expression is always false: !debug_enabled. gdk.c 281
, . , . .
, . , . .
N1.
void
gsk_vulkan_image_upload_regions (GskVulkanImage *self,
GskVulkanUploader *uploader,
guint num_regions,
GskImageRegion *regions)
{
....
for (int i = 0; i < num_regions; i++)
{
m = mem + offset;
if (regions[i].stride == regions[i].width * 4)
{
memcpy (m, regions[i].data, regions[i].stride * regions[i].height);
}
else
{
for (gsize r = 0; r < regions[i].height; i++) // <=
memcpy (m + r * regions[i].width * 4,
regions[i].data + r * regions[i].stride, regions[i].width * 4);
}
....
}
....
}
PVS-Studio: V533 [CWE-691] It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. gskvulkanimage.c 721
, r, i. - . !
N2.
. , memcpy - , . .. Segmentation fault.
, :
GtkCssValue *
_gtk_css_border_value_parse (GtkCssParser *parser,
GtkCssNumberParseFlags flags,
gboolean allow_auto,
gboolean allow_fill)
{
....
guint i;
....
for (; i < 4; i++) // <=
{
if (result->values[(i - 1) >> 1])
result->values[i] = _gtk_css_value_ref (result->values[(i - 1) >> 1]);
}
result->is_computed = TRUE;
for (; i < 4; i++) // <=
if (result->values[i] && !gtk_css_value_is_computed (result->values[i]))
{
result->is_computed = FALSE;
break;
}
....
}
PVS-Studio: V621 [CWE-835] Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. gtkcssbordervalue.c 221
, i 4. , . .
N3.
static void
gtk_list_base_class_init (GtkListBaseClass *klass)
{
....
properties[PROP_ORIENTATION] =
g_param_spec_enum ("orientation",
P_("Orientation"),
P_("The orientation of the orientable"),
GTK_TYPE_ORIENTATION,
GTK_ORIENTATION_VERTICAL,
G_PARAM_READWRITE | G_PARAM_EXPLICIT_NOTIFY |
G_PARAM_EXPLICIT_NOTIFY);
....
}
PVS-Studio: V501 There are identical sub-expressions 'G_PARAM_EXPLICIT_NOTIFY' to the left and to the right of the '|' operator. gtklistbase.c 1151
G_PARAM_EXPLICIT_NOTIFY. , .
N4.
post_insert_fixup. char_count_delta line_count_delta.
static void post_insert_fixup (GtkTextBTree *tree,
GtkTextLine *insert_line,
int char_count_delta,
int line_count_delta);
, , :
void
_gtk_text_btree_insert (GtkTextIter *iter,
const char *text,
int len)
{
....
int line_count_delta; /* Counts change to total number of
* lines in file.
*/
int char_count_delta; /* change to number of chars */
....
post_insert_fixup (tree, line, line_count_delta, char_count_delta);
....
}
PVS-Studio: V764 [CWE-683] Possible incorrect order of arguments passed to 'post_insert_fixup' function: 'line_count_delta' and 'char_count_delta'. gtktextbtree.c 1230
- . , , .
N5.
:
static guint
translate_keysym (GdkX11Keymap *keymap_x11,
guint hardware_keycode,
int group,
GdkModifierType state,
int *effective_group,
int *effective_level)
{
....
}
, :
static gboolean
gdk_x11_keymap_translate_keyboard_state (GdkKeymap *keymap,
guint hardware_keycode,
GdkModifierType state,
int group,
guint *keyval,
int *effective_group,
int *level,
GdkModifierType *consumed_modifiers)
{
....
tmp_keyval = translate_keysym (keymap_x11, hardware_keycode,
group, state,
level, effective_group); // <=
....
}
PVS-Studio: V764 [CWE-683] Possible incorrect order of arguments passed to 'translate_keysym' function: 'level' and 'effective_group'. gdkkeys-x11.c 1386
- . : level effective_group.
- PVS-Studio, :). ? , , ?
N6. (*)
gboolean
gtk_check_compact_table (...., int n_compose, ....)
{
....
guint16 *seq_index;
....
seq_index = bsearch (compose_buffer,
table->data,
table->n_index_size,
sizeof (guint16) * table->n_index_stride,
compare_seq_index);
if (!seq_index)
return FALSE;
if (seq_index && n_compose == 1)
return TRUE;
....
}
PVS-Studio: V560 [CWE-571] A part of conditional expression is always true: seq_index. gtkimcontextsimple.c 475
seq_index . . , , , , , . , , . :
if (!seq_index)
return FALSE;
if (*seq_index && n_compose == 1)
return TRUE;
N7-N9.
static void
gtk_message_dialog_init (GtkMessageDialog *dialog)
{
GtkMessageDialogPrivate *priv = ....;
....
priv->has_primary_markup = FALSE;
priv->has_secondary_text = FALSE;
priv->has_primary_markup = FALSE;
priv->has_secondary_text = FALSE;
....
}
PVS-Studio:
- V519 [CWE-563] The 'priv->has_primary_markup' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 262, 264. gtkmessagedialog.c 264
- V519 [CWE-563] The 'priv->has_secondary_text' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 263, 265. gtkmessagedialog.c 265
:
priv->has_primary_markup = FALSE; priv->has_secondary_text = FALSE;
, . , - . - .
:
- V519 [CWE-563] The 'self->state' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2851, 2855. gdkevents.c 2855
- V519 [CWE-563] The 'display->width' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2635, 2640. gtktextlayout.c 2640
N10.
static gboolean
on_flash_timeout (GtkInspectorWindow *iw)
{
iw->flash_count++;
gtk_highlight_overlay_set_color (GTK_HIGHLIGHT_OVERLAY (iw->flash_overlay),
&(GdkRGBA) {
0.0, 0.0, 1.0,
(iw && iw->flash_count % 2 == 0) ? 0.0 : 0.2
});
....
}
PVS-Studio: V595 [CWE-476] The 'iw' pointer was utilized before it was verified against nullptr. Check lines: 194, 199. inspect-button.c 194
iw , . , . :
(iw && iw->flash_count % 2 == 0)
, :
if (iw)
iw->flash_count++;
gtk_highlight_overlay_set_color (GTK_HIGHLIGHT_OVERLAY (iw->flash_overlay),
&(GdkRGBA) {
0.0, 0.0, 1.0,
(iw && iw->flash_count % 2 == 0) ? 0.0 : 0.2
});
, :). , :
GTK_HIGHLIGHT_OVERLAY (iw->flash_overlay)
, , , .
N11.
static void
cups_dispatch_watch_finalize (GSource *source)
{
GtkPrintCupsDispatchWatch *dispatch;
....
const char *username;
char hostname[HTTP_MAX_URI];
char *key;
httpGetHostname (dispatch->request->http, hostname, sizeof (hostname));
if (is_address_local (hostname))
strcpy (hostname, "localhost");
if (dispatch->backend->username != NULL) // <=
username = dispatch->backend->username; // <=
else
username = cupsUser ();
key = g_strconcat (username, "@", hostname, NULL);
GTK_NOTE (PRINTING,
g_print ("CUPS backend: removing stored password for %s\n", key));
g_hash_table_remove (dispatch->backend->auth, key); // <=
g_free (key);
if (dispatch->backend) // <=
dispatch->backend->authentication_lock = FALSE;
....
}
PVS-Studio: V595 [CWE-476] The 'dispatch->backend' pointer was utilized before it was verified against nullptr. Check lines: 1603, 1613. gtkprintbackendcups.c 1603
"" :). dispatch->backend (. , ). , - ! :
if (dispatch->backend)
:).
N12. ,
static GskRenderNode *
gtk_snapshot_collect_blend_top (GtkSnapshot *snapshot,
GtkSnapshotState *state,
GskRenderNode **nodes,
guint n_nodes)
{
GskRenderNode *bottom_node, *top_node, *blend_node;
GdkRGBA transparent = { 0, 0, 0, 0 };
top_node = gtk_snapshot_collect_default (snapshot, state, nodes, n_nodes);
bottom_node = state->data.blend.bottom_node != NULL
? gsk_render_node_ref (state->data.blend.bottom_node)
: NULL;
g_assert (top_node != NULL || bottom_node != NULL);
if (top_node == NULL)
top_node = gsk_color_node_new (&transparent, &bottom_node->bounds);
if (bottom_node == NULL)
bottom_node = gsk_color_node_new (&transparent, &top_node->bounds);
....
}
V595 [CWE-476] The 'bottom_node' pointer was utilized before it was verified against nullptr. Check lines: 1189, 1190. gtksnapshot.c 1189
, top_node bottom_node . :
g_assert (top_node != NULL || bottom_node != NULL);
, g_assert . . , :
if (top_node == NULL && bottom_node == NULL)
{
g_assert (false);
return NULL;
}
N13.
static void
stash_desktop_startup_notification_id (void)
{
const char *desktop_startup_id;
desktop_startup_id = g_getenv ("DESKTOP_STARTUP_ID");
if (desktop_startup_id && *desktop_startup_id != '\0')
{
if (!g_utf8_validate (desktop_startup_id, -1, NULL))
g_warning ("DESKTOP_STARTUP_ID contains invalid UTF-8");
else
startup_notification_id =
g_strdup (desktop_startup_id ? desktop_startup_id : "");
}
g_unsetenv ("DESKTOP_STARTUP_ID");
}
PVS-Studio: V547 [CWE-571] Expression 'desktop_startup_id' is always true. gdk.c 176
, , . , , -, , -, .
, :
desktop_startup_id = g_getenv ("DESKTOP_STARTUP_ID");
if (desktop_startup_id && *desktop_startup_id != '\0')
{
if (!g_utf8_validate (desktop_startup_id, -1, NULL))
g_warning ("DESKTOP_STARTUP_ID contains invalid UTF-8");
else
startup_notification_id = g_strdup (desktop_startup_id);
}
N14.
, . . / , . , , . , .
, :
#define MAX_LIST_SIZE 1000
static void
gtk_recent_manager_real_changed (GtkRecentManager *manager)
{
....
int age;
int max_size = MAX_LIST_SIZE;
....
.... // max_size .
....
if (age == 0 || max_size == 0 || !enabled)
{
g_bookmark_file_free (priv->recent_items);
priv->recent_items = g_bookmark_file_new ();
priv->size = 0;
}
else
{
if (age > 0)
gtk_recent_manager_clamp_to_age (manager, age);
if (max_size > 0)
gtk_recent_manager_clamp_to_size (manager, max_size);
}
....
}
PVS-Studio: V547 [CWE-571] Expression 'max_size > 0' is always true. gtkrecentmanager.c 480
, max_size , . . , , - .
. : "max_size == 0"? . . : V560 [CWE-570] A part of conditional expression is always false: max_size == 0. gtkrecentmanager.c 470.
N15, N16.
static void
action_handle_method (GtkAtSpiContext *self,
const char *method_name,
GVariant *parameters,
GDBusMethodInvocation *invocation,
const Action *actions,
int n_actions)
{
....
int idx = -1;
g_variant_get (parameters, "(i)", &idx);
const Action *action = &actions[idx];
if (idx >= 0 && idx < n_actions)
g_dbus_method_invocation_return_value (
invocation, g_variant_new ("(s)", action->name));
else
g_dbus_method_invocation_return_error (invocation,
G_IO_ERROR,
G_IO_ERROR_INVALID_ARGUMENT,
"Unknown action %d",
idx);
....
}
PVS-Studio: V781 [CWE-129] The value of the 'idx' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 71, 73. gtkatspiaction.c 71
idx :
const Action *action = &actions[idx];
, :
if (idx >= 0 && idx < n_actions)
, , , .
: V781 [CWE-129] The value of the 'idx' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 132, 134. gtkatspiaction.c 132
N17, N18.
static gboolean
parse_n_plus_b (GtkCssParser *parser,
int before,
int *a,
int *b)
{
const GtkCssToken *token;
token = gtk_css_parser_get_token (parser);
if (gtk_css_token_is_ident (token, "n"))
{
....
return parse_plus_b (parser, FALSE, b);
}
else if (gtk_css_token_is_ident (token, "n-"))
{
....
return parse_plus_b (parser, TRUE, b);
}
else if (gtk_css_token_is (token, GTK_CSS_TOKEN_IDENT) &&
string_has_number (token->string.string, "n-", b))
{
....
return TRUE;
}
else
{
*b = before;
*a = 0;
return TRUE;
}
gtk_css_parser_error_syntax (parser, "Not a valid an+b type");
return FALSE;
}
PVS-Studio: V779 [CWE-561] Unreachable code detected. It is possible that an error is present. gtkcssselector.c 1077
if-else-if-else… . , , .
: V779 [CWE-561] Unreachable code detected. It is possible that an error is present. gtktreemodelfilter.c 3289
N19, N20.
static void
gtk_paint_spinner (GtkStyleContext *context,
cairo_t *cr,
guint step,
int x,
int y,
int width,
int height)
{
GdkRGBA color;
guint num_steps;
double dx, dy;
....
dx = width / 2;
dy = height / 2;
....
cairo_move_to (cr,
dx + (radius - inset) * cos (i * G_PI / half),
dy + (radius - inset) * sin (i * G_PI / half));
cairo_line_to (cr,
dx + radius * cos (i * G_PI / half),
dy + radius * sin (i * G_PI / half));
....
}
PVS-Studio:
- V636 [CWE-682] The 'width / 2' expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. gtkcellrendererspinner.c 412
- V636 [CWE-682] The 'height / 2' expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. gtkcellrendererspinner.c 413
dx dy. , double. , :
dx = width / 2.0;
dy = height / 2.0;
, :
- V636 [CWE-682] The 'width / 2' expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. gtkswitch.c 255
- V636 [CWE-682] The 'width / 2' expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. gtkswitch.c 257
N21.
, . memset . , , , C C++, , , , . , , , .
, . , , , :
GTK 4? , free , .
, GTK 4 g_free. :
void
g_free (gpointer mem)
{
free (mem);
TRACE(GLIB_MEM_FREE((void*) mem));
}
g_free free? GLib 2.46 . :
It's important to match g_malloc() (and wrappers such as g_new()) with g_free(), g_slice_alloc() (and wrappers such as g_slice_new()) with g_slice_free(), plain malloc() with free(), and (if you're using C++) new with delete and new[] with delete[]. Otherwise bad things can happen, since these allocators may use different memory pools (and new/delete call constructors and destructors).
Since GLib 2.46 g_malloc() is hardcoded to always use the system malloc implementation.
, g_malloc malloc, free.
.
void overwrite_and_free (gpointer data)
{
char *password = (char *) data;
if (password != NULL)
{
memset (password, 0, strlen (password));
g_free (password);
}
}
PVS-Studio: V597 [CWE-14] The compiler could delete the 'memset' function call, which is used to flush 'password' object. The memset_s() function should be used to erase the private data. gtkprintbackendcups.c 848
, g_free. , , , . g_free overwrite_and_free, , memset .
.
PVS-Studio . -, IntelliJ IDEA, Rider, IncrediBuild, Jenkins, PlatformIO, Travis CI, GitLab CI/CD, CircleCI, TeamCity, Visual Studio . -, , , . , , , . , , , 10 , PVS-Studio Visual Studio. :). , .
, - . , - :). , , , SonarQube.
PVS-Studio. , , , , . , , . PVS-Studio , :). , , , ! : PVS-Studio (YouTube).
, : Andrey Karpov. Finding Typos in the GTK 4 Project by PVS-Studio.