* [bug report] lib: Add zstd modules
@ 2017-09-26 20:05 Dan Carpenter
2017-09-26 20:08 ` Dan Carpenter
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Dan Carpenter @ 2017-09-26 20:05 UTC (permalink / raw)
To: kernel-janitors
Hello Nick Terrell,
The patch 73f3d1b48f50: "lib: Add zstd modules" from Aug 9, 2017,
leads to the following static checker warning:
lib/zstd/zstd_opt.h:547 ZSTD_compressBlock_opt_generic()
error: buffer overflow 'opt[cur - mlen].rep' 3 <= 3
lib/zstd/zstd_opt.h
537
538 mlen = opt[cur].mlen;
539 if (opt[cur].off > ZSTD_REP_MOVE_OPT) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The checker is complaining that assume "opt[cur].off = ZSTD_REP_MOVE_OPT".
540 opt[cur].rep[2] = opt[cur - mlen].rep[1];
541 opt[cur].rep[1] = opt[cur - mlen].rep[0];
542 opt[cur].rep[0] = opt[cur].off - ZSTD_REP_MOVE_OPT;
543 } else {
544 opt[cur].rep[2] = (opt[cur].off > 1) ? opt[cur - mlen].rep[1] : opt[cur - mlen].rep[2];
545 opt[cur].rep[1] = (opt[cur].off > 0) ? opt[cur - mlen].rep[0] : opt[cur - mlen].rep[1];
546 opt[cur].rep[0] 547 ((opt[cur].off = ZSTD_REP_MOVE_OPT) && (mlen != 1)) ? (opt[cur - mlen].rep[0] - 1) : (opt[cur - mlen].rep[opt[cur].off]);
^^^^^^^^^^^^^^^^^
also we have to assume "mlen = 1" then opt[cur - mlen].rep[opt[cur].off]
is reading one element beyond the end of the array. It's possible that
both conditions can't be true but static analysis tools get annoyed when
we have impossible conditions.
548 }
549
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] lib: Add zstd modules
2017-09-26 20:05 [bug report] lib: Add zstd modules Dan Carpenter
@ 2017-09-26 20:08 ` Dan Carpenter
2017-09-26 21:54 ` Nick Terrell
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2017-09-26 20:08 UTC (permalink / raw)
To: kernel-janitors
Also a few lines later:
lib/zstd/zstd_opt.h:855 ZSTD_compressBlock_opt_extDict_generic()
error: buffer overflow 'opt[cur - mlen].rep' 3 <= 3
regards,
dan carpenter
On Tue, Sep 26, 2017 at 11:05:20PM +0300, Dan Carpenter wrote:
> Hello Nick Terrell,
>
> The patch 73f3d1b48f50: "lib: Add zstd modules" from Aug 9, 2017,
> leads to the following static checker warning:
>
> lib/zstd/zstd_opt.h:547 ZSTD_compressBlock_opt_generic()
> error: buffer overflow 'opt[cur - mlen].rep' 3 <= 3
>
> lib/zstd/zstd_opt.h
> 537
> 538 mlen = opt[cur].mlen;
> 539 if (opt[cur].off > ZSTD_REP_MOVE_OPT) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> The checker is complaining that assume "opt[cur].off = ZSTD_REP_MOVE_OPT".
>
> 540 opt[cur].rep[2] = opt[cur - mlen].rep[1];
> 541 opt[cur].rep[1] = opt[cur - mlen].rep[0];
> 542 opt[cur].rep[0] = opt[cur].off - ZSTD_REP_MOVE_OPT;
> 543 } else {
> 544 opt[cur].rep[2] = (opt[cur].off > 1) ? opt[cur - mlen].rep[1] : opt[cur - mlen].rep[2];
> 545 opt[cur].rep[1] = (opt[cur].off > 0) ? opt[cur - mlen].rep[0] : opt[cur - mlen].rep[1];
> 546 opt[cur].rep[0] > 547 ((opt[cur].off = ZSTD_REP_MOVE_OPT) && (mlen != 1)) ? (opt[cur - mlen].rep[0] - 1) : (opt[cur - mlen].rep[opt[cur].off]);
> ^^^^^^^^^^^^^^^^^
> also we have to assume "mlen = 1" then opt[cur - mlen].rep[opt[cur].off]
> is reading one element beyond the end of the array. It's possible that
> both conditions can't be true but static analysis tools get annoyed when
> we have impossible conditions.
>
> 548 }
> 549
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] lib: Add zstd modules
2017-09-26 20:05 [bug report] lib: Add zstd modules Dan Carpenter
2017-09-26 20:08 ` Dan Carpenter
@ 2017-09-26 21:54 ` Nick Terrell
2017-09-27 23:54 ` Nick Terrell
2017-09-28 9:33 ` Dan Carpenter
3 siblings, 0 replies; 5+ messages in thread
From: Nick Terrell @ 2017-09-26 21:54 UTC (permalink / raw)
To: kernel-janitors
On 9/26/17, 1:06 PM, "Dan Carpenter" <dan.carpenter@oracle.com> wrote:
> Hello Nick Terrell,
>
> The patch 73f3d1b48f50: "lib: Add zstd modules" from Aug 9, 2017,
> leads to the following static checker warning:
>
> lib/zstd/zstd_opt.h:547 ZSTD_compressBlock_opt_generic()
> error: buffer overflow 'opt[cur - mlen].rep' 3 <= 3
>
> lib/zstd/zstd_opt.h
> 537
> 538 mlen = opt[cur].mlen;
> 539 if (opt[cur].off > ZSTD_REP_MOVE_OPT) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> The checker is complaining that assume "opt[cur].off = ZSTD_REP_MOVE_OPT".
>
> 540 opt[cur].rep[2] = opt[cur - mlen].rep[1];
> 541 opt[cur].rep[1] = opt[cur - mlen].rep[0];
> 542 opt[cur].rep[0] = opt[cur].off - ZSTD_REP_MOVE_OPT;
> 543 } else {
> 544 opt[cur].rep[2] = (opt[cur].off > 1) ? opt[cur - mlen].rep[1] : opt[cur - mlen].rep[2];
> 545 opt[cur].rep[1] = (opt[cur].off > 0) ? opt[cur - mlen].rep[0] : opt[cur - mlen].rep[1];
> 546 opt[cur].rep[0] > 547 ((opt[cur].off = ZSTD_REP_MOVE_OPT) && (mlen != 1)) ? (opt[cur - mlen].rep[0] - 1) : (opt[cur - mlen].rep[opt[cur].off]);
> ^^^^^^^^^^^^^^^^^
> also we have to assume "mlen = 1" then opt[cur - mlen].rep[opt[cur].off]
> is reading one element beyond the end of the array. It's possible that
> both conditions can't be true but static analysis tools get annoyed when
> we have impossible conditions.
Thanks for the report! I'm not certain if both conditions can be true or
not. I'll investigate tomorrow and post an update, with a patch if
necessary.
>
> 548 }
> 549
>
> regards,
> dan carpenter
Best,
Nick Terrell
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] lib: Add zstd modules
2017-09-26 20:05 [bug report] lib: Add zstd modules Dan Carpenter
2017-09-26 20:08 ` Dan Carpenter
2017-09-26 21:54 ` Nick Terrell
@ 2017-09-27 23:54 ` Nick Terrell
2017-09-28 9:33 ` Dan Carpenter
3 siblings, 0 replies; 5+ messages in thread
From: Nick Terrell @ 2017-09-27 23:54 UTC (permalink / raw)
To: kernel-janitors
On 9/26/17, 1:06 PM, "Dan Carpenter" <dan.carpenter@oracle.com> wrote:
> Hello Nick Terrell,
>
> The patch 73f3d1b48f50: "lib: Add zstd modules" from Aug 9, 2017,
> leads to the following static checker warning:
>
> lib/zstd/zstd_opt.h:547 ZSTD_compressBlock_opt_generic()
> error: buffer overflow 'opt[cur - mlen].rep' 3 <= 3
>
> lib/zstd/zstd_opt.h
> 537
> 538 mlen = opt[cur].mlen;
> 539 if (opt[cur].off > ZSTD_REP_MOVE_OPT) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> The checker is complaining that assume "opt[cur].off = ZSTD_REP_MOVE_OPT".
>
> 540 opt[cur].rep[2] = opt[cur - mlen].rep[1];
> 541 opt[cur].rep[1] = opt[cur - mlen].rep[0];
> 542 opt[cur].rep[0] = opt[cur].off - ZSTD_REP_MOVE_OPT;
> 543 } else {
> 544 opt[cur].rep[2] = (opt[cur].off > 1) ? opt[cur - mlen].rep[1] : opt[cur - mlen].rep[2];
> 545 opt[cur].rep[1] = (opt[cur].off > 0) ? opt[cur - mlen].rep[0] : opt[cur - mlen].rep[1];
> 546 opt[cur].rep[0] > 547 ((opt[cur].off = ZSTD_REP_MOVE_OPT) && (mlen != 1)) ? (opt[cur - mlen].rep[0] - 1) : (opt[cur - mlen].rep[opt[cur].off]);
> ^^^^^^^^^^^^^^^^^
> also we have to assume "mlen = 1" then opt[cur - mlen].rep[opt[cur].off]
> is reading one element beyond the end of the array. It's possible that
> both conditions can't be true but static analysis tools get annoyed when
> we have impossible conditions.
I've looked into the code. It is impossible for both `mlen = 1' and
`opt[cur].off = ZSTD_REP_MOVE_OPT' to be true.
`mlen = 1' means that the previous byte will be encoded as a literal.
`opt[cur].off = ZSTD_REP_MOVE_OPT' is only used when the current position
is directly after a match, meaning that the previous byte is encoded as
match, not a literal.
The second part of the condition `mlen != 1' will always be true and can be
deleted. I deleted it in upstream zstd [1]. Since it is functionally
equivalent, I'm not sure if it is worthwhile to immediately port the change
to the kernel, but I can if you like.
[1] https://github.com/facebook/zstd/pull/871
>
> 548 }
> 549
>
> regards,
> dan carpenter
Best,
Nick Terrell
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] lib: Add zstd modules
2017-09-26 20:05 [bug report] lib: Add zstd modules Dan Carpenter
` (2 preceding siblings ...)
2017-09-27 23:54 ` Nick Terrell
@ 2017-09-28 9:33 ` Dan Carpenter
3 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2017-09-28 9:33 UTC (permalink / raw)
To: kernel-janitors
If it's not a real bug, the there is no need/rush to do anything. Do as
you see fit.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-09-28 9:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-26 20:05 [bug report] lib: Add zstd modules Dan Carpenter
2017-09-26 20:08 ` Dan Carpenter
2017-09-26 21:54 ` Nick Terrell
2017-09-27 23:54 ` Nick Terrell
2017-09-28 9:33 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.