note.c: memory leak

[guijan]

These lines of code leak:

scrot/src/note.c

Lines 122 to 123 in a1fbb65

	case 'f':
	note->font = parseText(&token, end);

parseText() allocates a string and returns it:

scrot/src/note.c

Line 288 in a1fbb65

return strndup(begin, length);

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.

scrot/src/options.c

Lines 591 to 602 in fafced4

	if (opt.note)
	free(opt.note);
	opt.note = estrdup(optarg);
	if (!opt.note)
	return;
	if (opt.note[0] == '\0')
	errx(EXIT_FAILURE, "Required arguments for --note.");
	scrotNoteNew(opt.note);

• 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.

scrot/src/note.c

Lines 105 to 107 in fafced4

	scrotNoteFree();
	note = ecalloc(1, sizeof(*note));

---

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

scrot/src/scrot_selection.c

Lines 57 to 61 in fafced4

	struct Selection **selectionGet(void)
	{
	static struct Selection *sel = NULL;
	return &sel;
	}

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).

[ifohancroft]

Isn't this now resolved, with the deprecation of --note?
Also, shouldn't we remove note.h and note.c?

[N-R-K]

Isn't this now resolved, with the deprecation of --note?

Not exactly. The issue is still there. We just don't care about fixing it since the feature will be removed.

Also, shouldn't we remove note.h and note.c?

It'll be removed on the next major version. There's no specific date for it yet. We're waiting until Imlib2 v1.11 is available in debian stable (see #244 for discussion regarding that).

[ifohancroft]

Isn't this now resolved, with the deprecation of --note?

Not exactly. The issue is still there. We just don't care about fixing it since the feature will be removed.

That's what I meant. Not that the bug is fixed, but that the issue (as in this GitHub Issue) is technically resolved since the feature would be removed.

Also, shouldn't we remove note.h and note.c?

It'll be removed on the next major version. There's no specific date for it yet. We're waiting until Imlib2 v1.11 is available in debian stable (see #244 for discussion regarding that).

Got it. Thank you!