diff mbox series

[RFC,babeltrace-1.5] Fix: report truncated files while reading (v2)

Message ID 20180206204019.10769-1-mathieu.desnoyers@efficios.com
State Accepted, archived
Delegated to: Jérémie Galarneau
Headers show
Series [RFC,babeltrace-1.5] Fix: report truncated files while reading (v2) | expand

Commit Message

Mathieu Desnoyers Feb. 6, 2018, 8:40 p.m. UTC
When index files are available, this ensures we report the issue
properly before mapping a truncated packet rather than hitting a
SIGBUS.

For traces without index files, remove the check that was done at
index creation, and use the new check done when mapping the packet
instead. This ensures babeltrace can print stream data prior to the
truncated packet for this stream.

TODO: What is _not_ done in this patch is changing handling of truncated
packets: this should be treated as a warning rather than an error, so
processing of other streams continue.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
CC: Jonathan Rajotte-Julien <jonathan.rajotte-julien at efficios.com>
CC: Jérémie Galarneau <jeremie.galarneau at efficios.com>
---
 formats/ctf/ctf.c              | 44 +++++++++++++++++++-----------------------
 include/babeltrace/ctf/types.h |  1 +
 2 files changed, 21 insertions(+), 24 deletions(-)

Comments

Jérémie Galarneau Feb. 20, 2018, 8:25 p.m. UTC | #1
Merged in master, stable-1.5, and stable-1.4.

Thanks!
Jérémie

