i3 / i3status

Generates status bar to use with i3bar, dzen2 or xmobar
BSD 3-Clause "New" or "Revised" License
602 stars 254 forks source link

slurp_battery_info: Fix reading uninitialised memory #531

Closed nh2 closed 1 month ago

nh2 commented 3 months ago

Looks like C is hard even for the most basic file reading tasks (more in #530 C file reading is implemented incorrectly); invariants of who initialises what memory how far are hard to maintain, and all C projects should really use valgrind in CI:

This PR fixes the for (walk = buf, ... loop reading all of buf even though buf is null-terminated string (an only partly initialised char array).

(nix-shell) niklas@t25 ~/src/i3status/build (git)-[remotes/origin/HEAD] % git rev-parse HEAD                                    
200fef9e0d3663835b04e18ad067d9656b75b9cd

(nix-shell) niklas@t25 ~/src/i3status/build (git)-[remotes/origin/HEAD] % valgrind ./i3status -c ../etc/i3status.conf --run-once
==41607== Memcheck, a memory error detector
==41607== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==41607== Using Valgrind-3.22.0 and LibVEX; rerun with -h for copyright info
==41607== Command: ./i3status -c ../etc/i3status.conf --run-once
==41607== 
i3status: trying to auto-detect output_format setting
i3status: auto-detected "term"
==41607== Conditional jump or move depends on uninitialised value(s)
==41607==    at 0x40F15A: slurp_battery_info (print_battery_info.c:164)
==41607==    by 0x40FA07: slurp_all_batteries (print_battery_info.c:558)
==41607==    by 0x40FCA6: print_battery_info (print_battery_info.c:612)
==41607==    by 0x409CA2: main (i3status.c:753)
==41607== 
==41607== Conditional jump or move depends on uninitialised value(s)
==41607==    at 0x40F17F: slurp_battery_info (print_battery_info.c:169)
==41607==    by 0x40FA07: slurp_all_batteries (print_battery_info.c:558)
==41607==    by 0x40FCA6: print_battery_info (print_battery_info.c:612)
==41607==    by 0x409CA2: main (i3status.c:753)
==41607== 
no IPv6 | W: ( 63% at Mywifi) 192.168.1.100 | E: down | BAT 12.65% 01:00 | 8.0 GiB | 0.91 | 14.3 GiB | 47.6 GiB | 2024-07-21 00:33:34

Printing where the unitilialized read occurs:

diff --git a/src/print_battery_info.c b/src/print_battery_info.c
index 8864978..819b403 100644
--- a/src/print_battery_info.c
+++ b/src/print_battery_info.c
@@ -161,6 +161,7 @@ static bool slurp_battery_info(battery_info_ctx_t *ctx, struct battery_info *bat
     }

     for (walk = buf, last = buf; (walk - buf) < 1024; walk++) {
+        printf("walk offset %d\n", (walk - buf));
         if (*walk == '\n') {
             last = walk + 1;
             continue;
walk offset 524
==44018== Conditional jump or move depends on uninitialised value(s)
==44018==    at 0x40F182: slurp_battery_info (print_battery_info.c:165)
==44018==    by 0x40FA2F: slurp_all_batteries (print_battery_info.c:559)
==44018==    by 0x40FCCE: print_battery_info (print_battery_info.c:613)
==44018==    by 0x409CA2: main (i3status.c:753)
==44018== 
==44018== Conditional jump or move depends on uninitialised value(s)
==44018==    at 0x40F1A7: slurp_battery_info (print_battery_info.c:170)
==44018==    by 0x40FA2F: slurp_all_batteries (print_battery_info.c:559)
==44018==    by 0x40FCCE: print_battery_info (print_battery_info.c:613)
==44018==    by 0x409CA2: main (i3status.c:753)
==44018== 

That's the length of my batpath file:

wc --bytes /sys/class/power_supply/BAT0/uevent
523 /sys/class/power_supply/BAT0/uevent
nh2 commented 1 month ago

I updated the PR to address the comments