Re: [FFmpeg-devel] [PATCH 05/11] avformat/nutenc: Don't pass NULL to memcmp
On Thu, Oct 31, 2019 at 07:38:00AM +, Andreas Rheinhardt wrote: > Andreas Rheinhardt: > > Carl Eugen Hoyos: > >> Am Fr., 20. Sept. 2019 um 00:30 Uhr schrieb Andreas Rheinhardt > >> : > >>> > >>> Fixes lots of FATE tests, e.g. lavf-nut, as well as the nut part of > >>> ticket #7980. > >>> > >>> Signed-off-by: Andreas Rheinhardt > >>> --- > >>> This patch is made to match the previous behaviour; whether the previous > >>> behaviour is correct at all if the header length is zero is unknown to > >>> me. > >>> > >>> libavformat/nutenc.c | 5 +++-- > >>> 1 file changed, 3 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c > >>> index e9a3bb49db..dc714eb809 100644 > >>> --- a/libavformat/nutenc.c > >>> +++ b/libavformat/nutenc.c > >>> @@ -791,8 +791,9 @@ static int get_needed_flags(NUTContext *nut, > >>> StreamContext *nus, FrameCode *fc, > >>> flags |= FLAG_CHECKSUM; > >>> if (pkt->size < nut->header_len[fc->header_idx] || > >>> (pkt->size > 4096 && fc->header_idx)|| > >>> -memcmp(pkt->data, nut->header[fc->header_idx], > >>> - nut->header_len[fc->header_idx])) > >>> +(nut->header_len[fc->header_idx] > 0 && > >>> + memcmp(pkt->data, nut->header[fc->header_idx], > >>> +nut->header_len[fc->header_idx]))) > >>> flags |= FLAG_HEADER_IDX; > >> > >> Is this different from the patch I sent? > >> (Why does it look different) > >> https://patchwork.ffmpeg.org/patch/13782/ > >> > >> Carl Eugen > > > > Yes. > > a) Your patch checks whether nut->header[fc->header_idx] is NULL. > > b) Michael's proposal is to check for fc->header_idx instead. This > > would work, because it is the index of the only used pointer in > > nut->header that is NULL, and this proposal has the advantage of one > > dereferencing less than a). > > c) My* patch checks whether nut->header_len[fc->header_idx] is zero. > > Given that nut->header_len[fc->header_idx] is already used in the very > > first check, my solution should not entail a dereference at all. > > > > Furthermore, b) is very specific to the nutenc situation here and a) > > might hide a situation where nut->header_len[fc->header_idx] is > 0, > > but nut->header[fc->header_idx] == NULL, which should of course not > > happen. And it singles out one of the two pointer arguments. > > > > - Andreas > > > > *: It is also what Reimar proposed (I was completely unaware of the > > earlier patch when I submitted mine). > > > Ping. I dont think the resulting code is particularly readable with the increasingly complex condition in the if() ill post a alternative suggestion for consideration that has a simpler condition after testing it thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 2 "100% positive feedback" - "All either got their money back or didnt complain" "Best seller ever, very honest" - "Seller refunded buyer after failed scam" signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH 05/11] avformat/nutenc: Don't pass NULL to memcmp
Andreas Rheinhardt: > Carl Eugen Hoyos: >> Am Fr., 20. Sept. 2019 um 00:30 Uhr schrieb Andreas Rheinhardt >> : >>> >>> Fixes lots of FATE tests, e.g. lavf-nut, as well as the nut part of >>> ticket #7980. >>> >>> Signed-off-by: Andreas Rheinhardt >>> --- >>> This patch is made to match the previous behaviour; whether the previous >>> behaviour is correct at all if the header length is zero is unknown to >>> me. >>> >>> libavformat/nutenc.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c >>> index e9a3bb49db..dc714eb809 100644 >>> --- a/libavformat/nutenc.c >>> +++ b/libavformat/nutenc.c >>> @@ -791,8 +791,9 @@ static int get_needed_flags(NUTContext *nut, >>> StreamContext *nus, FrameCode *fc, >>> flags |= FLAG_CHECKSUM; >>> if (pkt->size < nut->header_len[fc->header_idx] || >>> (pkt->size > 4096 && fc->header_idx)|| >>> -memcmp(pkt->data, nut->header[fc->header_idx], >>> - nut->header_len[fc->header_idx])) >>> +(nut->header_len[fc->header_idx] > 0 && >>> + memcmp(pkt->data, nut->header[fc->header_idx], >>> +nut->header_len[fc->header_idx]))) >>> flags |= FLAG_HEADER_IDX; >> >> Is this different from the patch I sent? >> (Why does it look different) >> https://patchwork.ffmpeg.org/patch/13782/ >> >> Carl Eugen > > Yes. > a) Your patch checks whether nut->header[fc->header_idx] is NULL. > b) Michael's proposal is to check for fc->header_idx instead. This > would work, because it is the index of the only used pointer in > nut->header that is NULL, and this proposal has the advantage of one > dereferencing less than a). > c) My* patch checks whether nut->header_len[fc->header_idx] is zero. > Given that nut->header_len[fc->header_idx] is already used in the very > first check, my solution should not entail a dereference at all. > > Furthermore, b) is very specific to the nutenc situation here and a) > might hide a situation where nut->header_len[fc->header_idx] is > 0, > but nut->header[fc->header_idx] == NULL, which should of course not > happen. And it singles out one of the two pointer arguments. > > - Andreas > > *: It is also what Reimar proposed (I was completely unaware of the > earlier patch when I submitted mine). > Ping. - Andreas ___ 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".
Re: [FFmpeg-devel] [PATCH 05/11] avformat/nutenc: Don't pass NULL to memcmp
Carl Eugen Hoyos: > Am Fr., 20. Sept. 2019 um 00:30 Uhr schrieb Andreas Rheinhardt > : >> >> Fixes lots of FATE tests, e.g. lavf-nut, as well as the nut part of >> ticket #7980. >> >> Signed-off-by: Andreas Rheinhardt >> --- >> This patch is made to match the previous behaviour; whether the previous >> behaviour is correct at all if the header length is zero is unknown to >> me. >> >> libavformat/nutenc.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c >> index e9a3bb49db..dc714eb809 100644 >> --- a/libavformat/nutenc.c >> +++ b/libavformat/nutenc.c >> @@ -791,8 +791,9 @@ static int get_needed_flags(NUTContext *nut, >> StreamContext *nus, FrameCode *fc, >> flags |= FLAG_CHECKSUM; >> if (pkt->size < nut->header_len[fc->header_idx] || >> (pkt->size > 4096 && fc->header_idx)|| >> -memcmp(pkt->data, nut->header[fc->header_idx], >> - nut->header_len[fc->header_idx])) >> +(nut->header_len[fc->header_idx] > 0 && >> + memcmp(pkt->data, nut->header[fc->header_idx], >> +nut->header_len[fc->header_idx]))) >> flags |= FLAG_HEADER_IDX; > > Is this different from the patch I sent? > (Why does it look different) > https://patchwork.ffmpeg.org/patch/13782/ > > Carl Eugen Yes. a) Your patch checks whether nut->header[fc->header_idx] is NULL. b) Michael's proposal is to check for fc->header_idx instead. This would work, because it is the index of the only used pointer in nut->header that is NULL, and this proposal has the advantage of one dereferencing less than a). c) My* patch checks whether nut->header_len[fc->header_idx] is zero. Given that nut->header_len[fc->header_idx] is already used in the very first check, my solution should not entail a dereference at all. Furthermore, b) is very specific to the nutenc situation here and a) might hide a situation where nut->header_len[fc->header_idx] is > 0, but nut->header[fc->header_idx] == NULL, which should of course not happen. And it singles out one of the two pointer arguments. - Andreas *: It is also what Reimar proposed (I was completely unaware of the earlier patch when I submitted mine). ___ 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".
Re: [FFmpeg-devel] [PATCH 05/11] avformat/nutenc: Don't pass NULL to memcmp
Am Fr., 20. Sept. 2019 um 00:30 Uhr schrieb Andreas Rheinhardt : > > Fixes lots of FATE tests, e.g. lavf-nut, as well as the nut part of > ticket #7980. > > Signed-off-by: Andreas Rheinhardt > --- > This patch is made to match the previous behaviour; whether the previous > behaviour is correct at all if the header length is zero is unknown to > me. > > libavformat/nutenc.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c > index e9a3bb49db..dc714eb809 100644 > --- a/libavformat/nutenc.c > +++ b/libavformat/nutenc.c > @@ -791,8 +791,9 @@ static int get_needed_flags(NUTContext *nut, > StreamContext *nus, FrameCode *fc, > flags |= FLAG_CHECKSUM; > if (pkt->size < nut->header_len[fc->header_idx] || > (pkt->size > 4096 && fc->header_idx)|| > -memcmp(pkt->data, nut->header[fc->header_idx], > - nut->header_len[fc->header_idx])) > +(nut->header_len[fc->header_idx] > 0 && > + memcmp(pkt->data, nut->header[fc->header_idx], > +nut->header_len[fc->header_idx]))) > flags |= FLAG_HEADER_IDX; Is this different from the patch I sent? (Why does it look different) https://patchwork.ffmpeg.org/patch/13782/ Carl Eugen ___ 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 05/11] avformat/nutenc: Don't pass NULL to memcmp
Fixes lots of FATE tests, e.g. lavf-nut, as well as the nut part of ticket #7980. Signed-off-by: Andreas Rheinhardt --- This patch is made to match the previous behaviour; whether the previous behaviour is correct at all if the header length is zero is unknown to me. libavformat/nutenc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libavformat/nutenc.c b/libavformat/nutenc.c index e9a3bb49db..dc714eb809 100644 --- a/libavformat/nutenc.c +++ b/libavformat/nutenc.c @@ -791,8 +791,9 @@ static int get_needed_flags(NUTContext *nut, StreamContext *nus, FrameCode *fc, flags |= FLAG_CHECKSUM; if (pkt->size < nut->header_len[fc->header_idx] || (pkt->size > 4096 && fc->header_idx)|| -memcmp(pkt->data, nut->header[fc->header_idx], - nut->header_len[fc->header_idx])) +(nut->header_len[fc->header_idx] > 0 && + memcmp(pkt->data, nut->header[fc->header_idx], +nut->header_len[fc->header_idx]))) flags |= FLAG_HEADER_IDX; return flags | (fc->flags & FLAG_CODED); -- 2.20.1 ___ 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".
