rrthomas / recode

Charset converter tool and library
GNU General Public License v3.0
130 stars 12 forks source link

double free in recode_file_to_file #48

Closed remicollet closed 1 year ago

remicollet commented 1 year ago

Using recode_file_to_file a double free occurs because of closing the provided file handle

I think recode_file_to_file should not close file handles given by the caller This is caller work (open + close)

Regression introduce in 2.7.13 in 951bdbc2d847e583e4bc62a997a00058bb75d541

Reverting this commit fixes the issue.

Found when running PHP recode extension.

(gdb) bt
#0  0x00007ffff7621b94 in __pthread_kill_implementation () from /lib64/libc.so.6
#1  0x00007ffff75d0aee in raise () from /lib64/libc.so.6
#2  0x00007ffff75b987f in abort () from /lib64/libc.so.6
#3  0x00007ffff75ba60f in __libc_message.cold () from /lib64/libc.so.6
#4  0x00007ffff762bac5 in malloc_printerr () from /lib64/libc.so.6
#5  0x00007ffff762dea5 in _int_free () from /lib64/libc.so.6
#6  0x00007ffff76304ee in free () from /lib64/libc.so.6
#7  0x00007ffff7609cd7 in fclose@@GLIBC_2.2.5 () from /lib64/libc.so.6
#8  0x00005555557aef19 in php_stdiop_close ()
#9  0x00005555557a9a07 in _php_stream_free ()
#10 0x00005555557339e3 in zif_fclose ()
#11 0x000055555586710a in execute_ex ()
#12 0x000055555586e6a0 in zend_execute ()
#13 0x00005555557fbc4b in zend_execute_scripts ()
#14 0x000055555579470a in php_execute_script ()
#15 0x00005555558e8306 in do_cli ()
#16 0x000055555563fac5 in main ()
(gdb) quit

Segfault happens when PHP closes the stream it has open before calling recode_file_to_file

remicollet commented 1 year ago

Perhaps the proper fix is to close the input file only if it was open by the function ?

-              if (subtask->input.file)
+              if (subtask->input.file && subtask->input.name && subtask->input.name[0])

Or to add a "openbyme" flag in the task struct to check if it need to be closed

Notice: when name is empty, stdin is used, and closing it may have strange unexpected results.

remicollet commented 1 year ago

FYI also reported to Fedora package owner as bug #2170818

rrthomas commented 1 year ago

Thanks @remicollet for the report, and for the fix, which looks good to me, and I've committed.

rrthomas commented 1 year ago

I will make another release shortly.

remicollet commented 1 year ago

Thanks. I confirm 2.7.13 (in Fedora 38) + https://github.com/rrthomas/recode/commit/80516f601ce5f1cee44848615dffe4252f2d205f properly fix this issue.