Bug 5335 - potential infinite loop in x_fwrite
potential infinite loop in x_fwrite
Status: NEW
Product: Samba 3.0
Classification: Unclassified
Component: File Services
3.0.28a
All All
: P3 normal
: none
Assigned To: Volker Lendecke
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-18 05:43 UTC by Louis Mussat
Modified: 2010-04-08 11:32 UTC (History)
1 user (show)

See Also:
vl: review-


Attachments
non-looping draft implementation of x_fwrite (1.41 KB, text/plain)
2008-03-18 06:08 UTC, Louis Mussat
no flags Details
Patch (1.06 KB, patch)
2008-04-04 19:35 UTC, Jeremy Allison
no flags Details
git am format patch (1.37 KB, patch)
2010-04-08 06:57 UTC, Volker Lendecke
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Louis Mussat 2008-03-18 05:43:00 UTC
On line 193 of "lib/xfile.c" there is a call to x_fflush which may start an infinite looping if something goes wrong during the flush and before f->bufused is updated.

Quick fix: test the return value of x_fflush and act accordingly

Better fix: replace the loop ; I've drafted some code that I can mail you if you're interested

Note: this may explain the loop reported in comment #14 of bug  #3817

Note: I've assigned this bug to 'ntlm_auth' because in ntlm_auth.c there is a lot of indirect calls to x_fwrite (through x_fprintf). But of course any other component of samba may be impacted.
Comment 1 Louis Mussat 2008-03-18 06:08:01 UTC
Created attachment 3187 [details]
non-looping draft implementation of x_fwrite

This is draft code I've written to implement x_fwrite with no loop. Please note that this code has not been tested, not even compiled.

Note also that this implementation better emulates fwrite in that upon error it returns zero or a positive number smaller than nmemb. Besides, the prototype of x_fwrite forbids the returning of -1.
Comment 2 Louis Mussat 2008-03-18 09:46:59 UTC
Comment on attachment 3187 [details]
non-looping draft implementation of x_fwrite

><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">/* simulate fwrite() */
>size_t x_fwrite(const void *p, size_t size, size_t nmemb, XFILE *f)
>{
> 	ssize_t ret;
>	size_t total=size*nmemb; // we should check for overflow...
>
>	if (!total) return 0; // this is what  fwrite  must do according to the standard
>
>	/* we might be writing unbuffered */
>	if (f->buftype == X_IONBF || 
>	    (!f->buf && !x_allocate_buffer(f))) {
>		ret = write(f->fd, p, total);
>		if (ret < 0) return 0;
>		return ret/size;
>	} 
>
>	if (total <= f->bufsize - f->bufused) {
>		memcpy(f->buf + f->bufused, (const char *)p, total);
>		f->bufused += total;
>	} else {
>		size_t buffered = total % f->bufsize; // 0 <= buffered <= total
>		if (x_fflush(f) < 0) return 0; // flush old content before writing more bytes
>		ret = write(f->fd, (const char *)p, total-buffered); // no-op when (buffered == total); write all when (buffered == 0)
>		if (ret < 0) return 0;
>		if (ret < total-buffered) return ret/size;
>		memcpy(f->buf, total-buffered+(const char *)p, buffered); // no-op when (buffered == 0); copy all when (buffered == total)
>		f->bufused = buffered;
>		}
>	}
>	/* when line buffered we need to flush at the last linefeed. This can
>	   flush a bit more than necessary, but that is harmless */
>	if (f->buftype == X_IOLBF && f->bufused) {
>		int i;
>		for (i=f->bufused-1; i>=0; i--) {
>			if (*(i+f->buf) == '\n') {
>				if (x_fflush(f) < 0) return (total-f->bufused)/size;
>				break;
>			}
>		}
>	}
>
>	return nmemb;
>}
Comment 3 Louis Mussat 2008-03-18 09:50:25 UTC
Comment on attachment 3187 [details]
non-looping draft implementation of x_fwrite

In the code attached to comment#2

>		if (ret < total-buffered) return written/size;

should be

		if (ret < total-buffered) return ret/size;
Comment 4 Volker Lendecke 2008-03-18 11:19:31 UTC
It would be best if you added a patch from "git
format-patch". This way you would also get all the credit in
the author information.

Thanks,

Volker
Comment 5 Jeremy Allison 2008-04-04 19:35:17 UTC
Created attachment 3243 [details]
Patch

I think this is a simpler patch for the problem. Not as clean as a rewrite, but easier to get into the current codebase. Let me know what you think.
Jeremy.
Comment 6 Volker Lendecke 2010-04-08 06:57:10 UTC
The patch is not in git am format. I will upload a correct patch next.

Volker
Comment 7 Volker Lendecke 2010-04-08 06:57:55 UTC
Created attachment 5612 [details]
git am format patch

Patch in git am format against master