What would that be used for? Look: client_free() frees
client->options, client->file AND client. So even if
you null client->options, client does no longer exist
as it has been freed. We should never use client after
calling client_free().
On 03/15/13 01:09, Ted Unangst wrote:
> After reading your description, I was expecting the patch to include a line
> setting options to null after freeing it. Whatever else we do, shouldn't we
> do that too?
>
>
> On Fri, Mar 15, 2013 at 00:02, Maxime Villard wrote:
>> Hi,
>> there is a huge bug in the tftp daemon.
>>
>> -- /usr/src/usr.sbin/tftpd/tftpd.c --
>> In tftp_open() on line 870, the daemon checks for options
>> (OACK) and handle them through the oack() function. Then,
>> it frees and NULLs the variable client->options.
>>
>> oack() - l.1390 - does some stuff, and if an error happens,
>> client_free() is called with the client structure, which
>> frees this structure, including client->options, but does
>> not null it.
>>
>> So, when returning after an error, client->options is freed
>> twice, causing a double-free and a crash.
>>
>> I succeeded in making the server crash, by changing the
>> tsize and blksize options before transferring a small file
>> in localhost. In fact, if you look on line 1432, you'll see
>> that you just have to close the socket to make the server
>> crash, just after sending the OACK packet.
>>
>> Typically, I just have to do:
>> $ tftp localhost
>> tftp> tsize 2
>> Tsize option on.
>> tftp> blksize 10
>> tftp> get test
>> [...wait ~ 10 sec...]
>> tftpd in free(): error: chunk is already free 0x1a658ca65f40
>>
>> Here is a patch. I switched oack() to int, so that we can
>> handle errors and call nak() to alert the client before
>> closing the connection. nak() frees the client structure,
>> so all goes well.
>>
>> Ok/Comments?
>>
>>
>> Index: tftpd.c
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/tftpd/tftpd.c,v
>> retrieving revision 1.8
>> diff -u -r1.8 tftpd.c
>> --- tftpd.c 13 Jul 2012 02:31:46 -0000 1.8
>> +++ tftpd.c 15 Mar 2013 00:36:40 -0000
>> @@ -168,7 +168,7 @@
>> void tftp(struct tftp_client *, struct tftphdr *, size_t);
>> void tftp_open(struct tftp_client *, const char *);
>> void nak(struct tftp_client *, int);
>> -void oack(struct tftp_client *);
>> +int oack(struct tftp_client *);
>> void oack_done(int, short, void *);
>>
>> void sendfile(struct tftp_client *);
>> @@ -876,8 +876,8 @@
>> goto error;
>>
>> if (client->options) {
>> - oack(client);
>> -
>> + if (oack(client) == -1)
>> + goto error;
>> free(client->options);
>> client->options = NULL;
>> } else if (client->opcode == WRQ) {
>> @@ -1386,7 +1386,7 @@
>> /*
>> * Send an oack packet (option acknowledgement).
>> */
>> -void
>> +int
>> oack(struct tftp_client *client)
>> {
>> struct opt_client *options = client->options;
>> @@ -1436,10 +1436,10 @@
>> oack_done, client);
>>
>> event_add(&client->sev, &client->tv);
>> - return;
>> + return 0;
>>
>> error:
>> - client_free(client);
>> + return -1;
>> }
>>
>> int
>
>
