Re: [FFmpeg-devel] [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more precise diagnostics, except for libformat
On Tue, May 14, 2019 at 6:48 PM myp...@gmail.com wrote: > > On Wed, May 15, 2019 at 7:01 AM Hendrik Leppkes wrote: > > > > On Tue, May 14, 2019 at 11:25 PM Adam Richter > > wrote: > > > > > > Consider, for example, if you agree that columnization makes this range > > > check > > > more recognizable in a glance and makes it easier to spot what the bounds > > > are > > > (the sixteen space indentation is taken from the code in which it > > > appeared): > > > > > > av_assert0(par->bits_per_coded_sample >= 0 && > > > par->bits_per_coded_sample <= 8); > > > > > > ...vs... > > > > > > av_assert0(par->bits_per_coded_sample >= 0); > > > av_assert0(par->bits_per_coded_sample <= 8); > > > > > > A possible counter-argument to this might be that, in a long sequence > > > of assertions, can be informative to group related assertions > > > together, which I think is true, but it is possible to get this other > > > means, such as by using blank lines to separate express such grouping. > > > > > > So, Mark, if you decide you are OK with my complete patches, I encourage > > > you to let me know. Otherwise, if there are any of those changes which > > > you > > > are OK with, I would like to just to to get those merged. Finally, if > > > you would > > > rather see none of those changes merged (one one to split the assertions > > > in > > > libavformat and one was for everywhere else in ffmpeg), please let me know > > > about that too, in which case, if no one advocates for their > > > inclusion, I'll drop > > > my proposal to merge these changes. > > > > > > > Unfortunately I have to agree with Mark. asserst that check the same > > value or extremely closely related values should stay combined. > > > I agree this part I am trying to understand what problem you see with this. It occurs to me that you might be concerned about generating less efficient code for the common assert success case, in particular, in the example I showed for readability, potentially dereferencing "par" twice, but I made a test program (attached) and determined from reading the generated assembly that at least for gcc with optimization -O2 on x86_64, x86_32, aarch64 and arm32, as long as the abort function has "__attribute__ ((noreturn))", the compiler seems to be able to avoid repeating the memory fetch for the second assertion. I also check this for clang -O2 on on x86_64. Of course, without the noreturn attribute on the abort function, the compiler realizes that the abort function could have written to memory, so it refetches the value in the split assertion case, which I think might have been your concern, but as long as the abort function is declared with an attribute noreturn, we should be OK for gcc. On the other hand, I'm not sure what compilers people use for other platforms such as Microsoft Windows and if you tell me that it is a known problem on a specific platform that is potentially relevant to ffmpeg, that would probably change my mind. Of course, it's not necessary for you change my mind or for you to invest further time in this discussion, as I imagine you and other participants have other things to do. So, if I don't get a further explanation, I may still believe that you're wrong, but I'll respect your need to prioritize tasks other than continuing this discussion, and will not expect to see my proposed change merged unless the predominant opinion of others in this discussion changes to being in favor it, which, so far, I acknowledge, it is not. > > > > > > Also after this, I may take a look at adding a branch hint to the > > > av_assertN > > > macros if nobody objects. > > > > > > > Please don't, we don't do branch hints anywhere, and this is a bad > > place to start. > > If an assert is truely performance sensitive (as in, it makes a > > measurable difference), it should probably be moved from assert0 to > > assert1, and as such is only enabled in testing builds. > > > If assert0 or assert1 lead to performance drop, we need some profiling > data, then try to fix it. The above comments by Hendrick and you does not identify a cost to adding a branch hint to assert. There would be a downside in the form of a few lines of code complexity in libavutil/avassert.h. If that is your concern, I guess our disagreement is that I see reducing the cost of assert so that perhaps CPU usage with and without would be a tiny bit more similar for reproducing pr
Re: [FFmpeg-devel] [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more precise diagnostics, except for libformat
On Sun, May 12, 2019 at 11:16 AM Michael Niedermayer
wrote:
>
> On Sun, May 12, 2019 at 05:58:50PM +0100, Mark Thompson wrote:
> > On 12/05/2019 16:24, Adam Richter wrote:
> > > This patch separates statements of the form "assert(a && b);" into
> > > "assert(a);" and "assert(b);", typically involving an assertion
> > > function like av_assert0.
> > >
> > > This patch covers all of ffmpeg, except for the libavformat, which I
> > > have already submitted separately.
> > >
> > > I have not tested this patch other than observing that ffmpeg still
> > > builds without any apparent new complaints, that no complaints in the
> > > build contain "assert", and that "make fate" seems to succeed.
> > >
> > > Thanks in advance for considering the attached patch.
> > >
> > > Adam
> > >
> > >
> > > From f815a2481a19cfd191b9f97e246b307b71d6c790 Mon Sep 17 00:00:00 2001
> > > From: Adam Richter
> > > Date: Sun, 12 May 2019 08:02:51 -0700
> > > Subject: [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more
> > > precise diagnostics, except for libformat.
> > >
> > > This patch separates statements of the form "assert(a && b);" into
> > > "assert(a);" and "assert(b);", for more precise diagnostics when
> > > an assertion fails, which can be especially important in cases where
> > > the problem may not be easily reproducible and save developer time.
> > > Usually, this involves functions like av_assert0.
> >
> > I don't feel very convinced by the general case of this argument.
> > Assertions are primarily present to assist a developer reading/writing the
> > code; they should never be triggering at runtime in non-development
> > contexts.
> >
> > Where the statements a and b are not related then yes, splitting them is a
> > good idea. But when it's something like a bounds check on one variable
> > ("av_assert0(A < n && n < B)", as appears quite a few times below) then I
> > think keeping it as one statement feels clearer. Similarly for highly
> > related conditions ("av_assert0(p && p->f)" or "av_assert0(x < width && y <
> > height)").
> >
> > > There are a couple of cases that this patch does not change:
> > > (1) assert conjunctions of the form assert(condition && "string literal
> > > to pass to the user as a helpful tip."), and
> > > (2) assert condjunctions where the first part contained a variable
> > > assignment that was used in the second part of the assertion and
> > > not outside the assert (so that the variable assignment be elided
> > > if the assertion checking omitted).
> > >
> > > This patch covers all of ffmpeg except for libavformat, which was
> > > covered in a patch that I previously submitted separately.
> > >
> > > These changes build without any new complaints that I noticed, and
> > > "make fate" succeeds, but I have not otherwise tested them.
> > >
> > > Signed-off-by: Adam Richter
> > > ...
> > > diff --git a/libavcodec/aacpsy.c b/libavcodec/aacpsy.c
> > > index fca692cb15..bef52e8b02 100644
> > > --- a/libavcodec/aacpsy.c
> > > +++ b/libavcodec/aacpsy.c
> > > @@ -504,9 +504,11 @@ static int calc_bit_demand(AacPsyContext *ctx, float
> > > pe, int bits, int size,
> > > fill_level = av_clipf((float)ctx->fill_level / size, clip_low,
> > > clip_high);
> > > clipped_pe = av_clipf(pe, ctx->pe.min, ctx->pe.max);
> > > bit_save = (fill_level + bitsave_add) * bitsave_slope;
> > > -assert(bit_save <= 0.3f && bit_save >= -0.0501f);
> > > +assert(bit_save <= 0.3f);
> > > +assert(bit_save >= -0.0501f);
> > > bit_spend = (fill_level + bitspend_add) * bitspend_slope;
> > > -assert(bit_spend <= 0.5f && bit_spend >= -0.1f);
> > > +assert(bit_spend <= 0.5f);
> > > +assert(bit_spend >= -0.1f);
> >
> > While you're touching calls to traditional assert() please consider turning
> > them into suitable av_assertN().
>
> I agree that they should be changed to av_assertN() but it should be
> in a seperate commit/patch
>
> These assert -> av_assertN changes will likely in some cases "activate&q
Re: [FFmpeg-devel] [PATCH] libswcale: Fix possible string overflow in test
Hi, Michael. Thanks for reviewing that possible string overflow found by cppcheck and proposing to try to make a better fix. I'll assume no further action on my part for this fix is necessary unless anyone tells me otherwise. Adam Adam On Mon, May 13, 2019 at 4:39 AM Michael Niedermayer wrote: > > On Sun, May 12, 2019 at 05:40:00AM -0700, Adam Richter wrote: > > This is a possible fix for a string overflow in some sscanf calls in > > libswcale/tests/swscale.c, in the function fileTest(), found by > > cppcheck. Please see the attachment for more discussion of this. > > > > Thanks in advance for considering this patch. > > > > Adam > > > swscale.c |4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > 337bfa52e3917c2d896ca5c7ba1b669d5970cdab > > 0002-libswcale-Fix-possible-string-overflow-in-test.patch > > From 8b5f994bcd2576588149f228695823b5cf8d3dc8 Mon Sep 17 00:00:00 2001 > > From: Adam Richter > > Date: Sun, 12 May 2019 05:03:25 -0700 > > Subject: [PATCH] libswcale: Fix possible string overflow in test. > > > > In libswcale/tests/swcale.c, the function fileTest() calls sscanf in > > an argument of "%12s" on character srcStr[] and dstStr[], which are > > only 12 bytes. So, if the input string is 12 characters, a > > terminating null byte can be written past the end of these arrays. > > > > This bug was found by cppcheck. > > > > I am not an ffmpeg or libswcale developer, and I believe that this is > > the first patch I am submitting to ffmpeg, so please let me know if > > I am doing anything wrong in the patch submission process. > > > > For the same reason, please examine this patch skeptically, especially > > considering that I have not tested this patch other than to see that > > it compiled without complaint and that "make fate" completed with a > > zero exit code. I do not know if this program actually > > expects these input strings to be a maximum of 11 or 12 characters long. > > In this patch, I assume that they could be 12 characters long, so I have > > extended the array sizes, but perhaps a more correct fix might > > be to change the "%12s" instances to "%11s" instead. > > > > Thanks in advance for considering this patch. > > I actually think 13 is not long enough for the longest name. > Ill fix it, thanks for finding this > > > [...] > > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Rewriting code that is poorly written but fully understood is good. > Rewriting code that one doesnt understand is a sign that one is less smart > then the original author, trying to rewrite it will not make it better. > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more precise diagnostics, except for libformat
This patch separates statements of the form "assert(a && b);" into "assert(a);" and "assert(b);", typically involving an assertion function like av_assert0. This patch covers all of ffmpeg, except for the libavformat, which I have already submitted separately. I have not tested this patch other than observing that ffmpeg still builds without any apparent new complaints, that no complaints in the build contain "assert", and that "make fate" seems to succeed. Thanks in advance for considering the attached patch. Adam From f815a2481a19cfd191b9f97e246b307b71d6c790 Mon Sep 17 00:00:00 2001 From: Adam Richter Date: Sun, 12 May 2019 08:02:51 -0700 Subject: [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more precise diagnostics, except for libformat. This patch separates statements of the form "assert(a && b);" into "assert(a);" and "assert(b);", for more precise diagnostics when an assertion fails, which can be especially important in cases where the problem may not be easily reproducible and save developer time. Usually, this involves functions like av_assert0. There are a couple of cases that this patch does not change: (1) assert conjunctions of the form assert(condition && "string literal to pass to the user as a helpful tip."), and (2) assert condjunctions where the first part contained a variable assignment that was used in the second part of the assertion and not outside the assert (so that the variable assignment be elided if the assertion checking omitted). This patch covers all of ffmpeg except for libavformat, which was covered in a patch that I previously submitted separately. These changes build without any new complaints that I noticed, and "make fate" succeeds, but I have not otherwise tested them. Signed-off-by: Adam Richter --- fftools/cmdutils.c| 3 +- fftools/ffmpeg.c | 3 +- libavcodec/4xm.c | 12 +--- libavcodec/aaccoder_twoloop.h | 3 +- libavcodec/aacenc.c | 3 +- libavcodec/aacenc_quantization_misc.h | 3 +- libavcodec/aacpsy.c | 6 ++-- libavcodec/ac3enc.c | 15 ++ libavcodec/acelp_filters.c| 3 +- libavcodec/amfenc.c | 5 ++-- libavcodec/amrnbdec.c | 3 +- libavcodec/av1_frame_split_bsf.c | 3 +- libavcodec/avpacket.c | 3 +- libavcodec/cbs.c | 42 ++- libavcodec/cbs_av1.c | 9 -- libavcodec/cbs_av1_syntax_template.c | 3 +- libavcodec/cbs_h2645.c| 10 --- libavcodec/cbs_mpeg2.c| 4 +-- libavcodec/cbs_vp9.c | 8 +++-- libavcodec/celp_filters.c | 3 +- libavcodec/dca_core.c | 3 +- libavcodec/decode.c | 4 +-- libavcodec/dvdsubdec.c| 3 +- libavcodec/dvenc.c| 3 +- libavcodec/dxva2_h264.c | 3 +- libavcodec/dxva2_hevc.c | 3 +- libavcodec/dxva2_vp9.c| 3 +- libavcodec/error_resilience.c | 3 +- libavcodec/ffv1dec.c | 3 +- libavcodec/flacenc.c | 6 ++-- libavcodec/get_bits.h | 35 +++--- libavcodec/h263dec.c | 3 +- libavcodec/h2645_parse.c | 6 ++-- libavcodec/h264_refs.c| 3 +- libavcodec/h264_slice.c | 3 +- libavcodec/h264chroma_template.c | 20 ++--- libavcodec/hevc_filter.c | 6 ++-- libavcodec/huffyuv.c | 3 +- libavcodec/huffyuvenc.c | 5 +++- libavcodec/ituh263enc.c | 3 +- libavcodec/ivi.c | 4 ++- libavcodec/jpeg2000dec.c | 3 +- libavcodec/lclenc.c | 3 +- libavcodec/lpc.c | 5 ++-- libavcodec/lzwenc.c | 9 -- libavcodec/mips/h264chroma_msa.c | 31 +++- libavcodec/mips/vc1dsp_mmi.c | 20 ++--- libavcodec/mjpegdec.c | 11 --- libavcodec/motion_est.c | 12 ++-- libavcodec/motion_est_template.c | 10 +-- libavcodec/mpeg12.c | 3 +- libavcodec/mpeg12enc.c| 6 ++-- libavcodec/mpeg4videoenc.c| 6 ++-- libavcodec/mpegaudiodec_template.c| 3 +- libavcodec/mpegaudioenc_template.c| 6 ++-- libavcodec/mpegutils.c| 6 ++-- libavcodec/mpegvideo_enc.c| 9 -- libavcodec/mpegvideo_xvmc.c | 3 +- libavcodec/mpegvideoencdsp.c | 3 +- libavcodec/mqcenc.c | 3 +- libavcodec/put_bits.h | 9 -- l
[FFmpeg-devel] [PATCH] libavformat: Separate assertions of the form "av_assertN(a && b)" to "av_assertN(a); av_assertN(b)" for more precise diagnostics.
This is the first of what I expect to be several patches to convert
assertions of the
form "assert(a && b)" to "assert(a); assert(b)".
Here are some reasons why I think this would be an improvement. This
lengthy argument is not included in the patch attachment.
1. Assertion failures are often sporadic, and users who report them may
not be in a position to efficiently narrow them down further, so it
is important to get as much precision from each assertion failure as
possible.
2. It is a more efficient use of developers time when a bug report
arrives if the problem has been narrowed down that much more. A
new bug report may initially attract more interest, so, if the
problem has been narrowed down that much more, it may increase the
chance that developers may invest the time to try to resolve the
problem, and also reduce unnecessary traffic on the developer mailing
list about possible causes of the bug that separating the assertion
was able to rule out.
3. It's often more readable, sometimes eliminating parentheses or
changing multi-line conditions to separate single line conditions.
4. When using a debugger to step over an assertion failure in the
first part of the statement, the second part is still tested.
5. Providing separate likelihood hints to the compiler in the form
of separate assert statements does not require the compiler to
be quite as smart to recognize that it should optimize both branches,
although I do not know if that makes a difference for any compiler
commonly used to compile X (that is, I suspect that they are all
smart enough to realize is that "a && b" is likely true, then "a"
is likely true and "b" is likely true).
I have confirmed that the resulting tree built without any apparent
complaints about the assert statements and that "make fate" completed
with a zero exit code. I have not done any other tests though.
Thanks in advance for considering this patch.
Adam
From edb58a5ee8030ec66c04736a025d2a44e7322ba3 Mon Sep 17 00:00:00 2001
From: Adam Richter
Date: Sun, 12 May 2019 03:41:49 -0700
Subject: [PATCH] libavformat: Separate assertions of the form
"av_assertN(a && b)" to "av_assertN(a); av_assertN(b)" for more precise
diagnostics.
Signed-off-by: Adam Richter
---
libavformat/au.c | 3 ++-
libavformat/avienc.c | 3 ++-
libavformat/aviobuf.c | 3 ++-
libavformat/matroskaenc.c | 6 --
libavformat/mov.c | 3 ++-
libavformat/rtmppkt.c | 3 ++-
libavformat/utils.c | 9 +
7 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/libavformat/au.c b/libavformat/au.c
index cb48e67feb..76f23de56d 100644
--- a/libavformat/au.c
+++ b/libavformat/au.c
@@ -177,7 +177,8 @@ static int au_read_header(AVFormatContext *s)
bps = 2;
} else {
const uint8_t bpcss[] = {4, 0, 3, 5};
-av_assert0(id >= 23 && id < 23 + 4);
+av_assert0(id >= 23);
+av_assert0(id < 23 + 4);
ba = bpcss[id - 23];
bps = bpcss[id - 23];
}
diff --git a/libavformat/avienc.c b/libavformat/avienc.c
index ac0f04c354..e96eb27e5e 100644
--- a/libavformat/avienc.c
+++ b/libavformat/avienc.c
@@ -797,7 +797,8 @@ static int avi_write_packet(AVFormatContext *s, AVPacket *pkt)
int pal_size = 1 << par->bits_per_coded_sample;
int pc_tag, i;
-av_assert0(par->bits_per_coded_sample >= 0 && par->bits_per_coded_sample <= 8);
+av_assert0(par->bits_per_coded_sample >= 0);
+av_assert0(par->bits_per_coded_sample <= 8);
if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && avist->pal_offset) {
int64_t cur_offset = avio_tell(pb);
diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index 5a33f82950..e71846e0e8 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -195,7 +195,8 @@ static void flush_buffer(AVIOContext *s)
void avio_w8(AVIOContext *s, int b)
{
-av_assert2(b>=-128 && b<=255);
+av_assert2(b >= -128);
+av_assert2(b <= 255);
*s->buf_ptr++ = b;
if (s->buf_ptr >= s->buf_end)
flush_buffer(s);
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index cef504fa05..51b6d1b16f 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -589,7 +589,8 @@ static int64_t mkv_write_cues(AVFormatContext *s, mkv_cues *cues, mkv_track *tra
tracks[j].has_cue = 0;
for (j = 0; j < cues->num_entries - i && entry[j].pts == pts; j++) {
int tracknum = entry[j].stream_idx;
-av_assert0(tracknum>=0 && tracknum= 0);
+av_assert0(tracknum < num_tracks);
[FFmpeg-devel] [PATCH] libswcale: Fix possible string overflow in test
This is a possible fix for a string overflow in some sscanf calls in
libswcale/tests/swscale.c, in the function fileTest(), found by
cppcheck. Please see the attachment for more discussion of this.
Thanks in advance for considering this patch.
Adam
From 8b5f994bcd2576588149f228695823b5cf8d3dc8 Mon Sep 17 00:00:00 2001
From: Adam Richter
Date: Sun, 12 May 2019 05:03:25 -0700
Subject: [PATCH] libswcale: Fix possible string overflow in test.
In libswcale/tests/swcale.c, the function fileTest() calls sscanf in
an argument of "%12s" on character srcStr[] and dstStr[], which are
only 12 bytes. So, if the input string is 12 characters, a
terminating null byte can be written past the end of these arrays.
This bug was found by cppcheck.
I am not an ffmpeg or libswcale developer, and I believe that this is
the first patch I am submitting to ffmpeg, so please let me know if
I am doing anything wrong in the patch submission process.
For the same reason, please examine this patch skeptically, especially
considering that I have not tested this patch other than to see that
it compiled without complaint and that "make fate" completed with a
zero exit code. I do not know if this program actually
expects these input strings to be a maximum of 11 or 12 characters long.
In this patch, I assume that they could be 12 characters long, so I have
extended the array sizes, but perhaps a more correct fix might
be to change the "%12s" instances to "%11s" instead.
Thanks in advance for considering this patch.
Signed-off-by: Adam Richter
---
libswscale/tests/swscale.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libswscale/tests/swscale.c b/libswscale/tests/swscale.c
index e72c4c3306..cb731f6211 100644
--- a/libswscale/tests/swscale.c
+++ b/libswscale/tests/swscale.c
@@ -312,10 +312,10 @@ static int fileTest(const uint8_t * const ref[4], int refStride[4],
while (fgets(buf, sizeof(buf), fp)) {
struct Results r;
enum AVPixelFormat srcFormat;
-char srcStr[12];
+char srcStr[13];
int srcW = 0, srcH = 0;
enum AVPixelFormat dstFormat;
-char dstStr[12];
+char dstStr[13];
int dstW = 0, dstH = 0;
int flags;
int ret;
--
2.21.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
