Open guijan opened 1 year ago
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:
scrotNoteNew()
also frees the old note structure). Instead of just doing it once after getopt
has finished.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.Unrelated to notes, but one more thing I've noticed was this:
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.
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.
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:
--- 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();
$ afl-clang-fast -g3 -fsanitize=address,undefined -fsanitize-undefined-trap-on-error -o scrot-fuzz src/*.c $(pkg-config --cflags --libs ./deps.pc)
$ 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
.
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
).
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#L288If you pass a font file multiple times to the note feature, it will assign the return value of
parseText()
tonote->font
withoutfree()
ingnote->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 toNULL
and thenfree()
it before callingparseText()
.Edit: here's an example that leaks memory: