On 07/10/2011 03:09 PM, Erik Hovland wrote:
diff --git a/src/ifo_read.c b/src/ifo_read.c
index 4a422c6..fc5b39b 100644
--- a/src/ifo_read.c
+++ b/src/ifo_read.c
@@ -1177,8 +1177,7 @@ int ifoRead_VTS_PTT_SRPT(ifo_handle_t *ifofile) {
info_length = vts_ptt_srpt->last_byte + 1 - VTS_PTT_SRPT_SIZE;
- data = (uint32_t *)malloc(info_length);
- if(!data) {
+ if(!(data = malloc(info_length))) {
Personally I prefer not to merge the lines, I think it makes the code
harder to read, but that's not important..
Fine w/ me. I fixed the other malloc to not have the cast as well.
Probably not this way though.
If you initialize data and vts_ptt_srpt to NULL
you only need
err_out:
free(data);
ifofile->vts_ptt_srpt = 0;
free(vts_ptt_srpt);
return 0;
New patch does it the way you suggest.
The ifoRead_VTS is only makes the name unwieldy, gotos can't go across
functions.
The label was long because I kept forgetting which function I was in.
This version removes it.
+ if(vts_ptt_srpt->nr_of_srpts * sizeof(uint32_t)> info_length) {
The suggestion was to use vts_ptt_srpt->nr_of_srpts * sizeof(*data)
so that the check matches the reads the code does no matter what type
data has.
And while not an issue here for several reasons I'd recommend going with
vts_ptt_srpt->nr_of_srpts> info_length / sizeof(*data)
which is safe from integer overflows.
Considering how often getting this wrong leads to exploitable bugs IMO
it's a good idea to always use the paranoid version in the hopes that
everyone gets used to doing it in the safe way.
Let's take the high road!
Thanks for the review.
E
The discussion on this kind of petered out. Is this going to be committed? Are there additional changes that need to
be made? The only additional comment I see is about vts_ptt_srpt->title cleanup which was already a loose end before
the proposed patch and should be solved in it's own patch.
_______________________________________________
DVDnav-discuss mailing list
[email protected]
https://lists.mplayerhq.hu/mailman/listinfo/dvdnav-discuss
