flac dev - May 2012 - [PATCH] Add missing functions to VorbisComment class + a few other things

Attached is a patch that adds 5 missing FLAC__metadata_object_vorbiscomment_*
functions to the VorbisComment class. In my previous message I stated 8
functions were missing, but on closer inspection, 3 of those belong in the
VorbisComment::Entry class, and 2 of them already have equivalent functions in
there. The last one (FLAC__metadata_object_vorbiscomment_entry_matches()) does
not, but I have not done that one (yet).

Looking at the FLAC__metadata_object_cuesheet_* FLAC__metadata_object_picture_*
functions, it looks like the corresponding FLAC++ classes are already complete.
Maybe some functions are missing from CueSheet::Track. If nobody objects, I will
take a look later this week.

Also, I've noticed that on my system, flac will not compile with
--enable-debug, because some functions that use 'PRId64' get compiled
in, but no included header defines 'PRId64'. I have not written a patch
because I'm not sure whether this is just my weird system (fairly standard
Fedora installation) or all gcc users or all *NIX users. However, should a fix
be necessary: PRId64 is defined in inttypes.h which can be included in line 43
of src/libFLAC/lpo.c (after the '#if defined DEBUG') to solve this on my
system.

Lastly, the reason I tried to compile with debug was some invalid read sizes
reported by valgrind when creating a VorbisComment::Entry. The invalid read is
reported in set_field_name() at line 653 (src/libFLAC++/metadata.cpp), where
strlen() is used on the newly created char *field_name_. I could not see
anything wrong with the code though, and strangely enough just calling printf on
the field_name_ 1 line before the strlen() removes all valgrind errors. So
I'm not sure what's going on, but it's probably a bug in valgrind,
maybe somebody on this list knows?

Bas Timmer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: add_vorbiscomment_members.patch
Type: application/octet-stream
Size: 3345 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/flac-dev/attachments/20120507/f6e0ea11/attachment.obj

El 07/05/12 17:02, Bastiaan Timmer escribi?:
> Attached is a patch that adds 5 missing
FLAC__metadata_object_vorbiscomment_* functions to the VorbisComment class. In
my previous message I stated 8 functions were missing, but on closer inspection,
3 of those belong in the VorbisComment::Entry class, and 2 of them already have
equivalent functions in there. The last one
(FLAC__metadata_object_vorbiscomment_entry_matches()) does not, but I have not
done that one (yet).
>
> Looking at the FLAC__metadata_object_cuesheet_*
FLAC__metadata_object_picture_* functions, it looks like the corresponding
FLAC++ classes are already complete. Maybe some functions are missing from
CueSheet::Track. If nobody objects, I will take a look later this week.
>
> Also, I've noticed that on my system, flac will not compile with
--enable-debug, because some functions that use 'PRId64' get compiled
in, but no included header defines 'PRId64'. I have not written a patch
because I'm not sure whether this is just my weird system (fairly standard
Fedora installation) or all gcc users or all *NIX users. However, should a fix
be necessary: PRId64 is defined in inttypes.h which can be included in line 43
of src/libFLAC/lpo.c (after the '#if defined DEBUG') to solve this on my
system.
>
> Lastly, the reason I tried to compile with debug was some invalid read
sizes reported by valgrind when creating a VorbisComment::Entry. The invalid
read is reported in set_field_name() at line 653 (src/libFLAC++/metadata.cpp),
where strlen() is used on the newly created char *field_name_. I could not see
anything wrong with the code though, and strangely enough just calling printf on
the field_name_ 1 line before the strlen() removes all valgrind errors. So
I'm not sure what's going on, but it's probably a bug in valgrind,
maybe somebody on this list knows?
>
While you are at it, can you check/fix the following warning ?

metadata.cpp:812:98: warning: narrowing conversion of 'strlen(((const 
char*)string))' from 'size_t {aka long unsigned int}' to
'FLAC__uint32
{aka unsigned int}' inside { } is ill-formed in C++11 [-Wnarrowing]


Thanks !

