note.c: memory leak

[guijan]

trafficstars

These lines of code leak: https://github.com/resurrecting-open-source-projects/scrot/blob/a1fbb65372a2a7ce49ddd7b1041ca5f2e824a183/src/note.c#L122-L123 parseText() allocates a string and returns it: https://github.com/resurrecting-open-source-projects/scrot/blob/a1fbb65372a2a7ce49ddd7b1041ca5f2e824a183/src/note.c#L288

If you pass a font file multiple times to the note feature, it will assign the return value of parseText() to note->font without free()ing note->font first. This leaks filenames as many times as font files are repeatedly specified.

I'd personally just remove the note feature entirely, but the fix is to make sure note->font is initialized to NULL and then free() it before calling parseText().

Edit: here's an example that leaks memory:

$ scrot -n "-f '/usr/share/fonts/TTF/DroidSans-Bold/40' -f '/usr/share/fonts/TTF/DroidSans-Bold/40' -x 10 -y 20 -c 255,0,0,255 -t 'Hi'"

[N-R-K]

I was glancing at the note code (investigating if those strdups are needed or not) and I'm left even more confused and with many more questions.

@guijan Since you have been involved in scrot development for longer, I figured I'd ask you first in case you knew already, before I put in the effort:

• If we only accept a single note, why are we free-ing and allocating a new note everytime? (scrotNoteNew() also frees the old note structure). Instead of just doing it once after getopt has finished.

https://github.com/resurrecting-open-source-projects/scrot/blob/fafced405301d1815025f1b6194aeca1d25173af/src/options.c#L591-L602

• Why is note allocated? The size is known at compile time. It should just be a global var. There shouldn't be any allocation/deallocation necessary at all.

https://github.com/resurrecting-open-source-projects/scrot/blob/fafced405301d1815025f1b6194aeca1d25173af/src/note.c#L105-L107

---

Unrelated to notes, but one more thing I've noticed was this:

https://github.com/resurrecting-open-source-projects/scrot/blob/fafced405301d1815025f1b6194aeca1d25173af/src/scrot_selection.c#L57-L61

What's the point of this function instead of just using a global var? It looks like it's trying to do some pseudo-OOP stuff but I don't see the point, there will only ever be a single selection in the program.

[guijan]

scrot has a lot of code that wasn't very well thought out, that's why we can rewrite it shorter, more portable, faster, etc, so often. There isn't really a reason for all the stuff you pointed out. The users of selectionGet() also look really odd, we can probably redo all of that in a fraction of the lines of code and very likely better assembly too.

[N-R-K]

Buffer overflow on the following argument to --note:

$ gcc -g3 -fsanitize=address,undefined -fsanitize-undefined-trap-on-error -o scrot src/*.c $(pkg-config --cflags --libs ./scrot-deps.pc)
$ ./scrot --note '-y0t -'
=================================================================
==1457==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000000037 at pc 0x0000004041fc bp 0x7ffe2a628150 sp 0x7ffe2a628148
READ of size 1 at 0x602000000037 thread T0
    #0 0x4041fb in nextSpace src/note.c:87
    #1 0x40476c in scrotNoteNew src/note.c:118
    #2 0x408a80 in optionsParseNote src/options.c:615
    #3 0x40791d in optionsParse src/options.c:433
    #4 0x408c32 in main src/scrot.c:110

Found via fuzzing. To fuzz yourself:

1. install afl++
2. Apply the following patch:

--- a/src/scrot.c
+++ b/src/scrot.c
@@ -90,6 +90,7 @@ static Imlib_Image stalkImageConcat(ScrotList *, const enum Direction);
 static int findWindowManagerFrame(Window *const, int *const);
 static Imlib_Image scrotGrabWindowById(Window const window);
 
+__AFL_FUZZ_INIT();
 
 int main(int argc, char *argv[])
 {
@@ -107,9 +108,23 @@ int main(int argc, char *argv[])
 
     atexit(uninitXAndImlib);
 
-    optionsParse(argc, argv);
+#ifdef __AFL_HAVE_MANUAL_CONTROL
+    __AFL_INIT();
+#endif
 
     initXAndImlib(opt.display, 0);
+    image = scrotGrabShot();
+
+    unsigned char *afl_buf = __AFL_FUZZ_TESTCASE_BUF;
+    while (__AFL_LOOP(10000)) {
+        int len = __AFL_FUZZ_TESTCASE_LEN;
+        char *buf = malloc(len + 1);
+        *(char *)mempcpy(buf, afl_buf, len) = 0;
+        optionsParseNote(buf);
+        scrotNoteDraw(image);
+        free(buf);
+    }
+    return 0;
 
     if (opt.selection.mode & SELECTION_MODE_ANY)
         image = scrotSelectionSelectMode();

3. Compile as the following:

$ afl-clang-fast -g3 -fsanitize=address,undefined -fsanitize-undefined-trap-on-error -o scrot-fuzz src/*.c $(pkg-config --cflags --libs ./deps.pc)

4. Create input, output dir and then run the fuzzer:

$ mkdir input output
$ printf "%s\n" "-f '/usr/share/fonts/test/Pragmasevka-regular.ttf' -x 10 -y 20 -c 255,0,0,255 -t 'Hi'" > input/sample
$ afl-fuzz -i input -o output ./scrot-fuzz

Crashes will be saved at output/default/crashes.

[N-R-K]

Since literally no one objected to removing notes in #195, I'm thinking we can just depreciate it and remove it along with --script. This will also remove dependency on imlib2[text] (as well as imlib2[filters] due to removing --script).