Failed assertion in detail reader on incomplete packet read
We've had a few FreeRADIUS instances run out of space in /var due to unresponsive remote accounting servers. When this happens, a short packet can be written to a detail file, causing this assertion failure: ASSERT FAILED detail.c[573]: data->state == STATE_QUEUED when the VP-reading loop hits EOF and falls through to the alloc_packet label without having read a trailing newline indicating end of packet: while (fgets(buffer, sizeof(buffer), data->fp)) { [...] /* * We're reading VP's, and got a blank line. * Queue the packet. */ if ((data->state == STATE_READING) && (buffer[0] == '\n')) { data->state = STATE_QUEUED; break; } [...] } [...] alloc_packet: rad_assert(data->state == STATE_QUEUED); The writes in rlm_detail's do_detail() are unchecked, so failed writes allow a successful Accounting-Response to be returned to the NAS even though the detail is incomplete and won't be processed by the detail reader. It seems the best way around this is for do_detail() to keep track of the number of bytes it's written, ftruncate() the detail file on a short write, and return RLM_MODULE_FAIL. fprintf() would need to be replaced with separate sprintf() and fwrite() in order to keep track of the number of bytes written for the current packet. Does this course of action make sense? I can work on this, but it may be a few weeks before I have more time. john -- John Morrissey _o /\ ---- __o jwm@horde.net _-< \_ / \ ---- < \, www.horde.net/ __(_)/_(_)________/ \_______(_) /_(_)__
John Morrissey wrote:
When this happens, a short packet can be written to a detail file, causing this assertion failure:
ASSERT FAILED detail.c[573]: data->state == STATE_QUEUED
OK. That should be changed to a conditional check.
The writes in rlm_detail's do_detail() are unchecked, so failed writes allow a successful Accounting-Response to be returned to the NAS even though the detail is incomplete and won't be processed by the detail reader.
It seems the best way around this is for do_detail() to keep track of the number of bytes it's written, ftruncate() the detail file on a short write, and return RLM_MODULE_FAIL. fprintf() would need to be replaced with separate sprintf() and fwrite() in order to keep track of the number of bytes written for the current packet.
That should be fixed, too.
Does this course of action make sense? I can work on this, but it may be a few weeks before I have more time.
I'll see if I can fix the assert for 2.1.9. It should treat that condition as an EOF, and delete the file. Alan DeKok.
participants (2)
-
Alan DeKok -
John Morrissey