Bastiaan Timmer wrote:
> Attached is a patch that adds 5 missing
Patch applied. Thanks.
> Looking at the FLAC__metadata_object_cuesheet_* FLAC__metadata_
> object_picture_* functions, it looks like the corresponding FLAC++
> classes are already complete. Maybe some functions are missing from
> CueSheet::Track. If nobody objects, I will take a look later this
> week.
Thanks.
> Also, I've noticed that on my system, flac will not compile with
> --enable-debug, because some functions that use 'PRId64' get
compiled
> in, but no included header defines 'PRId64'.
Fixed, thanks.
> Lastly, the reason I tried to compile with debug was some invalid read
> sizes reported by valgrind when creating a VorbisComment::Entry. The
> invalid read is reported in set_field_name() at line 653 (
> src/libFLAC++/metadata.cpp), where strlen() is used on the newly created
> char *field_name_. I could not see anything wrong with the code though,
> and strangely enough just calling printf on the field_name_ 1 line before
> the strlen() removes all valgrind errors. So I'm not sure what's
going on,
> but it's probably a bug in valgrind, maybe somebody on this list knows?
Honestly, I really doubt this is a bug in valgrind :-). How were you testing
this?

Cheers,
Erik
-- 
----------------------------------------------------------------------
Erik de Castro Lopo
http://www.mega-nerd.com/

--- On Tue, 5/8/12, Erik de Castro Lopo <mle+la at mega-nerd.com> wrote:
> Honestly, I really doubt this is a bug in valgrind :-). How
> were you testing
> this?
Well, I've read that there have been bugs in valgrind, were SSE optimized
versions of strlen() do guaranteed safe overreads of memory, but valgrind
wasn't aware the overreads were safe.

Anyway, it seems easy to reproduce for me, but I still may just be doing
something stupid:

[~]$ cat main.cc
#include <FLAC++/metadata.h>

int main()
{  
  FLAC::Metadata::VorbisComment::Entry entry("ARTIST", "Someone
cool and terribly hip");
  return 0;
}
[~]$ g++ -Wall -Wextra -O1 -g -lFLAC++ -o invalid_read main.cc 
[~]$ valgrind ./invalid_read 
==11150== Memcheck, a memory error detector
==11150== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==11150== Using Valgrind-3.6.1 and LibVEX; rerun with -h for copyright info
==11150== Command: ./invalid_read
==11150== 
==11150== Invalid read of size 4
==11150==    at 0x4030366:
FLAC::Metadata::VorbisComment::Entry::set_field_name(char const*)
(metadata.cpp:653)
==11150==    by 0x7D5E35: (below main) (in /lib/libc-2.13.so)
==11150==  Address 0x418802c is 4 bytes inside a block of size 7 alloc'd
==11150==    at 0x4007D89: malloc (vg_replace_malloc.c:236)
==11150==    by 0x83502F: strdup (in /lib/libc-2.13.so)
==11150==    by 0x4030347:
FLAC::Metadata::VorbisComment::Entry::set_field_name(char const*)
(metadata.cpp:649)
==11150==    by 0x7D5E35: (below main) (in /lib/libc-2.13.so)
==11150== 
==11150== 
==11150== HEAP SUMMARY:
==11150==     in use at exit: 0 bytes in 0 blocks
==11150==   total heap usage: 5 allocs, 5 frees, 119 bytes allocated
==11150== 
==11150== All heap blocks were freed -- no leaks are possible
==11150== 
==11150== For counts of detected and suppressed errors, rerun with: -v
==11150== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 21 from 8)
[~]$ 

Note, this is linked to current git (installed to /usr/local):
[~]$ ldd invalid_read | grep -i flac
        libFLAC++.so.5 => /usr/local/lib/libFLAC++.so.5 (0x00358000)
        libFLAC.so.8 => /usr/local/lib/libFLAC.so.8 (0x00110000)
[~]$

The exact same program, linked to the distro's current stable release
(1.2.1):
[~]$ g++ -Wall -Wextra -O1 -g -L/usr/lib -lFLAC++ -I/usr/include/ -o ok_read
-Xlinker -rpath -Xlinker /usr/lib main.cc
[~]$ ldd ok_read | grep -i flac
        libFLAC++.so.6 => /usr/lib/libFLAC++.so.6 (0x00f89000)
        libFLAC.so.8 => /usr/lib/libFLAC.so.8 (0x061d0000)
[~]$ valgrind ./ok_read 
==11241== Memcheck, a memory error detector
==11241== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==11241== Using Valgrind-3.6.1 and LibVEX; rerun with -h for copyright info
==11241== Command: ./ok_read
==11241== 
==11241== 
==11241== HEAP SUMMARY:
==11241==     in use at exit: 0 bytes in 0 blocks
==11241==   total heap usage: 5 allocs, 5 frees, 119 bytes allocated
==11241== 
==11241== All heap blocks were freed -- no leaks are possible
==11241== 
==11241== For counts of detected and suppressed errors, rerun with: -v
==11241== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 21 from 8)
[~]$

I still have this creeping suspicion I'm missing something obvious...

Thanks for looking at and applying my patches the last few days by the way!

Bas