likely bug: schedule_reset_timer not initialised
DESCRIPTION:
It appears that manager->schedule_reset_timer
is likely to be used
before it has been initialised. If so, then this is a coding error,
because taking decisions based on a semi-random unsigned integer
causes unpredictable behaviour.
VERSION: e7790f94 tag: refs/tags/0.4.6
CODE ANALYSIS:
In /usr/include/glib-2.0/glib/gtypes.h
, guint
is declared as a uint, with
nothing special about initialisation:
61:typedef unsigned int guint;
In manager.h
, it does not appear to be declared static or external :
$ grep -E -C3 "schedule_reset_timer" src/manager.h
struct EG25Manager {
GMainLoop *loop;
guint complete_reset_timer;
guint schedule_reset_timer;
gboolean use_libusb;
guint usb_vid;
guint usb_pid;
Definitions of struct EG25manager *manager
in the .c
files do not appear to
initialise schedule_reset_timer
to zero (or to any other value).
The only line where manager->schedule_reset_timer
is given a value
is 54 in udev.c
, which is after its value has been checked in line 37 and
possibly lines 48 and 49 of udev.c
.
On line 156 of manager.c
, the value of manager->schedule_reset_timer
is checked within modem_reset()
, which can be called by line 54 of udev.c
(which is already too late, since lines 37 and maybe 48 and 49 have already
made decisions based on the value); and on line 39 of suspend.c
in
modem_resume()
- which is not clearly labelled as an initialisation routine.
$ grep -C3 -n schedule_reset_timer src/*.[ch]
src/manager.c-153- int fd, ret, len;
src/manager.c-154-
src/manager.c-155- /* reset sequence started, cannot be canceled anymore */
src/manager.c:156: if (manager->schedule_reset_timer) {
src/manager.c:157: g_source_remove(manager->schedule_reset_timer);
src/manager.c:158: manager->schedule_reset_timer = 0;
src/manager.c-159- }
src/manager.c-160-
src/manager.c-161- if (manager->modem_recovery_timer) {
--
src/manager.h-72-struct EG25Manager {
src/manager.h-73- GMainLoop *loop;
src/manager.h-74- guint complete_reset_timer;
src/manager.h:75: guint schedule_reset_timer;
src/manager.h-76- gboolean use_libusb;
src/manager.h-77- guint usb_vid;
src/manager.h-78- guint usb_pid;
--
src/udev.c-34- /*
src/udev.c-35- * Modem is probably executing a FW upgrade, make sure we don't interrupt it
src/udev.c-36- */
src/udev.c:37: if (manager->schedule_reset_timer != 0) {
src/udev.c-38- g_message("Modem re-appeared with different VID/PID, cancel reset.");
src/udev.c:39: g_source_remove(manager->schedule_reset_timer);
src/udev.c:40: manager->schedule_reset_timer = 0;
src/udev.c-41- }
src/udev.c-42- manager->modem_state = EG25_STATE_UPDATING;
src/udev.c-43- }
--
src/udev.c-46- manager->modem_state == EG25_STATE_UPDATING ||
src/udev.c-47- manager->modem_state == EG25_STATE_RESETTING ||
src/udev.c-48- manager->complete_reset_timer != 0 ||
src/udev.c:49: manager->schedule_reset_timer != 0) {
src/udev.c-50- return;
src/udev.c-51- }
src/udev.c-52-
src/udev.c-53- g_message("Lost modem, resetting...");
src/udev.c:54: manager->schedule_reset_timer = g_timeout_add_seconds(3, G_SOURCE_FUNC(modem_reset), manager);
src/udev.c-55-}
src/udev.c-56-
src/udev.c-57-void udev_init (struct EG25Manager *manager, toml_table_t *config[])
DISCUSSION:
Either I missed something in the code that initialises schedule_reset_timer
(which is quite possible :)) or this is a bug.
There may be other uninitialised values in the code. Use of valgrind
on a "fake" simulated modem would be one way of systematically checking
for uninitialised variables and/or memory leaks.
I found this likely bug by chance in the context of discussion
concerning pinephone_modem_sdk
issue #213:
https://github.com/the-modem-distro/pinephone_modem_sdk/issues/213
POSSIBLY RELATED: