[PATCH v3] blkparse: Fix a potential coredump issue

[PATCH v3] blkparse: Fix a potential coredump issue

[303146950]

From: Kou Wenqi <kouwenqi@kylinos.cn>

Executing "blkparse -t" may cause coredump due to
accessing uninitialized memory.

8,16   0     6092     1.436280373 20685  Q   W 15356912 + 4096 [kworker/u256:2]
8,16   0     6093     1.436282093 20685  X   W 15356912 / 15357936 [kworker/u256:2]
8,16   0     6094     1.436282735 20685  G   W 15356912 + 1024 [kworker/u256:2]
8,16   0     6099     1.436358617 20685  X   W 15357936 / 15358960 [kworker/u256:2]
8,16   0     6100     1.436359418 20685  G   W 15357936 + 1024 [kworker/u256:2]
8,16   0     6102     1.436359931 20685  I   W 15356912 + 1024 [kworker/u256:2]
8,16   0     6103     1.436360874 20685  D   W 15356912 + 1024 [kworker/u256:2]
8,16   0     6105     1.436436572 20685  X   W 15358960 / 15359984 [kworker/u256:2]
8,16   0     6106     1.436437679 20685  G   W 15358960 + 1024 [kworker/u256:2]
8,16   0     6108     1.436438205 20685  I   W 15357936 + 1024 [kworker/u256:2]
8,16   0     6109     1.436439368 20685  D   W 15357936 + 1024 [kworker/u256:2]
8,16   0     6111     1.436530404 20685  G   W 15359984 + 1024 [kworker/u256:2]
8,16   0     6113     1.436530842 20685  I   W 15358960 + 1024 [kworker/u256:2]
8,16   0     6114     1.436531748 20685  D   W 15358960 + 1024 [kworker/u256:2]
8,16   0     6120     1.436755261 20685  I   W 15359984 + 1024 [kworker/u256:2]
8,16   0     6121     1.436756476 20685  D   W 15359984 + 1024 [kworker/u256:2]
8,16   0     6128     1.437120354     0  C   W 15356912 + 1024 [0]
8,16   0     6129     1.437121875     0  C   W 15356912 + 2048 [0]

Breakpoint 3, log_track_split (pdi=0x430260, t=0x711990) at blkparse.c:1076
1076            iot = find_track(pdi, t->pid, t->sector);
(gdb) n
1077            split = malloc(sizeof(*iot));
(gdb)
1078            split->req = iot->req;
(gdb) p split
$1 = (struct io_track *) 0x69bdd0
(gdb) p *split
$2 = {rb_node = {rb_parent_color = 6929360, rb_right = 0x0, rb_left = 0xe81bf0}, req = 0x2000d00080000, next = 0x800010000050cd, sector = 2251799813685248}

Program received signal SIGSEGV, Segmentation fault.
0x000000000040bcbc in rb_set_parent (rb=0xe81bf0, p=0x69bbd0) at rbtree.h:133
133             rb->rb_parent_color = (rb->rb_parent_color & 3) | (unsigned long)p;
(gdb) bt
 #0  0x000000000040bcbc in rb_set_parent (rb=0xe81bf0, p=0x69bbd0) at rbtree.h:133
 #1  0x000000000040c724 in rb_erase (node=0x69bdd0, root=0x430418) at rbtree.c:273
 #2  0x0000000000404810 in log_track_complete (pdi=0x430260, t=0x712810) at blkparse.c:1223
 #3  0x00000000004055e4 in log_complete (pdi=0x430260, pci=0x4bc100, t=0x712810, act=0x40cf60 "C") at blkparse.c:1518
 #4  0x0000000000405ea0 in dump_trace_fs (t=0x712810, pdi=0x430260, pci=0x4bc100) at blkparse.c:1680
 #5  0x000000000040604c in dump_trace (t=0x712810, pci=0x4bc100, pdi=0x430260) at blkparse.c:1722
 #6  0x00000000004090e8 in handle (msp=0x433cf0) at blkparse.c:2639
 #7  0x000000000040931c in do_file () at blkparse.c:2712
 #8  0x000000000040a0cc in main (argc=4, argv=0xfffffffff438) at blkparse.c:3045
(gdb) p rb
$3 = (struct rb_node *) 0xe81bf0
(gdb) p *rb
Cannot access memory at address 0xe81bf0
(gdb) up
273                     rb_set_parent(child, parent);
(gdb) p child
$4 = (struct rb_node *) 0xe81bf0
(gdb) p node
$5 = (struct rb_node *) 0x69bdd0
(gdb) p *node
$6 = {rb_parent_color = 6929360, rb_right = 0x0, rb_left = 0xe81bf0}

Signed-off-by: Kou Wenqi <kouwenqi@kylinos.cn>
---
 blkparse.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/blkparse.c b/blkparse.c
index 9d2029a..d6aaa8b 100644
--- a/blkparse.c
+++ b/blkparse.c
@@ -1022,7 +1022,7 @@ static struct io_track *find_track(struct per_dev_info *pdi, pid_t pid,
 	if (!iot) {
 		struct io_track_req *req;
 
-		req = malloc(sizeof(*req) + sizeof(*iot));
+		req = calloc(1, sizeof(*req) + sizeof(*iot));
 		req->ppm = find_ppm(pid);
 		if (!req->ppm)
 			req->ppm = add_ppm_hash(pid, "unknown");
@@ -1106,7 +1106,7 @@ static void log_track_split(struct per_dev_info *pdi, struct blk_io_trace *t)
 	 * parts.
 	 */
 	iot = find_track(pdi, t->pid, t->sector);
-	split = malloc(sizeof(*iot));
+	split = calloc(1, sizeof(*iot));
 	split->req = iot->req;
 	split->next = iot->next;
 	iot->next = split;
-- 
2.43.0

Re: [PATCH v3] blkparse: Fix a potential coredump issue

[Jens Axboe]

On 3/19/25 7:02 AM, 303146950@qq.com wrote:
> diff --git a/blkparse.c b/blkparse.c
> index 9d2029a..d6aaa8b 100644
> --- a/blkparse.c
> +++ b/blkparse.c
> @@ -1022,7 +1022,7 @@ static struct io_track *find_track(struct per_dev_info *pdi, pid_t pid,
>  	if (!iot) {
>  		struct io_track_req *req;
>  
> -		req = malloc(sizeof(*req) + sizeof(*iot));
> +		req = calloc(1, sizeof(*req) + sizeof(*iot));
>  		req->ppm = find_ppm(pid);
>  		if (!req->ppm)
>  			req->ppm = add_ppm_hash(pid, "unknown");
> @@ -1106,7 +1106,7 @@ static void log_track_split(struct per_dev_info *pdi, struct blk_io_trace *t)
>  	 * parts.
>  	 */
>  	iot = find_track(pdi, t->pid, t->sector);
> -	split = malloc(sizeof(*iot));
> +	split = calloc(1, sizeof(*iot));
>  	split->req = iot->req;
>  	split->next = iot->next;
>  	iot->next = split;

Nit picking, but the idiomatic way would be to make it calloc(size, 1)
not the other way around. The first one is the size, the 2nd one is the
number of them.

-- 
Jens Axboe

Re: [PATCH v3] blkparse: Fix a potential coredump issue

[303146950]

From: Kou Wenqi <kouwenqi@kylinos.cn>

On 3/19/25 7:10 AM, axboe@kernel.dk wrote:
> Nit picking, but the idiomatic way would be to make it calloc(size, 1)
> not the other way around. The first one is the size, the 2nd one is the
> number of them.

Thank you for reviewing the patch!
I wanted to double-check the calloc usage
to ensure alignment with the project's conventions. 
The man page defines calloc as:
void *calloc(size_t nmemb, size_t size); 
where nmemb is the number of elements, and size is the size of each element.
In the submitted code, 
split = calloc(1, sizeof(*iot)) follows this order: nmemb=1 and size=sizeof(...). 
I also reviewed existing code in blktrace and saw consistent usage like calloc(1, size), 
which matches the man page’s parameter order.
Could you clarify if there’s a specific reason or project-specific idiom
for reversing the parameters (e.g., calloc(size, 1) instead)?
I’m happy to adjust it if there’s a stylistic preference or guideline I’ve overlooked.
Thanks again for your guidance!

-- 
Kou Wenqi

Re: [PATCH v3] blkparse: Fix a potential coredump issue

[Jens Axboe]

On 3/19/25 7:49 PM, 303146950@qq.com wrote:
> From: Kou Wenqi <kouwenqi@kylinos.cn>
> 
> On 3/19/25 7:10 AM, axboe@kernel.dk wrote:
>> Nit picking, but the idiomatic way would be to make it calloc(size, 1)
>> not the other way around. The first one is the size, the 2nd one is the
>> number of them.
> 
> Thank you for reviewing the patch!
> I wanted to double-check the calloc usage
> to ensure alignment with the project's conventions. 
> The man page defines calloc as:
> void *calloc(size_t nmemb, size_t size); 
> where nmemb is the number of elements, and size is the size of each element.
> In the submitted code, 
> split = calloc(1, sizeof(*iot)) follows this order: nmemb=1 and size=sizeof(...). 
> I also reviewed existing code in blktrace and saw consistent usage like calloc(1, size), 
> which matches the man page’s parameter order.
> Could you clarify if there’s a specific reason or project-specific idiom
> for reversing the parameters (e.g., calloc(size, 1) instead)?
> I’m happy to adjust it if there’s a stylistic preference or guideline I’ve overlooked.
> Thanks again for your guidance!

You are right, I was apparently smoking crack there for a second!

-- 
Jens Axboe

Re: [PATCH v3] blkparse: Fix a potential coredump issue

[Jens Axboe]

On Wed, 19 Mar 2025 21:02:49 +0800, 303146950@qq.com wrote:
> Executing "blkparse -t" may cause coredump due to
> accessing uninitialized memory.
> 
> 8,16   0     6092     1.436280373 20685  Q   W 15356912 + 4096 [kworker/u256:2]
> 8,16   0     6093     1.436282093 20685  X   W 15356912 / 15357936 [kworker/u256:2]
> 8,16   0     6094     1.436282735 20685  G   W 15356912 + 1024 [kworker/u256:2]
> 8,16   0     6099     1.436358617 20685  X   W 15357936 / 15358960 [kworker/u256:2]
> 8,16   0     6100     1.436359418 20685  G   W 15357936 + 1024 [kworker/u256:2]
> 8,16   0     6102     1.436359931 20685  I   W 15356912 + 1024 [kworker/u256:2]
> 8,16   0     6103     1.436360874 20685  D   W 15356912 + 1024 [kworker/u256:2]
> 8,16   0     6105     1.436436572 20685  X   W 15358960 / 15359984 [kworker/u256:2]
> 8,16   0     6106     1.436437679 20685  G   W 15358960 + 1024 [kworker/u256:2]
> 8,16   0     6108     1.436438205 20685  I   W 15357936 + 1024 [kworker/u256:2]
> 8,16   0     6109     1.436439368 20685  D   W 15357936 + 1024 [kworker/u256:2]
> 8,16   0     6111     1.436530404 20685  G   W 15359984 + 1024 [kworker/u256:2]
> 8,16   0     6113     1.436530842 20685  I   W 15358960 + 1024 [kworker/u256:2]
> 8,16   0     6114     1.436531748 20685  D   W 15358960 + 1024 [kworker/u256:2]
> 8,16   0     6120     1.436755261 20685  I   W 15359984 + 1024 [kworker/u256:2]
> 8,16   0     6121     1.436756476 20685  D   W 15359984 + 1024 [kworker/u256:2]
> 8,16   0     6128     1.437120354     0  C   W 15356912 + 1024 [0]
> 8,16   0     6129     1.437121875     0  C   W 15356912 + 2048 [0]
> 
> [...]

Applied, thanks!

[1/1] blkparse: Fix a potential coredump issue
      commit: f9bd00dfbd67ce62ca6df6f55d6275b523cd0b39

Best regards,
-- 
Jens Axboe