Re: [PATCH] vcs-svn: Fix some compiler warnings

Jonathan Nieder [off-list ref] writes:

Thanks for tracking this down.  Interesting.  -Wtype-limits was split
out from the default set of warnings (!) in gcc 4.3 to address
<http://gcc.gnu.org/PR12963>, among other bugs (r124875, 2007-05-20).

[...]

quoted

--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c

@@ -300,7 +300,8 @@ void fast_export_blob_delta(uint32_t mode,
 				uint32_t len, struct line_buffer *input)
 {
 	long postimage_len;
-	if (len > maximum_signed_value_of_type(off_t))
+	uintmax_t delta_len = (uintmax_t) len;
+	if (delta_len > maximum_signed_value_of_type(off_t))
 		die("enormous delta");
 	postimage_len = apply_delta((off_t) len, input, old_data, old_mode);

Is there some less ugly way to write the condition "if this value is
not representable in this type"?

I guess I could live with something like the following (please don't
take the names too seriously):
...
What do you think?

I'd hold the branch in 'next' for now, until this gets resolved (one
possible resolution is to declare Ramsey's patch is good enough for now,
and do the follow-up later).

Junio C Hamano wrote:

I'd hold the branch in 'next' for now, until this gets resolved (one
possible resolution is to declare Ramsey's patch is good enough for now,
and do the follow-up later).

Sounds sensible.  How about the following?  Intended to replace
Ramsay's patch.  Not well tested yet.

By the way, wouldn't the (p->field < 0) test in grep.c line 330
trigger the same compiler bug?

Jonathan Nieder (2):
  vcs-svn: allow import of > 4GiB files
  vcs-svn: suppress a -Wtype-limits warning

Ramsay Allan Jones (1):
  vcs-svn: rename check_overflow arguments for clarity

 vcs-svn/fast_export.c    |   15 +++++++++------
 vcs-svn/fast_export.h    |    4 ++--
 vcs-svn/sliding_window.c |   10 +++++-----
 vcs-svn/svndump.c        |   21 +++++++++++++++------
 4 files changed, 31 insertions(+), 19 deletions(-)

From: Ramsay Jones <redacted>

Code using the argument names a and b just doesn't look right (not
sure why!).  Use more explicit names "offset" and "len" to make their
type and function clearer.

Signed-off-by: Jonathan Nieder <redacted>
---
Split out from Ramsay's patch.

 vcs-svn/sliding_window.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c
index 1bac7a4c..fafa4a63 100644
--- a/vcs-svn/sliding_window.c
+++ b/vcs-svn/sliding_window.c

@@ -31,15 +31,15 @@ static int read_to_fill_or_whine(struct line_buffer *file,
 	return 0;
 }
 
-static int check_overflow(off_t a, size_t b)
+static int check_overflow(off_t offset, size_t len)
 {
-	if (b > maximum_signed_value_of_type(off_t))
+	if (len > maximum_signed_value_of_type(off_t))
 		return error("unrepresentable length in delta: "
-				"%"PRIuMAX" > OFF_MAX", (uintmax_t) b);
+				"%"PRIuMAX" > OFF_MAX", (uintmax_t) len);
-	if (signed_add_overflows(a, (off_t) b))
+	if (signed_add_overflows(offset, (off_t) len))
 		return error("unrepresentable offset in delta: "
 				"%"PRIuMAX" + %"PRIuMAX" > OFF_MAX",
-				(uintmax_t) a, (uintmax_t) b);
+				(uintmax_t) offset, (uintmax_t) len);
 	return 0;
 }
 

-- 
1.7.9

There is no reason in principle that an svn-format dump would not be
able to represent a file whose length does not fit in a 32-bit
integer.  Use off_t consistently to represent file lengths (in place
of using uint32_t in some contexts) so we can handle that.

Most svn-fe code is already ready to do that without this patch and
passes values of type off_t around.  The type mismatch from stragglers
was noticed with gcc -Wtype-limits.

While at it, tighten the parsing of the Text-content-length field to
make sure it is a number and does not overflow, and tighten other
overflow checks as that value is passed around and manipulated.