On Tue, Feb 06, 2018 at 03:40:19PM -0500, Mathieu Desnoyers wrote:
> When index files are available, this ensures we report the issue
> properly before mapping a truncated packet rather than hitting a
> SIGBUS.
> 
> For traces without index files, remove the check that was done at
> index creation, and use the new check done when mapping the packet
> instead. This ensures babeltrace can print stream data prior to the
> truncated packet for this stream.
> 
> TODO: What is _not_ done in this patch is changing handling of truncated
> packets: this should be treated as a warning rather than an error, so
> processing of other streams continue.
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
> CC: Jonathan Rajotte-Julien <jonathan.rajotte-julien at efficios.com>
> CC: Jérémie Galarneau <jeremie.galarneau at efficios.com>
> ---
>  formats/ctf/ctf.c              | 44 +++++++++++++++++++-----------------------
>  include/babeltrace/ctf/types.h |  1 +
>  2 files changed, 21 insertions(+), 24 deletions(-)
> 
> diff --git a/formats/ctf/ctf.c b/formats/ctf/ctf.c
> index ab98b288..b74cddd7 100644
> --- a/formats/ctf/ctf.c
> +++ b/formats/ctf/ctf.c
> @@ -1180,6 +1180,14 @@ void ctf_packet_seek(struct bt_stream_pos *stream_pos, size_t index, int whence)
>  				goto end;
>  			}
>  		}
> +
> +		if (packet_index->packet_size > ((uint64_t) pos->file_length - packet_index->offset) * CHAR_BIT) {
> +			fprintf(stderr, "[error] Packet size (%" PRIu64 " bits) is larger than remaining file size (%" PRIu64 " bits).\n",
> +				packet_index->packet_size, ((uint64_t) pos->file_length - packet_index->offset) * CHAR_BIT);
> +			ret = -1;
> +			goto end;
> +		}
> +
>  		pos->content_size = packet_index->content_size;
>  		pos->packet_size = packet_index->packet_size;
>  		pos->mmap_offset = packet_index->offset;
> @@ -1708,8 +1716,7 @@ int stream_assign_class(struct ctf_trace *td,
>  static
>  int create_stream_one_packet_index(struct ctf_stream_pos *pos,
>  			struct ctf_trace *td,
> -			struct ctf_file_stream *file_stream,
> -			size_t filesize)
> +			struct ctf_file_stream *file_stream)
>  {
>  	struct packet_index packet_index;
>  	uint64_t stream_id = 0;
> @@ -1724,8 +1731,8 @@ begin:
>  		first_packet = 1;
>  	}
>  
> -	if (filesize - pos->mmap_offset < (packet_map_len >> LOG2_CHAR_BIT)) {
> -		packet_map_len = (filesize - pos->mmap_offset) << LOG2_CHAR_BIT;
> +	if (pos->file_length - pos->mmap_offset < (packet_map_len >> LOG2_CHAR_BIT)) {
> +		packet_map_len = (pos->file_length - pos->mmap_offset) << LOG2_CHAR_BIT;
>  	}
>  
>  	if (pos->base_mma) {
> @@ -1843,7 +1850,7 @@ begin:
>  			packet_index.packet_size = bt_get_unsigned_int(field);
>  		} else {
>  			/* Use file size for packet size */
> -			packet_index.packet_size = filesize * CHAR_BIT;
> +			packet_index.packet_size = pos->file_length * CHAR_BIT;
>  		}
>  
>  		/* read content size from header */
> @@ -1855,7 +1862,7 @@ begin:
>  			packet_index.content_size = bt_get_unsigned_int(field);
>  		} else {
>  			/* Use packet size if non-zero, else file size */
> -			packet_index.content_size = packet_index.packet_size ? : filesize * CHAR_BIT;
> +			packet_index.content_size = packet_index.packet_size ? : pos->file_length * CHAR_BIT;
>  		}
>  
>  		/* read timestamp begin from header */
> @@ -1912,9 +1919,9 @@ begin:
>  		}
>  	} else {
>  		/* Use file size for packet size */
> -		packet_index.packet_size = filesize * CHAR_BIT;
> +		packet_index.packet_size = pos->file_length * CHAR_BIT;
>  		/* Use packet size if non-zero, else file size */
> -		packet_index.content_size = packet_index.packet_size ? : filesize * CHAR_BIT;
> +		packet_index.content_size = packet_index.packet_size ? : pos->file_length * CHAR_BIT;
>  	}
>  
>  	/* Validate content size and packet size values */
> @@ -1924,12 +1931,6 @@ begin:
>  		return -EINVAL;
>  	}
>  
> -	if (packet_index.packet_size > ((uint64_t) filesize - packet_index.offset) * CHAR_BIT) {
> -		fprintf(stderr, "[error] Packet size (%" PRIu64 " bits) is larger than remaining file size (%" PRIu64 " bits).\n",
> -			packet_index.packet_size, ((uint64_t) filesize - packet_index.offset) * CHAR_BIT);
> -		return -EINVAL;
> -	}
> -
>  	if (packet_index.content_size < pos->offset) {
>  		fprintf(stderr, "[error] Invalid CTF stream: content size is smaller than packet headers.\n");
>  		return -EINVAL;
> @@ -1952,7 +1953,7 @@ begin:
>  
>  	/* Retry with larger mapping */
>  retry:
> -	if (packet_map_len == ((filesize - pos->mmap_offset) << LOG2_CHAR_BIT)) {
> +	if (packet_map_len == ((pos->file_length - pos->mmap_offset) << LOG2_CHAR_BIT)) {
>  		/*
>  		 * Reached EOF, but still expecting header/context data.
>  		 */
> @@ -1975,17 +1976,12 @@ int create_stream_packet_index(struct ctf_trace *td,
>  			struct ctf_file_stream *file_stream)
>  {
>  	struct ctf_stream_pos *pos;
> -	struct stat filestats;
>  	int ret;
>  
>  	pos = &file_stream->pos;
>  
> -	ret = fstat(pos->fd, &filestats);
> -	if (ret < 0)
> -		return ret;
> -
>  	/* Deal with empty files */
> -	if (!filestats.st_size) {
> +	if (!pos->file_length) {
>  		if (file_stream->parent.trace_packet_header
>  				|| file_stream->parent.stream_packet_context) {
>  			/*
> @@ -2008,9 +2004,8 @@ int create_stream_packet_index(struct ctf_trace *td,
>  		}
>  	}
>  
> -	for (pos->mmap_offset = 0; pos->mmap_offset < filestats.st_size; ) {
> -		ret = create_stream_one_packet_index(pos, td, file_stream,
> -			filestats.st_size);
> +	for (pos->mmap_offset = 0; pos->mmap_offset < pos->file_length; ) {
> +		ret = create_stream_one_packet_index(pos, td, file_stream);
>  		if (ret)
>  			return ret;
>  	}
> @@ -2195,6 +2190,7 @@ int ctf_open_file_stream_read(struct ctf_trace *td, const char *path, int flags,
>  	file_stream->pos.last_offset = LAST_OFFSET_POISON;
>  	file_stream->pos.fd = -1;
>  	file_stream->pos.index_fp = NULL;
> +	file_stream->pos.file_length = statbuf.st_size;
>  
>  	strncpy(file_stream->parent.path, path, PATH_MAX);
>  	file_stream->parent.path[PATH_MAX - 1] = '\0';
> diff --git a/include/babeltrace/ctf/types.h b/include/babeltrace/ctf/types.h
> index 59ce9981..0bc003c8 100644
> --- a/include/babeltrace/ctf/types.h
> +++ b/include/babeltrace/ctf/types.h
> @@ -92,6 +92,7 @@ struct ctf_stream_pos {
>  
>  	int dummy;		/* dummy position, for length calculation */
>  	struct bt_stream_callbacks *cb;	/* Callbacks registered for iterator. */
> +	size_t file_length;	/* length of backing file, in bytes. */
>  	void *priv;
>  };
>  
> -- 
> 2.11.0
>
diff mbox series

Patch

diff --git a/formats/ctf/ctf.c b/formats/ctf/ctf.c
index ab98b288..b74cddd7 100644
--- a/formats/ctf/ctf.c
+++ b/formats/ctf/ctf.c
@@ -1180,6 +1180,14 @@  void ctf_packet_seek(struct bt_stream_pos *stream_pos, size_t index, int whence)
 				goto end;
 			}
 		}
+
+		if (packet_index->packet_size > ((uint64_t) pos->file_length - packet_index->offset) * CHAR_BIT) {
+			fprintf(stderr, "[error] Packet size (%" PRIu64 " bits) is larger than remaining file size (%" PRIu64 " bits).\n",
+				packet_index->packet_size, ((uint64_t) pos->file_length - packet_index->offset) * CHAR_BIT);
+			ret = -1;
+			goto end;
+		}
+
 		pos->content_size = packet_index->content_size;
 		pos->packet_size = packet_index->packet_size;
 		pos->mmap_offset = packet_index->offset;
@@ -1708,8 +1716,7 @@  int stream_assign_class(struct ctf_trace *td,
 static
 int create_stream_one_packet_index(struct ctf_stream_pos *pos,
 			struct ctf_trace *td,
-			struct ctf_file_stream *file_stream,
-			size_t filesize)
+			struct ctf_file_stream *file_stream)
 {
 	struct packet_index packet_index;
 	uint64_t stream_id = 0;
@@ -1724,8 +1731,8 @@  begin:
 		first_packet = 1;
 	}
 
-	if (filesize - pos->mmap_offset < (packet_map_len >> LOG2_CHAR_BIT)) {
-		packet_map_len = (filesize - pos->mmap_offset) << LOG2_CHAR_BIT;
+	if (pos->file_length - pos->mmap_offset < (packet_map_len >> LOG2_CHAR_BIT)) {
+		packet_map_len = (pos->file_length - pos->mmap_offset) << LOG2_CHAR_BIT;
 	}
 
 	if (pos->base_mma) {
@@ -1843,7 +1850,7 @@  begin:
 			packet_index.packet_size = bt_get_unsigned_int(field);
 		} else {
 			/* Use file size for packet size */
-			packet_index.packet_size = filesize * CHAR_BIT;
+			packet_index.packet_size = pos->file_length * CHAR_BIT;
 		}
 
 		/* read content size from header */
@@ -1855,7 +1862,7 @@  begin:
 			packet_index.content_size = bt_get_unsigned_int(field);
 		} else {
 			/* Use packet size if non-zero, else file size */
-			packet_index.content_size = packet_index.packet_size ? : filesize * CHAR_BIT;
+			packet_index.content_size = packet_index.packet_size ? : pos->file_length * CHAR_BIT;
 		}
 
 		/* read timestamp begin from header */
@@ -1912,9 +1919,9 @@  begin:
 		}
 	} else {
 		/* Use file size for packet size */
-		packet_index.packet_size = filesize * CHAR_BIT;
+		packet_index.packet_size = pos->file_length * CHAR_BIT;
 		/* Use packet size if non-zero, else file size */
-		packet_index.content_size = packet_index.packet_size ? : filesize * CHAR_BIT;
+		packet_index.content_size = packet_index.packet_size ? : pos->file_length * CHAR_BIT;
 	}
 
 	/* Validate content size and packet size values */
@@ -1924,12 +1931,6 @@  begin:
 		return -EINVAL;
 	}
 
-	if (packet_index.packet_size > ((uint64_t) filesize - packet_index.offset) * CHAR_BIT) {
-		fprintf(stderr, "[error] Packet size (%" PRIu64 " bits) is larger than remaining file size (%" PRIu64 " bits).\n",
-			packet_index.packet_size, ((uint64_t) filesize - packet_index.offset) * CHAR_BIT);
-		return -EINVAL;
-	}
-
 	if (packet_index.content_size < pos->offset) {
 		fprintf(stderr, "[error] Invalid CTF stream: content size is smaller than packet headers.\n");
 		return -EINVAL;
@@ -1952,7 +1953,7 @@  begin:
 
 	/* Retry with larger mapping */
 retry:
-	if (packet_map_len == ((filesize - pos->mmap_offset) << LOG2_CHAR_BIT)) {
+	if (packet_map_len == ((pos->file_length - pos->mmap_offset) << LOG2_CHAR_BIT)) {
 		/*
 		 * Reached EOF, but still expecting header/context data.
 		 */
@@ -1975,17 +1976,12 @@  int create_stream_packet_index(struct ctf_trace *td,
 			struct ctf_file_stream *file_stream)
 {
 	struct ctf_stream_pos *pos;
-	struct stat filestats;
 	int ret;
 
 	pos = &file_stream->pos;
 
-	ret = fstat(pos->fd, &filestats);
-	if (ret < 0)
-		return ret;
-
 	/* Deal with empty files */
-	if (!filestats.st_size) {
+	if (!pos->file_length) {
 		if (file_stream->parent.trace_packet_header
 				|| file_stream->parent.stream_packet_context) {
 			/*
@@ -2008,9 +2004,8 @@  int create_stream_packet_index(struct ctf_trace *td,
 		}
 	}
 
-	for (pos->mmap_offset = 0; pos->mmap_offset < filestats.st_size; ) {
-		ret = create_stream_one_packet_index(pos, td, file_stream,
-			filestats.st_size);
+	for (pos->mmap_offset = 0; pos->mmap_offset < pos->file_length; ) {
+		ret = create_stream_one_packet_index(pos, td, file_stream);
 		if (ret)
 			return ret;
 	}
@@ -2195,6 +2190,7 @@  int ctf_open_file_stream_read(struct ctf_trace *td, const char *path, int flags,
 	file_stream->pos.last_offset = LAST_OFFSET_POISON;
 	file_stream->pos.fd = -1;
 	file_stream->pos.index_fp = NULL;
+	file_stream->pos.file_length = statbuf.st_size;
 
 	strncpy(file_stream->parent.path, path, PATH_MAX);
 	file_stream->parent.path[PATH_MAX - 1] = '\0';
diff --git a/include/babeltrace/ctf/types.h b/include/babeltrace/ctf/types.h
index 59ce9981..0bc003c8 100644
--- a/include/babeltrace/ctf/types.h
+++ b/include/babeltrace/ctf/types.h
@@ -92,6 +92,7 @@  struct ctf_stream_pos {
 
 	int dummy;		/* dummy position, for length calculation */
 	struct bt_stream_callbacks *cb;	/* Callbacks registered for iterator. */
+	size_t file_length;	/* length of backing file, in bytes. */
 	void *priv;
 };