Bug 227325

Summary: PCM ioctl updates
Product: [Developer tools] valgrind Reporter: lool <lool>
Component: generalAssignee: Julian Seward <jseward>
Status: RESOLVED FIXED    
Severity: normal CC: lool, tom
Priority: NOR    
Version: 3.6 SVN   
Target Milestone: wanted3.5.1   
Platform: Unlisted Binaries   
OS: Linux   
Latest Commit: Version Fixed In:
Attachments: PCM ioctls updates

Description lool@dooz.org 2010-02-17 10:04:49 UTC 
Created attachment 40869 [details]
PCM ioctls updates

Hi

Samuel Thibault <samuel.thibault@ens-lyon.org> provided in Debian bug http://bugs.debian.org/296323 a patch for the bug he describes as follows:
"""
Upon

ioctl(fd, SNDCTL_DSP_GETBLKSIZE, &blockSize);

valgrind complains if blockSize was not initialized. But its value is
really not read by the kernel before being written to.

And ioctl(fd, SNDCTL_DSP_SETFMT, &format); is not recognized as reading
and then writing a value.
"""

Some time later, he provided an updated patch in Debian bug http://bugs.debian.org/305051 which I'll attach here.

Thanks,
Comment 1 Tom Hughes 2010-02-18 16:46:33 UTC 
The patch isn't quite right, because GETBLKSIZE still needs to do a PRE_MEM_WRITE before the calls. Also SETFMT needs to do a POST_MEM_WRITE afterwards as the kernel will return a value.

You also haven't said why you're removing the extra case for PCM_READ_BITS, which doesn't seem to be in the original patch on the debian bug.
Comment 2 Tom Hughes 2010-02-18 16:50:47 UTC 
Ughh.... I see now... That extra READ_BITS case is actually the same as SETFMT because it is forcing the W bit...

Does that mean PCM_READ_BITS used to be declared as WR and then got fixed, and it's old ioctl got reused for SETFMT? That would be really nasty... To be honest overloading the same ioctl number for different things based on the R/W bits is just horrible anyway.
Comment 3 Tom Hughes 2010-02-18 16:55:02 UTC 
Improved patch committed as r11050.
Comment 4 lool@dooz.org 2010-02-18 18:01:01 UTC 
I'm afraid I don't have the technical background on the patch, albeit I did point the submitter / original author at this bug; perhaps he will comment.

Thanks a lot for improving/fixing and committing the patch!