Inspired-by: Ramsay Jones [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
---
 vcs-svn/fast_export.c |   15 +++++++++------
 vcs-svn/fast_export.h |    4 ++--
 vcs-svn/svndump.c     |   21 +++++++++++++++------
 3 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 19d7c34c..b823b851 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c

@@ -227,15 +227,18 @@ static long apply_delta(off_t len, struct line_buffer *input,
 	return ret;
 }
 
-void fast_export_data(uint32_t mode, uint32_t len, struct line_buffer *input)
+void fast_export_data(uint32_t mode, off_t len, struct line_buffer *input)
 {
+	assert(len >= 0);
 	if (mode == REPO_MODE_LNK) {
 		/* svn symlink blobs start with "link " */
+		if (len < 5)
+			die("invalid dump: symlink too short for \"link\" prefix");
 		len -= 5;
 		if (buffer_skip_bytes(input, 5) != 5)
 			die_short_read(input);
 	}
-	printf("data %"PRIu32"\n", len);
+	printf("data %"PRIuMAX"\n", (uintmax_t) len);
 	if (buffer_copy_bytes(input, len) != len)
 		die_short_read(input);
 	fputc('\n', stdout);

@@ -297,12 +300,12 @@ int fast_export_ls(const char *path, uint32_t *mode, struct strbuf *dataref)
 
 void fast_export_blob_delta(uint32_t mode,
 				uint32_t old_mode, const char *old_data,
-				uint32_t len, struct line_buffer *input)
+				off_t len, struct line_buffer *input)
 {
 	long postimage_len;
-	if (len > maximum_signed_value_of_type(off_t))
-		die("enormous delta");
-	postimage_len = apply_delta((off_t) len, input, old_data, old_mode);
+
+	assert(len >= 0);
+	postimage_len = apply_delta(len, input, old_data, old_mode);
 	if (mode == REPO_MODE_LNK) {
 		buffer_skip_bytes(&postimage, strlen("link "));
 		postimage_len -= strlen("link ");

diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index 43d05b65..aa629f54 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h

@@ -14,10 +14,10 @@ void fast_export_begin_commit(uint32_t revision, const char *author,
 			const struct strbuf *log, const char *uuid,
 			const char *url, unsigned long timestamp);
 void fast_export_end_commit(uint32_t revision);
-void fast_export_data(uint32_t mode, uint32_t len, struct line_buffer *input);
+void fast_export_data(uint32_t mode, off_t len, struct line_buffer *input);
 void fast_export_blob_delta(uint32_t mode,
 			uint32_t old_mode, const char *old_data,
-			uint32_t len, struct line_buffer *input);
+			off_t len, struct line_buffer *input);
 
 /* If there is no such file at that rev, returns -1, errno == ENOENT. */
 int fast_export_ls_rev(uint32_t rev, const char *path,

diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index ca63760f..644fdc71 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c

@@ -40,7 +40,8 @@
 static struct line_buffer input = LINE_BUFFER_INIT;
 
 static struct {
-	uint32_t action, propLength, textLength, srcRev, type;
+	uint32_t action, propLength, srcRev, type;
+	off_t text_length;
 	struct strbuf src, dst;
 	uint32_t text_delta, prop_delta;
 } node_ctx;

@@ -61,7 +62,7 @@ static void reset_node_ctx(char *fname)
 	node_ctx.type = 0;
 	node_ctx.action = NODEACT_UNKNOWN;
 	node_ctx.propLength = LENGTH_UNKNOWN;
-	node_ctx.textLength = LENGTH_UNKNOWN;
+	node_ctx.text_length = -1;
 	strbuf_reset(&node_ctx.src);
 	node_ctx.srcRev = 0;
 	strbuf_reset(&node_ctx.dst);

@@ -209,7 +210,7 @@ static void handle_node(void)
 {
 	const uint32_t type = node_ctx.type;
 	const int have_props = node_ctx.propLength != LENGTH_UNKNOWN;
-	const int have_text = node_ctx.textLength != LENGTH_UNKNOWN;
+	const int have_text = node_ctx.text_length != -1;
 	/*
 	 * Old text for this node:
 	 *  NULL	- directory or bug

@@ -291,12 +292,12 @@ static void handle_node(void)
 	}
 	if (!node_ctx.text_delta) {
 		fast_export_modify(node_ctx.dst.buf, node_ctx.type, "inline");
-		fast_export_data(node_ctx.type, node_ctx.textLength, &input);
+		fast_export_data(node_ctx.type, node_ctx.text_length, &input);
 		return;
 	}
 	fast_export_modify(node_ctx.dst.buf, node_ctx.type, "inline");
 	fast_export_blob_delta(node_ctx.type, old_mode, old_data,
-				node_ctx.textLength, &input);
+				node_ctx.text_length, &input);
 }
 
 static void begin_revision(void)

@@ -409,7 +410,15 @@ void svndump_read(const char *url)
 			break;
 		case sizeof("Text-content-length"):
 			if (!constcmp(t, "Text-content-length")) {
-				node_ctx.textLength = atoi(val);
+				char *end;
+				uintmax_t textlen;
+
+				textlen = strtoumax(val, &end, 10);
+				if (!isdigit(*val) || *end)
+					die("invalid dump: non-numeric length %s", val);
+				if (textlen > maximum_signed_value_of_type(off_t))
+					die("unrepresentable length in dump: %s", val);
+				node_ctx.text_length = (off_t) textlen;
 				break;
 			}
 			if (constcmp(t, "Prop-content-length"))

-- 
1.7.9

Hi

On Thu, Feb 2, 2012 at 4:59 PM, Jonathan Nieder [off-list ref] wrote:

From: Ramsay Jones <redacted>

Code using the argument names a and b just doesn't look right (not
sure why!).  Use more explicit names "offset" and "len" to make their
type and function clearer.

Well, it's still not clear. Given off_t a, size_t b, check that a+b
fits into type... which type?
"offset" and "length" don't imply that it's "type of offset" or maybe
"type of length".

On 32-bit architectures with 64-bit file offsets, gcc 4.3 and earlier
produce the following warning:

	    CC vcs-svn/sliding_window.o
	vcs-svn/sliding_window.c: In function `check_overflow':
	vcs-svn/sliding_window.c:36: warning: comparison is always false \
	    due to limited range of data type

The warning appears even when gcc is run without any warning flags
(this is gcc bug 12963).  In later versions the same warning can be
reproduced with -Wtype-limits, which is implied by -Wextra.

On 64-bit architectures it really is possible for a size_t not to be
representable as an off_t so the check this is warning about is not
actually redundant.  But even false positives are distracting.  Avoid
the warning by making the "len" argument to check_overflow a
uintmax_t; no functional change intended.

Reported-by: Ramsay Jones <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
That's the end of the series.  I hope it was entertaining.

Thoughts of all kinds welcome, as usual.

Jonathan

 vcs-svn/sliding_window.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c
index fafa4a63..2f4ae60f 100644
--- a/vcs-svn/sliding_window.c
+++ b/vcs-svn/sliding_window.c

@@ -31,15 +31,15 @@ static int read_to_fill_or_whine(struct line_buffer *file,
 	return 0;
 }
 
-static int check_overflow(off_t offset, size_t len)
+static int check_overflow(off_t offset, uintmax_t len)
 {
 	if (len > maximum_signed_value_of_type(off_t))
 		return error("unrepresentable length in delta: "
-				"%"PRIuMAX" > OFF_MAX", (uintmax_t) len);
+				"%"PRIuMAX" > OFF_MAX", len);
 	if (signed_add_overflows(offset, (off_t) len))
 		return error("unrepresentable offset in delta: "
 				"%"PRIuMAX" + %"PRIuMAX" > OFF_MAX",
-				(uintmax_t) offset, (uintmax_t) len);
+				(uintmax_t) offset, len);
 	return 0;
 }
 

-- 
1.7.9

Dmitry Ivankov wrote:

On Thu, Feb 2, 2012 at 4:59 PM, Jonathan Nieder [off-list ref] wrote:

quoted

From: Ramsay Jones <redacted>

Code using the argument names a and b just doesn't look right (not
sure why!).  Use more explicit names "offset" and "len" to make their
type and function clearer.

Well, it's still not clear. Given off_t a, size_t b, check that a+b
fits into type... which type?
"offset" and "length" don't imply that it's "type of offset" or maybe
"type of length".

Hmm... in vector arithmetic, position (i.e., file offset) + displacement
(i.e., size of chunk) = position (i.e., new file offset).  Any ideas
for making this clearer?

On Thu, Feb 2, 2012 at 10:16 PM, Jonathan Nieder [off-list ref] wrote:

Dmitry Ivankov wrote:

quoted

On Thu, Feb 2, 2012 at 4:59 PM, Jonathan Nieder [off-list ref] wrote:

quoted

quoted

From: Ramsay Jones <redacted>

Code using the argument names a and b just doesn't look right (not
sure why!).  Use more explicit names "offset" and "len" to make their
type and function clearer.

Well, it's still not clear. Given off_t a, size_t b, check that a+b
fits into type... which type?
"offset" and "length" don't imply that it's "type of offset" or maybe
"type of length".

Hmm... in vector arithmetic, position (i.e., file offset) + displacement
(i.e., size of chunk) = position (i.e., new file offset).  Any ideas
for making this clearer?

Maybe rename to check_offset_overflow to make it explicit?
--
David Barr

David Barr wrote:

Maybe rename to check_offset_overflow to make it explicit?

Ok, here's the patch I'll squash in. ;-)

diff --git i/vcs-svn/sliding_window.c w/vcs-svn/sliding_window.c
index 2f4ae60f..ec2707c9 100644
--- i/vcs-svn/sliding_window.c
+++ w/vcs-svn/sliding_window.c

@@ -31,7 +31,7 @@ static int read_to_fill_or_whine(struct line_buffer *file,
 	return 0;
 }
 
-static int check_overflow(off_t offset, uintmax_t len)
+static int check_offset_overflow(off_t offset, uintmax_t len)
 {
 	if (len > maximum_signed_value_of_type(off_t))
 		return error("unrepresentable length in delta: "

@@ -48,9 +48,9 @@ int move_window(struct sliding_view *view, off_t off, size_t width)
 	off_t file_offset;
 	assert(view);
 	assert(view->width <= view->buf.len);
-	assert(!check_overflow(view->off, view->buf.len));
+	assert(!check_offset_overflow(view->off, view->buf.len));
 
-	if (check_overflow(off, width))
+	if (check_offset_overflow(off, width))
 		return -1;
 	if (off < view->off || off + width < view->off + view->width)
 		return error("invalid delta: window slides left");

Jonathan Nieder wrote:

---
That's the end of the series.  I hope it was entertaining.

Thoughts of all kinds welcome, as usual.

Yes, this is a much better approach! Thanks!

I've only compile tested (on cygwin and mingw) so far, but
I don't expect any problems ...

So, please disregard my earlier v2 patch. [If it's not already
obvious, I often don't read the list every day and I missed
all of the discussion which resulted in this series (while, at
the same time, writing testing and sending the v2 patch!).]

ATB,
Ramsay Jones