* [PATCH 01/11] xfsprogs: xfs_io: fix a memory leak in imap_f
2015-12-02 11:19 [PATCH 00/11] xfsprogs misc fixes Vivek Trivedi
@ 2015-12-02 11:19 ` Vivek Trivedi
2015-12-02 17:55 ` Eric Sandeen
2015-12-02 11:19 ` [PATCH 02/11] xfsprogs: fix integer overflow in xlog_find_verify_cycle Vivek Trivedi
` (10 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Vivek Trivedi @ 2015-12-02 11:19 UTC (permalink / raw)
To: xfs; +Cc: Vivek Trivedi, a.sahrawat, pankaj.m
add NULL check for malloc return and free allocated memory in
return path in imap_f
Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
---
io/imap.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/io/imap.c b/io/imap.c
index 34901cb..6ed98eb 100644
--- a/io/imap.c
+++ b/io/imap.c
@@ -39,6 +39,8 @@ imap_f(int argc, char **argv)
nent = atoi(argv[1]);
t = malloc(nent * sizeof(*t));
+ if (!t)
+ return 0;
bulkreq.lastip = &last;
bulkreq.icount = nent;
@@ -46,8 +48,10 @@ imap_f(int argc, char **argv)
bulkreq.ocount = &count;
while (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS, &bulkreq) == 0) {
- if (count == 0)
+ if (count == 0) {
+ free(t);
return 0;
+ }
for (i = 0; i < count; i++) {
printf(_("ino %10llu count %2d mask %016llx\n"),
(unsigned long long)t[i].xi_startino,
@@ -55,6 +59,7 @@ imap_f(int argc, char **argv)
(unsigned long long)t[i].xi_allocmask);
}
}
+ free(t);
perror("xfsctl(XFS_IOC_FSINUMBERS)");
exitcode = 1;
return 0;
--
1.7.9.5
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 01/11] xfsprogs: xfs_io: fix a memory leak in imap_f
2015-12-02 11:19 ` [PATCH 01/11] xfsprogs: xfs_io: fix a memory leak in imap_f Vivek Trivedi
@ 2015-12-02 17:55 ` Eric Sandeen
0 siblings, 0 replies; 30+ messages in thread
From: Eric Sandeen @ 2015-12-02 17:55 UTC (permalink / raw)
To: xfs
On 12/2/15 5:19 AM, Vivek Trivedi wrote:
> add NULL check for malloc return and free allocated memory in
> return path in imap_f
>
> Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> ---
> io/imap.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/io/imap.c b/io/imap.c
> index 34901cb..6ed98eb 100644
> --- a/io/imap.c
> +++ b/io/imap.c
> @@ -39,6 +39,8 @@ imap_f(int argc, char **argv)
> nent = atoi(argv[1]);
>
> t = malloc(nent * sizeof(*t));
> + if (!t)
> + return 0;
>
> bulkreq.lastip = &last;
> bulkreq.icount = nent;
> @@ -46,8 +48,10 @@ imap_f(int argc, char **argv)
> bulkreq.ocount = &count;
>
> while (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS, &bulkreq) == 0) {
> - if (count == 0)
> + if (count == 0) {
> + free(t);
> return 0;
> + }
> for (i = 0; i < count; i++) {
> printf(_("ino %10llu count %2d mask %016llx\n"),
> (unsigned long long)t[i].xi_startino,
> @@ -55,6 +59,7 @@ imap_f(int argc, char **argv)
> (unsigned long long)t[i].xi_allocmask);
> }
> }
> + free(t);
> perror("xfsctl(XFS_IOC_FSINUMBERS)");
> exitcode = 1;
> return 0;
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 02/11] xfsprogs: fix integer overflow in xlog_find_verify_cycle
2015-12-02 11:19 [PATCH 00/11] xfsprogs misc fixes Vivek Trivedi
2015-12-02 11:19 ` [PATCH 01/11] xfsprogs: xfs_io: fix a memory leak in imap_f Vivek Trivedi
@ 2015-12-02 11:19 ` Vivek Trivedi
2015-12-02 17:58 ` Eric Sandeen
2015-12-03 5:49 ` Dave Chinner
2015-12-02 11:19 ` [PATCH 03/11] xfsprogs: mkfs: fix unintentional integer overflow Vivek Trivedi
` (9 subsequent siblings)
11 siblings, 2 replies; 30+ messages in thread
From: Vivek Trivedi @ 2015-12-02 11:19 UTC (permalink / raw)
To: xfs; +Cc: Vivek Trivedi, a.sahrawat, pankaj.m
Fix unintentional integer overflow in xlog_find_verify_cycle.
Reported by coverity.
Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
---
libxlog/xfs_log_recover.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libxlog/xfs_log_recover.c b/libxlog/xfs_log_recover.c
index ef7cf68..73f4d36 100644
--- a/libxlog/xfs_log_recover.c
+++ b/libxlog/xfs_log_recover.c
@@ -254,7 +254,7 @@ xlog_find_verify_cycle(
* try a smaller size. We need to be able to read at least
* a log sector, or we're out of luck.
*/
- bufblks = 1 << ffs(nbblks);
+ bufblks = (xfs_daddr_t)1 << ffs(nbblks);
while (bufblks > log->l_logBBsize)
bufblks >>= 1;
while (!(bp = xlog_get_bp(log, bufblks))) {
--
1.7.9.5
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 02/11] xfsprogs: fix integer overflow in xlog_find_verify_cycle
2015-12-02 11:19 ` [PATCH 02/11] xfsprogs: fix integer overflow in xlog_find_verify_cycle Vivek Trivedi
@ 2015-12-02 17:58 ` Eric Sandeen
2015-12-03 5:49 ` Dave Chinner
1 sibling, 0 replies; 30+ messages in thread
From: Eric Sandeen @ 2015-12-02 17:58 UTC (permalink / raw)
To: xfs
On 12/2/15 5:19 AM, Vivek Trivedi wrote:
> Fix unintentional integer overflow in xlog_find_verify_cycle.
> Reported by coverity.
>
> Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
Coverity-Id: 200684
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> ---
> libxlog/xfs_log_recover.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libxlog/xfs_log_recover.c b/libxlog/xfs_log_recover.c
> index ef7cf68..73f4d36 100644
> --- a/libxlog/xfs_log_recover.c
> +++ b/libxlog/xfs_log_recover.c
> @@ -254,7 +254,7 @@ xlog_find_verify_cycle(
> * try a smaller size. We need to be able to read at least
> * a log sector, or we're out of luck.
> */
> - bufblks = 1 << ffs(nbblks);
> + bufblks = (xfs_daddr_t)1 << ffs(nbblks);
> while (bufblks > log->l_logBBsize)
> bufblks >>= 1;
> while (!(bp = xlog_get_bp(log, bufblks))) {
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 02/11] xfsprogs: fix integer overflow in xlog_find_verify_cycle
2015-12-02 11:19 ` [PATCH 02/11] xfsprogs: fix integer overflow in xlog_find_verify_cycle Vivek Trivedi
2015-12-02 17:58 ` Eric Sandeen
@ 2015-12-03 5:49 ` Dave Chinner
1 sibling, 0 replies; 30+ messages in thread
From: Dave Chinner @ 2015-12-03 5:49 UTC (permalink / raw)
To: Vivek Trivedi; +Cc: a.sahrawat, pankaj.m, xfs
On Wed, Dec 02, 2015 at 04:49:18PM +0530, Vivek Trivedi wrote:
> Fix unintentional integer overflow in xlog_find_verify_cycle.
> Reported by coverity.
>
> Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
> ---
> libxlog/xfs_log_recover.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libxlog/xfs_log_recover.c b/libxlog/xfs_log_recover.c
> index ef7cf68..73f4d36 100644
> --- a/libxlog/xfs_log_recover.c
> +++ b/libxlog/xfs_log_recover.c
> @@ -254,7 +254,7 @@ xlog_find_verify_cycle(
> * try a smaller size. We need to be able to read at least
> * a log sector, or we're out of luck.
> */
> - bufblks = 1 << ffs(nbblks);
> + bufblks = (xfs_daddr_t)1 << ffs(nbblks);
Ummm, in isolation that change is technically correct, but when you
look at what bufblks contains it is clearly wrong. nbblks is an
int, so "1 << ffs(nbblks)" should not be larger than an int.
i.e. bufblks is simply a count of blocks in the log, which by
definition cannot be more than an int (in fact, 2^31 / 2^9 is the
largest legal value it can have). Hence it can't be larger than an
int, and all the functions it is passed to expect it to be an
int...
Hence the use of xfs_daddr_t is wrong, and that's the first thing
that needs fixing....
> while (bufblks > log->l_logBBsize)
> bufblks >>= 1;
> while (!(bp = xlog_get_bp(log, bufblks))) {
^^^^^^^^
And given this, it's clear that coverity doesn't warn about
overflows caused by passing a 64 bit value into a 32 bit function
parameter....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 03/11] xfsprogs: mkfs: fix unintentional integer overflow
2015-12-02 11:19 [PATCH 00/11] xfsprogs misc fixes Vivek Trivedi
2015-12-02 11:19 ` [PATCH 01/11] xfsprogs: xfs_io: fix a memory leak in imap_f Vivek Trivedi
2015-12-02 11:19 ` [PATCH 02/11] xfsprogs: fix integer overflow in xlog_find_verify_cycle Vivek Trivedi
@ 2015-12-02 11:19 ` Vivek Trivedi
2015-12-03 3:34 ` Eric Sandeen
2015-12-03 5:54 ` Dave Chinner
2015-12-02 11:19 ` [PATCH 04/11] xfsprogs: xfsdb: remove unnessary checks in process_leaf_node_dir_v2_free Vivek Trivedi
` (8 subsequent siblings)
11 siblings, 2 replies; 30+ messages in thread
From: Vivek Trivedi @ 2015-12-02 11:19 UTC (permalink / raw)
To: xfs; +Cc: Vivek Trivedi, a.sahrawat, pankaj.m
Fix unintentional integer overflow in mkfs.
Reported by coverity.
Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
---
mkfs/xfs_mkfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 7cba41a..e540c48 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2033,7 +2033,7 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
/* check that rswidth is a multiple of fs blocksize */
if (!norsflag && rswidth && !(BBTOB(rswidth) % blocksize)) {
rswidth = DTOBT(rswidth);
- rtextbytes = rswidth << blocklog;
+ rtextbytes = (__uint64_t)rswidth << blocklog;
if (XFS_MIN_RTEXTSIZE <= rtextbytes &&
(rtextbytes <= XFS_MAX_RTEXTSIZE)) {
rtextblocks = rswidth;
--
1.7.9.5
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 03/11] xfsprogs: mkfs: fix unintentional integer overflow
2015-12-02 11:19 ` [PATCH 03/11] xfsprogs: mkfs: fix unintentional integer overflow Vivek Trivedi
@ 2015-12-03 3:34 ` Eric Sandeen
2015-12-03 5:54 ` Dave Chinner
1 sibling, 0 replies; 30+ messages in thread
From: Eric Sandeen @ 2015-12-03 3:34 UTC (permalink / raw)
To: xfs
I don't think it's possible for blocklog to be large enough to overflow,
but I suppose being defensive is fine.
Coverity-Id: 996962
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
On 12/2/15 5:19 AM, Vivek Trivedi wrote:
> Fix unintentional integer overflow in mkfs.
> Reported by coverity.
>
> Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
> ---
> mkfs/xfs_mkfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 7cba41a..e540c48 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2033,7 +2033,7 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
> /* check that rswidth is a multiple of fs blocksize */
> if (!norsflag && rswidth && !(BBTOB(rswidth) % blocksize)) {
> rswidth = DTOBT(rswidth);
> - rtextbytes = rswidth << blocklog;
> + rtextbytes = (__uint64_t)rswidth << blocklog;
> if (XFS_MIN_RTEXTSIZE <= rtextbytes &&
> (rtextbytes <= XFS_MAX_RTEXTSIZE)) {
> rtextblocks = rswidth;
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 03/11] xfsprogs: mkfs: fix unintentional integer overflow
2015-12-02 11:19 ` [PATCH 03/11] xfsprogs: mkfs: fix unintentional integer overflow Vivek Trivedi
2015-12-03 3:34 ` Eric Sandeen
@ 2015-12-03 5:54 ` Dave Chinner
1 sibling, 0 replies; 30+ messages in thread
From: Dave Chinner @ 2015-12-03 5:54 UTC (permalink / raw)
To: Vivek Trivedi; +Cc: a.sahrawat, pankaj.m, xfs
On Wed, Dec 02, 2015 at 04:49:19PM +0530, Vivek Trivedi wrote:
> Fix unintentional integer overflow in mkfs.
> Reported by coverity.
>
> Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
> ---
> mkfs/xfs_mkfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 7cba41a..e540c48 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2033,7 +2033,7 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
> /* check that rswidth is a multiple of fs blocksize */
> if (!norsflag && rswidth && !(BBTOB(rswidth) % blocksize)) {
> rswidth = DTOBT(rswidth);
> - rtextbytes = rswidth << blocklog;
> + rtextbytes = (__uint64_t)rswidth << blocklog;
> if (XFS_MIN_RTEXTSIZE <= rtextbytes &&
> (rtextbytes <= XFS_MAX_RTEXTSIZE)) {
> rtextblocks = rswidth;
I dislike unexplained casts in code like this. Nobody knows exactly
why it is there. A better fix is to change the definition of
rswidth to a 64 bit type so casts are not ever necessary...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 04/11] xfsprogs: xfsdb: remove unnessary checks in process_leaf_node_dir_v2_free
2015-12-02 11:19 [PATCH 00/11] xfsprogs misc fixes Vivek Trivedi
` (2 preceding siblings ...)
2015-12-02 11:19 ` [PATCH 03/11] xfsprogs: mkfs: fix unintentional integer overflow Vivek Trivedi
@ 2015-12-02 11:19 ` Vivek Trivedi
2015-12-03 5:57 ` Dave Chinner
2015-12-02 11:19 ` [PATCH 05/11] xfsprogs: xfs_mdrestore: check bad read count in perform_restore Vivek Trivedi
` (7 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Vivek Trivedi @ 2015-12-02 11:19 UTC (permalink / raw)
To: xfs; +Cc: Vivek Trivedi, a.sahrawat, pankaj.m
xfs_dir2_free_hdr_t entries are unsigned, so they can't be negative.
remove these unnessary checks in process_leaf_node_dir_v2_free.
Reported by coverity.
Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
---
db/check.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/db/check.c b/db/check.c
index 9c1541d..9d986b9 100644
--- a/db/check.c
+++ b/db/check.c
@@ -3073,9 +3073,7 @@ process_leaf_node_dir_v3_free(
return;
}
if (be32_to_cpu(free->hdr.nvalid) > maxent ||
- be32_to_cpu(free->hdr.nvalid) < 0 ||
be32_to_cpu(free->hdr.nused) > maxent ||
- be32_to_cpu(free->hdr.nused) < 0 ||
be32_to_cpu(free->hdr.nused) >
be32_to_cpu(free->hdr.nvalid)) {
if (!sflag || v)
--
1.7.9.5
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 04/11] xfsprogs: xfsdb: remove unnessary checks in process_leaf_node_dir_v2_free
2015-12-02 11:19 ` [PATCH 04/11] xfsprogs: xfsdb: remove unnessary checks in process_leaf_node_dir_v2_free Vivek Trivedi
@ 2015-12-03 5:57 ` Dave Chinner
0 siblings, 0 replies; 30+ messages in thread
From: Dave Chinner @ 2015-12-03 5:57 UTC (permalink / raw)
To: Vivek Trivedi; +Cc: a.sahrawat, pankaj.m, xfs
On Wed, Dec 02, 2015 at 04:49:20PM +0530, Vivek Trivedi wrote:
> xfs_dir2_free_hdr_t entries are unsigned, so they can't be negative.
> remove these unnessary checks in process_leaf_node_dir_v2_free.
> Reported by coverity.
>
> Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
> ---
> db/check.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/db/check.c b/db/check.c
> index 9c1541d..9d986b9 100644
> --- a/db/check.c
> +++ b/db/check.c
> @@ -3073,9 +3073,7 @@ process_leaf_node_dir_v3_free(
> return;
> }
> if (be32_to_cpu(free->hdr.nvalid) > maxent ||
> - be32_to_cpu(free->hdr.nvalid) < 0 ||
> be32_to_cpu(free->hdr.nused) > maxent ||
> - be32_to_cpu(free->hdr.nused) < 0 ||
> be32_to_cpu(free->hdr.nused) >
> be32_to_cpu(free->hdr.nvalid)) {
> if (!sflag || v)
Pretty meaningless, but I think we made this change libxfs code some
time ago, so we may as well change it here....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 05/11] xfsprogs: xfs_mdrestore: check bad read count in perform_restore
2015-12-02 11:19 [PATCH 00/11] xfsprogs misc fixes Vivek Trivedi
` (3 preceding siblings ...)
2015-12-02 11:19 ` [PATCH 04/11] xfsprogs: xfsdb: remove unnessary checks in process_leaf_node_dir_v2_free Vivek Trivedi
@ 2015-12-02 11:19 ` Vivek Trivedi
2015-12-03 4:54 ` Eric Sandeen
2015-12-02 11:19 ` [PATCH 06/11] xfsprogs: xfs_db: check null derefernce after block_to_bt Vivek Trivedi
` (6 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Vivek Trivedi @ 2015-12-02 11:19 UTC (permalink / raw)
To: xfs; +Cc: Vivek Trivedi, a.sahrawat, pankaj.m
fix error reported by coverity - Integer overflowed argument
also, add print incase of invalid read count to get more debug
information.
Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
---
mdrestore/xfs_mdrestore.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
index 5764616..a87a091 100644
--- a/mdrestore/xfs_mdrestore.c
+++ b/mdrestore/xfs_mdrestore.c
@@ -93,6 +93,10 @@ perform_restore(
block_index = (__be64 *)((char *)metablock + sizeof(xfs_metablock_t));
block_buffer = (char *)metablock + block_size;
+ if (block_size < sizeof(tmb))
+ fatal("bad read count, block_size: %d, tmb size %d\n",
+ block_size, sizeof(tmb));
+
if (fread(block_index, block_size - sizeof(tmb), 1, src_f) != 1)
fatal("error reading from file: %s\n", strerror(errno));
--
1.7.9.5
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 05/11] xfsprogs: xfs_mdrestore: check bad read count in perform_restore
2015-12-02 11:19 ` [PATCH 05/11] xfsprogs: xfs_mdrestore: check bad read count in perform_restore Vivek Trivedi
@ 2015-12-03 4:54 ` Eric Sandeen
2015-12-03 5:59 ` Dave Chinner
0 siblings, 1 reply; 30+ messages in thread
From: Eric Sandeen @ 2015-12-03 4:54 UTC (permalink / raw)
To: xfs
On 12/2/15 5:19 AM, Vivek Trivedi wrote:
> fix error reported by coverity - Integer overflowed argument
>
> also, add print incase of invalid read count to get more debug
> information.
>
> Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
> ---
> mdrestore/xfs_mdrestore.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> index 5764616..a87a091 100644
> --- a/mdrestore/xfs_mdrestore.c
> +++ b/mdrestore/xfs_mdrestore.c
> @@ -93,6 +93,10 @@ perform_restore(
> block_index = (__be64 *)((char *)metablock + sizeof(xfs_metablock_t));
> block_buffer = (char *)metablock + block_size;
>
> + if (block_size < sizeof(tmb))
> + fatal("bad read count, block_size: %d, tmb size %d\n",
> + block_size, sizeof(tmb));
> +
block_size is block_size = 1 << tmb.mb_blocklog; where mb_blocklog is
always metablock->mb_blocklog = BBSHIFT;, so block_size is always 512.
On the other hand, sizeof(tmb) is simply 8.
There seems to be no possible path for this to be a problem, so it hardly
seems worth the printf.
Would an ASSERT(block_size >= sizeof(tmb)) make coverity happy?
-Eric
> if (fread(block_index, block_size - sizeof(tmb), 1, src_f) != 1)
> fatal("error reading from file: %s\n", strerror(errno));
>
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 05/11] xfsprogs: xfs_mdrestore: check bad read count in perform_restore
2015-12-03 4:54 ` Eric Sandeen
@ 2015-12-03 5:59 ` Dave Chinner
2015-12-03 6:05 ` Eric Sandeen
0 siblings, 1 reply; 30+ messages in thread
From: Dave Chinner @ 2015-12-03 5:59 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On Wed, Dec 02, 2015 at 10:54:19PM -0600, Eric Sandeen wrote:
> On 12/2/15 5:19 AM, Vivek Trivedi wrote:
> > fix error reported by coverity - Integer overflowed argument
> >
> > also, add print incase of invalid read count to get more debug
> > information.
> >
> > Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
> > ---
> > mdrestore/xfs_mdrestore.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> > index 5764616..a87a091 100644
> > --- a/mdrestore/xfs_mdrestore.c
> > +++ b/mdrestore/xfs_mdrestore.c
> > @@ -93,6 +93,10 @@ perform_restore(
> > block_index = (__be64 *)((char *)metablock + sizeof(xfs_metablock_t));
> > block_buffer = (char *)metablock + block_size;
> >
> > + if (block_size < sizeof(tmb))
> > + fatal("bad read count, block_size: %d, tmb size %d\n",
> > + block_size, sizeof(tmb));
> > +
>
> block_size is block_size = 1 << tmb.mb_blocklog; where mb_blocklog is
> always metablock->mb_blocklog = BBSHIFT;, so block_size is always 512.
>
> On the other hand, sizeof(tmb) is simply 8.
>
> There seems to be no possible path for this to be a problem, so it hardly
> seems worth the printf.
>
> Would an ASSERT(block_size >= sizeof(tmb)) make coverity happy?
Just ignoring this coverity warning would be more appropriate, i
think.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 05/11] xfsprogs: xfs_mdrestore: check bad read count in perform_restore
2015-12-03 5:59 ` Dave Chinner
@ 2015-12-03 6:05 ` Eric Sandeen
0 siblings, 0 replies; 30+ messages in thread
From: Eric Sandeen @ 2015-12-03 6:05 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On 12/2/15 11:59 PM, Dave Chinner wrote:
> On Wed, Dec 02, 2015 at 10:54:19PM -0600, Eric Sandeen wrote:
>> On 12/2/15 5:19 AM, Vivek Trivedi wrote:
>>> fix error reported by coverity - Integer overflowed argument
>>>
>>> also, add print incase of invalid read count to get more debug
>>> information.
>>>
>>> Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
>>> ---
>>> mdrestore/xfs_mdrestore.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
>>> index 5764616..a87a091 100644
>>> --- a/mdrestore/xfs_mdrestore.c
>>> +++ b/mdrestore/xfs_mdrestore.c
>>> @@ -93,6 +93,10 @@ perform_restore(
>>> block_index = (__be64 *)((char *)metablock + sizeof(xfs_metablock_t));
>>> block_buffer = (char *)metablock + block_size;
>>>
>>> + if (block_size < sizeof(tmb))
>>> + fatal("bad read count, block_size: %d, tmb size %d\n",
>>> + block_size, sizeof(tmb));
>>> +
>>
>> block_size is block_size = 1 << tmb.mb_blocklog; where mb_blocklog is
>> always metablock->mb_blocklog = BBSHIFT;, so block_size is always 512.
>>
>> On the other hand, sizeof(tmb) is simply 8.
>>
>> There seems to be no possible path for this to be a problem, so it hardly
>> seems worth the printf.
>>
>> Would an ASSERT(block_size >= sizeof(tmb)) make coverity happy?
>
> Just ignoring this coverity warning would be more appropriate, i
> think.
Or that ... FWIW this one does not even show up in the coverity scan project
as an issue.
-Eric
> Cheers,
>
> Dave.
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 06/11] xfsprogs: xfs_db: check null derefernce after block_to_bt
2015-12-02 11:19 [PATCH 00/11] xfsprogs misc fixes Vivek Trivedi
` (4 preceding siblings ...)
2015-12-02 11:19 ` [PATCH 05/11] xfsprogs: xfs_mdrestore: check bad read count in perform_restore Vivek Trivedi
@ 2015-12-02 11:19 ` Vivek Trivedi
2015-12-03 5:43 ` Eric Sandeen
2015-12-02 11:19 ` [PATCH 07/11] xfsprogs: xfs_fsr: replace sprintf with snprintf to avoid buffer overflow Vivek Trivedi
` (5 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Vivek Trivedi @ 2015-12-02 11:19 UTC (permalink / raw)
To: xfs; +Cc: Vivek Trivedi, a.sahrawat, pankaj.m
add assert if block_to_bt returns NULL to avoid null pointer
dereference and get backtrace.
Reported by coverity.
Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
---
db/btblock.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/db/btblock.c b/db/btblock.c
index 46140fc..91593f8 100644
--- a/db/btblock.c
+++ b/db/btblock.c
@@ -180,6 +180,7 @@ btblock_key_offset(
struct xfs_db_btree *bt = block_to_bt(block);
int offset;
+ ASSERT(bt != NULL);
ASSERT(startoff == 0);
ASSERT(block->bb_level != 0);
@@ -201,6 +202,7 @@ btblock_ptr_offset(
int offset;
int maxrecs;
+ ASSERT(bt != NULL);
ASSERT(startoff == 0);
ASSERT(block->bb_level != 0);
@@ -225,6 +227,7 @@ btblock_rec_offset(
struct xfs_db_btree *bt = block_to_bt(block);
int offset;
+ ASSERT(bt != NULL);
ASSERT(startoff == 0);
ASSERT(block->bb_level == 0);
--
1.7.9.5
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 06/11] xfsprogs: xfs_db: check null derefernce after block_to_bt
2015-12-02 11:19 ` [PATCH 06/11] xfsprogs: xfs_db: check null derefernce after block_to_bt Vivek Trivedi
@ 2015-12-03 5:43 ` Eric Sandeen
0 siblings, 0 replies; 30+ messages in thread
From: Eric Sandeen @ 2015-12-03 5:43 UTC (permalink / raw)
To: xfs
I think this is ok; another "should never happen" scenario,
so the ASSERT seems fine.
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
On 12/2/15 5:19 AM, Vivek Trivedi wrote:
> add assert if block_to_bt returns NULL to avoid null pointer
> dereference and get backtrace.
> Reported by coverity.
>
> Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
> ---
> db/btblock.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/db/btblock.c b/db/btblock.c
> index 46140fc..91593f8 100644
> --- a/db/btblock.c
> +++ b/db/btblock.c
> @@ -180,6 +180,7 @@ btblock_key_offset(
> struct xfs_db_btree *bt = block_to_bt(block);
> int offset;
>
> + ASSERT(bt != NULL);
> ASSERT(startoff == 0);
> ASSERT(block->bb_level != 0);
>
> @@ -201,6 +202,7 @@ btblock_ptr_offset(
> int offset;
> int maxrecs;
>
> + ASSERT(bt != NULL);
> ASSERT(startoff == 0);
> ASSERT(block->bb_level != 0);
>
> @@ -225,6 +227,7 @@ btblock_rec_offset(
> struct xfs_db_btree *bt = block_to_bt(block);
> int offset;
>
> + ASSERT(bt != NULL);
> ASSERT(startoff == 0);
> ASSERT(block->bb_level == 0);
>
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 07/11] xfsprogs: xfs_fsr: replace sprintf with snprintf to avoid buffer overflow
2015-12-02 11:19 [PATCH 00/11] xfsprogs misc fixes Vivek Trivedi
` (5 preceding siblings ...)
2015-12-02 11:19 ` [PATCH 06/11] xfsprogs: xfs_db: check null derefernce after block_to_bt Vivek Trivedi
@ 2015-12-02 11:19 ` Vivek Trivedi
2015-12-03 5:44 ` Eric Sandeen
2015-12-02 11:19 ` [PATCH 08/11] xfsprogs: xfs_repair: fix possible null dereference in build_ino_tree Vivek Trivedi
` (4 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Vivek Trivedi @ 2015-12-02 11:19 UTC (permalink / raw)
To: xfs; +Cc: Vivek Trivedi, a.sahrawat, pankaj.m
Fix possible buffer overflow by replacing sprintf with snprintf in tmp_next
and tmp_close.
Reported by coverity.
Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
---
fsr/xfs_fsr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 424fbce..bd459b6 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -1804,7 +1804,7 @@ tmp_next(char *mnt)
{
static char buf[SMBUFSZ];
- sprintf(buf, "%s/.fsr/ag%d/tmp%d",
+ snprintf(buf, SMBUFSZ, "%s/.fsr/ag%d/tmp%d",
( (strcmp(mnt, "/") == 0) ? "" : mnt),
tmp_agi,
getpid());
@@ -1823,7 +1823,7 @@ tmp_close(char *mnt)
/* No data is ever actually written so we can just do rmdir's */
for (i=0; i < fsgeom.agcount; i++) {
- sprintf(buf, "%s/.fsr/ag%d", mnt, i);
+ snprintf(buf, SMBUFSZ, "%s/.fsr/ag%d", mnt, i);
if (rmdir(buf) < 0) {
if (errno != ENOENT) {
fsrprintf(
--
1.7.9.5
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 07/11] xfsprogs: xfs_fsr: replace sprintf with snprintf to avoid buffer overflow
2015-12-02 11:19 ` [PATCH 07/11] xfsprogs: xfs_fsr: replace sprintf with snprintf to avoid buffer overflow Vivek Trivedi
@ 2015-12-03 5:44 ` Eric Sandeen
2015-12-03 6:07 ` Dave Chinner
0 siblings, 1 reply; 30+ messages in thread
From: Eric Sandeen @ 2015-12-03 5:44 UTC (permalink / raw)
To: xfs
it seems like the sprintfs in i.e. fsrall_cleanup() and tmp_init()
might have the same problem, no?
And then what happens if it is truncated to SMBUFSZ; at that point
I think this needs error handling, if the string got truncated.
-Eric
On 12/2/15 5:19 AM, Vivek Trivedi wrote:
> Fix possible buffer overflow by replacing sprintf with snprintf in tmp_next
> and tmp_close.
> Reported by coverity.
>
> Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
> ---
> fsr/xfs_fsr.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index 424fbce..bd459b6 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -1804,7 +1804,7 @@ tmp_next(char *mnt)
> {
> static char buf[SMBUFSZ];
>
> - sprintf(buf, "%s/.fsr/ag%d/tmp%d",
> + snprintf(buf, SMBUFSZ, "%s/.fsr/ag%d/tmp%d",
> ( (strcmp(mnt, "/") == 0) ? "" : mnt),
> tmp_agi,
> getpid());
> @@ -1823,7 +1823,7 @@ tmp_close(char *mnt)
>
> /* No data is ever actually written so we can just do rmdir's */
> for (i=0; i < fsgeom.agcount; i++) {
> - sprintf(buf, "%s/.fsr/ag%d", mnt, i);
> + snprintf(buf, SMBUFSZ, "%s/.fsr/ag%d", mnt, i);
> if (rmdir(buf) < 0) {
> if (errno != ENOENT) {
> fsrprintf(
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 07/11] xfsprogs: xfs_fsr: replace sprintf with snprintf to avoid buffer overflow
2015-12-03 5:44 ` Eric Sandeen
@ 2015-12-03 6:07 ` Dave Chinner
0 siblings, 0 replies; 30+ messages in thread
From: Dave Chinner @ 2015-12-03 6:07 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On Wed, Dec 02, 2015 at 11:44:02PM -0600, Eric Sandeen wrote:
>
> it seems like the sprintfs in i.e. fsrall_cleanup() and tmp_init()
> might have the same problem, no?
>
> And then what happens if it is truncated to SMBUFSZ; at that point
> I think this needs error handling, if the string got truncated.
Might be easier to simply increase the size of SMBUFSZ so that
overrun is not possible?
-Dave
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 08/11] xfsprogs: xfs_repair: fix possible null dereference in build_ino_tree
2015-12-02 11:19 [PATCH 00/11] xfsprogs misc fixes Vivek Trivedi
` (6 preceding siblings ...)
2015-12-02 11:19 ` [PATCH 07/11] xfsprogs: xfs_fsr: replace sprintf with snprintf to avoid buffer overflow Vivek Trivedi
@ 2015-12-02 11:19 ` Vivek Trivedi
2015-12-03 6:19 ` Dave Chinner
2015-12-02 11:19 ` [PATCH 09/11] xfsprogs: xfs_repair: fix possible null dereference in traverse_int_dir2block Vivek Trivedi
` (3 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Vivek Trivedi @ 2015-12-02 11:19 UTC (permalink / raw)
To: xfs; +Cc: Vivek Trivedi, a.sahrawat, pankaj.m
Fix possible null dereference in build_ino_tree if ino_rec is NULL.
Reported by coverity.
Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
---
repair/phase5.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/repair/phase5.c b/repair/phase5.c
index 109e37b..5d95e22 100644
--- a/repair/phase5.c
+++ b/repair/phase5.c
@@ -1235,7 +1235,7 @@ build_ino_tree(xfs_mount_t *mp, xfs_agnumber_t agno,
if (lptr->modulo > 0)
lptr->modulo--;
- if (lptr->num_recs_pb > 0)
+ if (lptr->num_recs_pb > 0 && ino_rec)
prop_ino_cursor(mp, agno, btree_curs,
ino_rec->ino_startnum, 0);
--
1.7.9.5
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 08/11] xfsprogs: xfs_repair: fix possible null dereference in build_ino_tree
2015-12-02 11:19 ` [PATCH 08/11] xfsprogs: xfs_repair: fix possible null dereference in build_ino_tree Vivek Trivedi
@ 2015-12-03 6:19 ` Dave Chinner
0 siblings, 0 replies; 30+ messages in thread
From: Dave Chinner @ 2015-12-03 6:19 UTC (permalink / raw)
To: Vivek Trivedi; +Cc: a.sahrawat, pankaj.m, xfs
On Wed, Dec 02, 2015 at 04:49:24PM +0530, Vivek Trivedi wrote:
> Fix possible null dereference in build_ino_tree if ino_rec is NULL.
> Reported by coverity.
>
> Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
> ---
> repair/phase5.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/repair/phase5.c b/repair/phase5.c
> index 109e37b..5d95e22 100644
> --- a/repair/phase5.c
> +++ b/repair/phase5.c
> @@ -1235,7 +1235,7 @@ build_ino_tree(xfs_mount_t *mp, xfs_agnumber_t agno,
> if (lptr->modulo > 0)
> lptr->modulo--;
>
> - if (lptr->num_recs_pb > 0)
> + if (lptr->num_recs_pb > 0 && ino_rec)
> prop_ino_cursor(mp, agno, btree_curs,
> ino_rec->ino_startnum, 0);
>
Another "can't happen" case. The only time that ino_rec can be zero
is if there are no inodes in the AG, and in that case
init_ino_cursor() initialises lptr->num_recs_pb = 0.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 09/11] xfsprogs: xfs_repair: fix possible null dereference in traverse_int_dir2block
2015-12-02 11:19 [PATCH 00/11] xfsprogs misc fixes Vivek Trivedi
` (7 preceding siblings ...)
2015-12-02 11:19 ` [PATCH 08/11] xfsprogs: xfs_repair: fix possible null dereference in build_ino_tree Vivek Trivedi
@ 2015-12-02 11:19 ` Vivek Trivedi
2015-12-03 5:51 ` Eric Sandeen
2015-12-02 11:19 ` [PATCH 10/11] xfsprogs: fix possible null pointer dereference in xfs_iformat_extents Vivek Trivedi
` (2 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Vivek Trivedi @ 2015-12-02 11:19 UTC (permalink / raw)
To: xfs; +Cc: Vivek Trivedi, a.sahrawat, pankaj.m
Fix possible null dereference in traverse_int_dir2block if buffer pointer is NULL.
Reported by coverity.
Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
---
repair/dir2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/repair/dir2.c b/repair/dir2.c
index 61912d1..fe360dc 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -1300,7 +1300,7 @@ _("block %" PRIu64 " for directory inode %" PRIu64 " is missing\n"),
bp = da_read_buf(mp, nex, bmp, &xfs_dir3_data_buf_ops);
if (bmp != &lbmp)
free(bmp);
- if (bp == NULL) {
+ if (bp == NULL || !bp->b_addr) {
do_warn(
_("can't read block %" PRIu64 " for directory inode %" PRIu64 "\n"),
dbno, ino);
--
1.7.9.5
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 09/11] xfsprogs: xfs_repair: fix possible null dereference in traverse_int_dir2block
2015-12-02 11:19 ` [PATCH 09/11] xfsprogs: xfs_repair: fix possible null dereference in traverse_int_dir2block Vivek Trivedi
@ 2015-12-03 5:51 ` Eric Sandeen
2015-12-03 6:22 ` Dave Chinner
0 siblings, 1 reply; 30+ messages in thread
From: Eric Sandeen @ 2015-12-03 5:51 UTC (permalink / raw)
To: xfs
On 12/2/15 5:19 AM, Vivek Trivedi wrote:
> Fix possible null dereference in traverse_int_dir2block if buffer pointer is NULL.
> Reported by coverity.
Hm, against what version of xfsprogs?
traverse_int_dir2block has been gone for a while now. Can you please recheck
against current git, and if there's still an issue, explain a bit more;
I don't see offhand how we get a bp back from da_read_buf with a null bp->b_addr.
thanks,
-Eric
> Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
> ---
> repair/dir2.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/repair/dir2.c b/repair/dir2.c
> index 61912d1..fe360dc 100644
> --- a/repair/dir2.c
> +++ b/repair/dir2.c
> @@ -1300,7 +1300,7 @@ _("block %" PRIu64 " for directory inode %" PRIu64 " is missing\n"),
> bp = da_read_buf(mp, nex, bmp, &xfs_dir3_data_buf_ops);
> if (bmp != &lbmp)
> free(bmp);
> - if (bp == NULL) {
> + if (bp == NULL || !bp->b_addr) {
> do_warn(
> _("can't read block %" PRIu64 " for directory inode %" PRIu64 "\n"),
> dbno, ino);
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 09/11] xfsprogs: xfs_repair: fix possible null dereference in traverse_int_dir2block
2015-12-03 5:51 ` Eric Sandeen
@ 2015-12-03 6:22 ` Dave Chinner
0 siblings, 0 replies; 30+ messages in thread
From: Dave Chinner @ 2015-12-03 6:22 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On Wed, Dec 02, 2015 at 11:51:31PM -0600, Eric Sandeen wrote:
> On 12/2/15 5:19 AM, Vivek Trivedi wrote:
> > Fix possible null dereference in traverse_int_dir2block if buffer pointer is NULL.
> > Reported by coverity.
>
> Hm, against what version of xfsprogs?
>
> traverse_int_dir2block has been gone for a while now. Can you please recheck
> against current git, and if there's still an issue, explain a bit more;
> I don't see offhand how we get a bp back from da_read_buf with a null bp->b_addr.
it's also worth pointing out that there are several callers of
da_read_buf() in this function, all of which have identical error
checking and hence are all going to have the same problem if it
does actually exist.
When coverity issues a warning, we really need to check all
instances of the same code for the same problem, even if coverity
doesn't warn about the other call sites....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 10/11] xfsprogs: fix possible null pointer dereference in xfs_iformat_extents
2015-12-02 11:19 [PATCH 00/11] xfsprogs misc fixes Vivek Trivedi
` (8 preceding siblings ...)
2015-12-02 11:19 ` [PATCH 09/11] xfsprogs: xfs_repair: fix possible null dereference in traverse_int_dir2block Vivek Trivedi
@ 2015-12-02 11:19 ` Vivek Trivedi
2015-12-03 6:31 ` Dave Chinner
2015-12-02 11:19 ` [PATCH 11/11] xfsprogs: xfs_repair: fix possible null pointer dereference in mark_standalone_inodes Vivek Trivedi
2015-12-02 15:10 ` [PATCH 00/11] xfsprogs misc fixes Eric Sandeen
11 siblings, 1 reply; 30+ messages in thread
From: Vivek Trivedi @ 2015-12-02 11:19 UTC (permalink / raw)
To: xfs; +Cc: Vivek Trivedi, a.sahrawat, pankaj.m
Fix possible null pointer dereference in xfs_iformat_extents and
xfs_iext_get_ext if fail to locate inode record.
Reported by coverity.
Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
---
libxfs/xfs_inode_fork.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/libxfs/xfs_inode_fork.c b/libxfs/xfs_inode_fork.c
index e1968b4..36aa0c8 100644
--- a/libxfs/xfs_inode_fork.c
+++ b/libxfs/xfs_inode_fork.c
@@ -331,6 +331,8 @@ xfs_iformat_extents(
xfs_validate_extents(ifp, nex, XFS_EXTFMT_INODE(ip));
for (i = 0; i < nex; i++, dp++) {
xfs_bmbt_rec_host_t *ep = xfs_iext_get_ext(ifp, i);
+ if (!ep)
+ return -EFSCORRUPTED;
ep->l0 = get_unaligned_be64(&dp->l0);
ep->l1 = get_unaligned_be64(&dp->l1);
}
@@ -890,6 +892,8 @@ xfs_iext_get_ext(
xfs_extnum_t page_idx = idx; /* ext index in target list */
erp = xfs_iext_idx_to_irec(ifp, &page_idx, &erp_idx, 0);
+ if (!erp)
+ return NULL;
return &erp->er_extbuf[page_idx];
} else if (ifp->if_bytes) {
return &ifp->if_u1.if_extents[idx];
--
1.7.9.5
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 10/11] xfsprogs: fix possible null pointer dereference in xfs_iformat_extents
2015-12-02 11:19 ` [PATCH 10/11] xfsprogs: fix possible null pointer dereference in xfs_iformat_extents Vivek Trivedi
@ 2015-12-03 6:31 ` Dave Chinner
0 siblings, 0 replies; 30+ messages in thread
From: Dave Chinner @ 2015-12-03 6:31 UTC (permalink / raw)
To: Vivek Trivedi; +Cc: a.sahrawat, pankaj.m, xfs
On Wed, Dec 02, 2015 at 04:49:26PM +0530, Vivek Trivedi wrote:
> Fix possible null pointer dereference in xfs_iformat_extents and
> xfs_iext_get_ext if fail to locate inode record.
> Reported by coverity.
ignore it. Code
>
> Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
> ---
> libxfs/xfs_inode_fork.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/libxfs/xfs_inode_fork.c b/libxfs/xfs_inode_fork.c
> index e1968b4..36aa0c8 100644
> --- a/libxfs/xfs_inode_fork.c
> +++ b/libxfs/xfs_inode_fork.c
> @@ -331,6 +331,8 @@ xfs_iformat_extents(
> xfs_validate_extents(ifp, nex, XFS_EXTFMT_INODE(ip));
> for (i = 0; i < nex; i++, dp++) {
> xfs_bmbt_rec_host_t *ep = xfs_iext_get_ext(ifp, i);
> + if (!ep)
> + return -EFSCORRUPTED;
> ep->l0 = get_unaligned_be64(&dp->l0);
> ep->l1 = get_unaligned_be64(&dp->l1);
Can't possibly happen, as nex is a count of the number of entries in
the extent list and so we will never overrun the list here.
> @@ -890,6 +892,8 @@ xfs_iext_get_ext(
> xfs_extnum_t page_idx = idx; /* ext index in target list */
>
> erp = xfs_iext_idx_to_irec(ifp, &page_idx, &erp_idx, 0);
> + if (!erp)
> + return NULL;
Same again - this is guaranteed to succeed because the indexes
passed into the function are bound to within range by the caller.
The ASSERT()s at the beginning of the function enforce this....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 11/11] xfsprogs: xfs_repair: fix possible null pointer dereference in mark_standalone_inodes
2015-12-02 11:19 [PATCH 00/11] xfsprogs misc fixes Vivek Trivedi
` (9 preceding siblings ...)
2015-12-02 11:19 ` [PATCH 10/11] xfsprogs: fix possible null pointer dereference in xfs_iformat_extents Vivek Trivedi
@ 2015-12-02 11:19 ` Vivek Trivedi
2015-12-03 6:34 ` Dave Chinner
2015-12-02 15:10 ` [PATCH 00/11] xfsprogs misc fixes Eric Sandeen
11 siblings, 1 reply; 30+ messages in thread
From: Vivek Trivedi @ 2015-12-02 11:19 UTC (permalink / raw)
To: xfs; +Cc: Vivek Trivedi, a.sahrawat, pankaj.m
Fix possible null pointer dereference in mark_standalone_inodes by
rearranging and adding ASSERT for null irec.
Reported by coverity.
Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
---
repair/phase6.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/repair/phase6.c b/repair/phase6.c
index e41bf20..1e5fc46 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -3088,11 +3088,11 @@ mark_standalone_inodes(xfs_mount_t *mp)
irec = find_inode_rec(mp, XFS_INO_TO_AGNO(mp, mp->m_sb.sb_rsumino),
XFS_INO_TO_AGINO(mp, mp->m_sb.sb_rsumino));
+ ASSERT(irec != NULL);
+
offset = XFS_INO_TO_AGINO(mp, mp->m_sb.sb_rsumino) -
irec->ino_startnum;
- ASSERT(irec != NULL);
-
add_inode_reached(irec, offset);
if (fs_quotas) {
@@ -3101,6 +3101,7 @@ mark_standalone_inodes(xfs_mount_t *mp)
irec = find_inode_rec(mp, XFS_INO_TO_AGNO(mp,
mp->m_sb.sb_uquotino),
XFS_INO_TO_AGINO(mp, mp->m_sb.sb_uquotino));
+ ASSERT(irec != NULL);
offset = XFS_INO_TO_AGINO(mp, mp->m_sb.sb_uquotino)
- irec->ino_startnum;
add_inode_reached(irec, offset);
@@ -3110,6 +3111,7 @@ mark_standalone_inodes(xfs_mount_t *mp)
irec = find_inode_rec(mp, XFS_INO_TO_AGNO(mp,
mp->m_sb.sb_gquotino),
XFS_INO_TO_AGINO(mp, mp->m_sb.sb_gquotino));
+ ASSERT(irec != NULL);
offset = XFS_INO_TO_AGINO(mp, mp->m_sb.sb_gquotino)
- irec->ino_startnum;
add_inode_reached(irec, offset);
@@ -3119,6 +3121,7 @@ mark_standalone_inodes(xfs_mount_t *mp)
irec = find_inode_rec(mp, XFS_INO_TO_AGNO(mp,
mp->m_sb.sb_pquotino),
XFS_INO_TO_AGINO(mp, mp->m_sb.sb_pquotino));
+ ASSERT(irec != NULL);
offset = XFS_INO_TO_AGINO(mp, mp->m_sb.sb_pquotino)
- irec->ino_startnum;
add_inode_reached(irec, offset);
--
1.7.9.5
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 00/11] xfsprogs misc fixes
2015-12-02 11:19 [PATCH 00/11] xfsprogs misc fixes Vivek Trivedi
` (10 preceding siblings ...)
2015-12-02 11:19 ` [PATCH 11/11] xfsprogs: xfs_repair: fix possible null pointer dereference in mark_standalone_inodes Vivek Trivedi
@ 2015-12-02 15:10 ` Eric Sandeen
11 siblings, 0 replies; 30+ messages in thread
From: Eric Sandeen @ 2015-12-02 15:10 UTC (permalink / raw)
To: xfs
Thanks, I'll help look over these.
Could you please map the coverity CIDs to these fixes?
We like to include that in the commit metadata.
Thanks,
-Eric
On 12/2/15 5:19 AM, Vivek Trivedi wrote:
> minor fixes in xfsprogs, mostly coverity reported fixes.
>
> Vivek Trivedi (11):
> xfsprogs: xfs_io: fix a memory leak in imap_f
> xfsprogs: fix integer overflow in xlog_find_verify_cycle
> xfsprogs: mkfs: fix unintentional integer overflow
> xfsprogs: xfsdb: remove unnessary checks in
> process_leaf_node_dir_v2_free
> xfsprogs: xfs_mdrestore: check bad read count in perform_restore
> xfsprogs: xfs_db: check null derefernce after block_to_bt
> xfsprogs: xfs_fsr: replace sprintf with snprintf to avoid buffer
> overflow
> xfsprogs: xfs_repair: fix possible null dereference in build_ino_tree
> xfsprogs: xfs_repair: fix possible null dereference in
> traverse_int_dir2block
> xfsprogs: fix possible null pointer dereference in
> xfs_iformat_extents
> xfsprogs: xfs_repair: fix possible null pointer dereference in
> mark_standalone_inodes
>
> db/btblock.c | 3 +++
> db/check.c | 2 --
> fsr/xfs_fsr.c | 4 ++--
> io/imap.c | 7 ++++++-
> libxfs/xfs_inode_fork.c | 4 ++++
> libxlog/xfs_log_recover.c | 2 +-
> mdrestore/xfs_mdrestore.c | 4 ++++
> mkfs/xfs_mkfs.c | 2 +-
> repair/dir2.c | 2 +-
> repair/phase5.c | 2 +-
> repair/phase6.c | 7 +++++--
> 11 files changed, 28 insertions(+), 11 deletions(-)
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread