r23727 - trunk/libdvdcss/libdvdcss.c
Author: reimar Date: Sat Jul 7 00:07:35 2007 New Revision: 23727 Log: Fix CSS format strings, they could print more than two digits for "negative" numbers. Modified: trunk/libdvdcss/libdvdcss.c Modified: trunk/libdvdcss/libdvdcss.c ============================================================================== --- trunk/libdvdcss/libdvdcss.c (original) +++ trunk/libdvdcss/libdvdcss.c Sat Jul 7 00:07:35 2007 @@ -472,7 +472,7 @@ LIBDVDCSS_EXPORT dvdcss_t dvdcss_open ( { char psz_tmp[16 + 1]; sprintf( psz_tmp, - "%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x", + "%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"", psz_serial[0], psz_serial[1], psz_serial[2], psz_serial[3], psz_serial[4], psz_serial[5], psz_serial[6], psz_serial[7] ); @@ -488,7 +488,7 @@ LIBDVDCSS_EXPORT dvdcss_t dvdcss_open ( psz_key[0] = '-'; for( i = 0; i < KEY_SIZE; i++ ) { - sprintf( &psz_key[1+i*2], "%.2x", dvdcss->css.p_disc_key[i] ); + sprintf( &psz_key[1+i*2], "%.2"PRIx8, dvdcss->css.p_disc_key[i] ); } psz_key[1 + KEY_SIZE * 2] = '\0'; }
New Revision: 23727
Log: Fix CSS format strings, they could print more than two digits for
Modified: trunk/libdvdcss/libdvdcss.c
--- trunk/libdvdcss/libdvdcss.c (original) +++ trunk/libdvdcss/libdvdcss.c Sat Jul 7 00:07:35 2007 @@ -472,7 +472,7 @@ LIBDVDCSS_EXPORT dvdcss_t dvdcss_open ( { char psz_tmp[16 + 1]; sprintf( psz_tmp, - "%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x", + "%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"", psz_serial[0], psz_serial[1], psz_serial[2], psz_serial[3], psz_serial[4], psz_serial[5], psz_serial[6], psz_serial[7] ); @@ -488,7 +488,7 @@ LIBDVDCSS_EXPORT dvdcss_t dvdcss_open ( psz_key[0] = '-'; for( i = 0; i < KEY_SIZE; i++ ) { - sprintf( &psz_key[1+i*2], "%.2x", dvdcss->css.p_disc_key[i] ); + sprintf( &psz_key[1+i*2], "%.2"PRIx8, dvdcss->css.p_disc_key[i] ); } psz_key[1 + KEY_SIZE * 2] = '\0'; }
Is this fit for upstream? PRIx format specifiers are not used elsewhere in libdvdcss. Are they available everywhere? Diego
Hello, On Fri, Jul 13, 2007 at 06:49:31PM +0200, Diego Biurrun wrote:
New Revision: 23727
Log: Fix CSS format strings, they could print more than two digits for
Modified: trunk/libdvdcss/libdvdcss.c
--- trunk/libdvdcss/libdvdcss.c (original) +++ trunk/libdvdcss/libdvdcss.c Sat Jul 7 00:07:35 2007 @@ -472,7 +472,7 @@ LIBDVDCSS_EXPORT dvdcss_t dvdcss_open ( { char psz_tmp[16 + 1]; sprintf( psz_tmp, - "%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x", + "%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"", psz_serial[0], psz_serial[1], psz_serial[2], psz_serial[3], psz_serial[4], psz_serial[5], psz_serial[6], psz_serial[7] ); @@ -488,7 +488,7 @@ LIBDVDCSS_EXPORT dvdcss_t dvdcss_open ( psz_key[0] = '-'; for( i = 0; i < KEY_SIZE; i++ ) { - sprintf( &psz_key[1+i*2], "%.2x", dvdcss->css.p_disc_key[i] ); + sprintf( &psz_key[1+i*2], "%.2"PRIx8, dvdcss->css.p_disc_key[i] ); } psz_key[1 + KEY_SIZE * 2] = '\0'; }
Is this fit for upstream?
PRIx format specifiers are not used elsewhere in libdvdcss. Are they available everywhere?
They are available everywhere where a proper inttypes.h is available. It is not a very relevant change though (esp. compared to r23728). You could also cast all parameters to unsigned instead. IMO using sprintf (and even snprintf) here is a stupid idea anyway. Greetings, Reimar Döffinger
On Sat, Jul 14, 2007 at 10:06:31AM +0200, Reimar Döffinger wrote:
On Fri, Jul 13, 2007 at 06:49:31PM +0200, Diego Biurrun wrote:
Log: Fix CSS format strings, they could print more than two digits for
Modified: trunk/libdvdcss/libdvdcss.c
--- trunk/libdvdcss/libdvdcss.c (original) +++ trunk/libdvdcss/libdvdcss.c Sat Jul 7 00:07:35 2007 @@ -472,7 +472,7 @@ LIBDVDCSS_EXPORT dvdcss_t dvdcss_open ( { char psz_tmp[16 + 1]; sprintf( psz_tmp, - "%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x", + "%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"%.2"PRIx8"", psz_serial[0], psz_serial[1], psz_serial[2], psz_serial[3], psz_serial[4], psz_serial[5], psz_serial[6], psz_serial[7] ); @@ -488,7 +488,7 @@ LIBDVDCSS_EXPORT dvdcss_t dvdcss_open ( psz_key[0] = '-'; for( i = 0; i < KEY_SIZE; i++ ) { - sprintf( &psz_key[1+i*2], "%.2x", dvdcss->css.p_disc_key[i] ); + sprintf( &psz_key[1+i*2], "%.2"PRIx8, dvdcss->css.p_disc_key[i] ); } psz_key[1 + KEY_SIZE * 2] = '\0'; }
Is this fit for upstream?
PRIx format specifiers are not used elsewhere in libdvdcss. Are they available everywhere?
They are available everywhere where a proper inttypes.h is available.
I'm asking because libdvdcss does not contain any PRI format specifiers, so there is a probability that they will not want to add them.
It is not a very relevant change though (esp. compared to r23728). You could also cast all parameters to unsigned instead. IMO using sprintf (and even snprintf) here is a stupid idea anyway.
I'm asking because I wish to keep our diffs to upstream as small as possible. If r23728 is enough to solve the problem then this might as well be reverted. Diego
Hello, On Sat, Jul 14, 2007 at 02:50:00PM +0200, Diego Biurrun wrote:
On Sat, Jul 14, 2007 at 10:06:31AM +0200, Reimar Döffinger wrote:
It is not a very relevant change though (esp. compared to r23728). You could also cast all parameters to unsigned instead. IMO using sprintf (and even snprintf) here is a stupid idea anyway.
I'm asking because I wish to keep our diffs to upstream as small as possible. If r23728 is enough to solve the problem then this might as well be reverted.
It solves the critical problem, but without it the format strings do not match the type, though I do not know on what type of system it will make a difference. Greetings, Reimar Döffinger
On Sat, Jul 14, 2007 at 03:23:46PM +0200, Reimar Döffinger wrote:
On Sat, Jul 14, 2007 at 02:50:00PM +0200, Diego Biurrun wrote:
On Sat, Jul 14, 2007 at 10:06:31AM +0200, Reimar Döffinger wrote:
It is not a very relevant change though (esp. compared to r23728). You could also cast all parameters to unsigned instead. IMO using sprintf (and even snprintf) here is a stupid idea anyway.
I'm asking because I wish to keep our diffs to upstream as small as possible. If r23728 is enough to solve the problem then this might as well be reverted.
It solves the critical problem, but without it the format strings do not match the type, though I do not know on what type of system it will make a difference.
Umm, you mean Ivan's commit solves the critical problem while yours makes the types match? My point is that if your commit is more or less cosmetic then I would suggest reverting it to minimize the diff towards upstream. Diego
Hello, On Mon, Jul 16, 2007 at 11:24:17PM +0200, Diego Biurrun wrote:
On Sat, Jul 14, 2007 at 03:23:46PM +0200, Reimar Döffinger wrote:
On Sat, Jul 14, 2007 at 02:50:00PM +0200, Diego Biurrun wrote:
On Sat, Jul 14, 2007 at 10:06:31AM +0200, Reimar Döffinger wrote:
It is not a very relevant change though (esp. compared to r23728). You could also cast all parameters to unsigned instead. IMO using sprintf (and even snprintf) here is a stupid idea anyway.
I'm asking because I wish to keep our diffs to upstream as small as possible. If r23728 is enough to solve the problem then this might as well be reverted.
It solves the critical problem, but without it the format strings do not match the type, though I do not know on what type of system it will make a difference.
Umm, you mean Ivan's commit solves the critical problem while yours makes the types match?
My point is that if your commit is more or less cosmetic then I would suggest reverting it to minimize the diff towards upstream.
AFAICT sprintf behaviour is unspecified when the types mismatch as is the case without this patch. But actually this is moot, since AFAICT sprintf isn't really specified sufficiently to be used in this way if you are pedantic. Which is why I proposed a different patch on libdvdcss-devel - unfortunately the mails never got through, nor did I receive any of the mails sent, despite I received a confirmation mail that I was subscribed. Is there a chance of getting dvdcss upstream that is not just a joke (or more precisely a black hole)? I'll attach the mails I sent, since you found a magic way to get through the black hole they use as email filtering. Greetings, Reimar Döffinger
On Tue, Jul 17, 2007 at 12:37:30AM +0200, Reimar Döffinger wrote:
On Mon, Jul 16, 2007 at 11:24:17PM +0200, Diego Biurrun wrote:
On Sat, Jul 14, 2007 at 03:23:46PM +0200, Reimar Döffinger wrote:
On Sat, Jul 14, 2007 at 02:50:00PM +0200, Diego Biurrun wrote:
On Sat, Jul 14, 2007 at 10:06:31AM +0200, Reimar Döffinger wrote:
It is not a very relevant change though (esp. compared to r23728). You could also cast all parameters to unsigned instead. IMO using sprintf (and even snprintf) here is a stupid idea anyway.
I'm asking because I wish to keep our diffs to upstream as small as possible. If r23728 is enough to solve the problem then this might as well be reverted.
It solves the critical problem, but without it the format strings do not match the type, though I do not know on what type of system it will make a difference.
Umm, you mean Ivan's commit solves the critical problem while yours makes the types match?
My point is that if your commit is more or less cosmetic then I would suggest reverting it to minimize the diff towards upstream.
AFAICT sprintf behaviour is unspecified when the types mismatch as is the case without this patch. But actually this is moot, since AFAICT sprintf isn't really specified sufficiently to be used in this way if you are pedantic. Which is why I proposed a different patch on libdvdcss-devel - unfortunately the mails never got through, nor did I receive any of the mails sent, despite I received a confirmation mail that I was subscribed. I'll attach the mails I sent, since you found a magic way to get through the black hole they use as email filtering.
Do you get mails from the mailing list? Otherwise it's probably time to complain to their mailing list admin.
Is there a chance of getting dvdcss upstream that is not just a joke (or more precisely a black hole)?
It worked for me when I sent Sam Hocevar personal emails and/or contacted him through IRC, he used to be sam on freenode. Nowadays I mostly go through Steve L'Homme, he is a VLC committer and can commit to libdvdcss as well. He is robUx4 on freenode and often hangs out in the #ffmpeg channel. He has funneled our last set of libdvdcss patches into their repository. Diego
I'm working on moving our libdvdcss patches upstream... On Tue, Jul 17, 2007 at 12:37:30AM +0200, Reimar Döffinger wrote:
On Mon, Jul 16, 2007 at 11:24:17PM +0200, Diego Biurrun wrote:
On Sat, Jul 14, 2007 at 03:23:46PM +0200, Reimar Döffinger wrote:
On Sat, Jul 14, 2007 at 02:50:00PM +0200, Diego Biurrun wrote:
On Sat, Jul 14, 2007 at 10:06:31AM +0200, Reimar Döffinger wrote:
It is not a very relevant change though (esp. compared to r23728). You could also cast all parameters to unsigned instead. IMO using sprintf (and even snprintf) here is a stupid idea anyway.
I'm asking because I wish to keep our diffs to upstream as small as possible. If r23728 is enough to solve the problem then this might as well be reverted.
It solves the critical problem, but without it the format strings do not match the type, though I do not know on what type of system it will make a difference.
Umm, you mean Ivan's commit solves the critical problem while yours makes the types match?
My point is that if your commit is more or less cosmetic then I would suggest reverting it to minimize the diff towards upstream.
AFAICT sprintf behaviour is unspecified when the types mismatch as is the case without this patch. But actually this is moot, since AFAICT sprintf isn't really specified sufficiently to be used in this way if you are pedantic. Which is why I proposed a different patch on libdvdcss-devel
Your patch does not even compile: cc -Wdisabled-optimization -Wno-pointer-sign -Wdeclaration-after-statement -std=gnu99 -I. -Wall -Wno-switch -Wpointer-arith -Wredundant-decls -O4 -march=k6-3 -mtune=k6-3 -pipe -ffast-math -fomit-frame-pointer -D_REENTRANT -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE64_SOURCE -I/usr/include/directfb -I/usr/include/ -I/usr/include/SDL -D_REENTRANT -I/usr/include/kde/artsc -pthread -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/libpng12 -D__USE_UNIX98 -D_GNU_SOURCE -DVERSION=\"1.2.9\" -c -o libdvdcss/libdvdcss.o libdvdcss/libdvdcss.c libdvdcss/libdvdcss.c: In function 'to_hex': libdvdcss/libdvdcss.c:155: warning: initialization makes integer from pointer without a cast libdvdcss/libdvdcss.c:156: warning: initialization makes integer from pointer without a cast libdvdcss/libdvdcss.c:159: error: invalid type argument of 'unary *' libdvdcss/libdvdcss.c:159: warning: passing argument 1 of 'put_hex4' makes pointer from integer without a cast libdvdcss/libdvdcss.c:160: error: invalid type argument of 'unary *' libdvdcss/libdvdcss.c:160: warning: passing argument 1 of 'put_hex4' makes pointer from integer without a cast make: *** [libdvdcss/libdvdcss.o] Error 1 I have attached the patch for reference. Diego
On 8/21/08, Diego Biurrun <diego@biurrun.de> wrote:
I'm working on moving our libdvdcss patches upstream...
On Tue, Jul 17, 2007 at 12:37:30AM +0200, Reimar Döffinger wrote:
On Mon, Jul 16, 2007 at 11:24:17PM +0200, Diego Biurrun wrote:
On Sat, Jul 14, 2007 at 03:23:46PM +0200, Reimar Döffinger wrote:
On Sat, Jul 14, 2007 at 02:50:00PM +0200, Diego Biurrun wrote:
On Sat, Jul 14, 2007 at 10:06:31AM +0200, Reimar Döffinger wrote:
It is not a very relevant change though (esp. compared to r23728). You could also cast all parameters to unsigned instead. IMO using sprintf (and even snprintf) here is a stupid idea anyway.
I'm asking because I wish to keep our diffs to upstream as small as possible. If r23728 is enough to solve the problem then this might as well be reverted.
It solves the critical problem, but without it the format strings do not match the type, though I do not know on what type of system it will make a difference.
Umm, you mean Ivan's commit solves the critical problem while yours makes the types match?
My point is that if your commit is more or less cosmetic then I would suggest reverting it to minimize the diff towards upstream.
AFAICT sprintf behaviour is unspecified when the types mismatch as is the case without this patch. But actually this is moot, since AFAICT sprintf isn't really specified sufficiently to be used in this way if you are pedantic. Which is why I proposed a different patch on libdvdcss-devel
Your patch does not even compile:
cc -Wdisabled-optimization -Wno-pointer-sign -Wdeclaration-after-statement -std=gnu99 -I. -Wall -Wno-switch -Wpointer-arith -Wredundant-decls -O4 -march=k6-3 -mtune=k6-3 -pipe -ffast-math -fomit-frame-pointer -D_REENTRANT -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE64_SOURCE -I/usr/include/directfb -I/usr/include/ -I/usr/include/SDL -D_REENTRANT -I/usr/include/kde/artsc -pthread -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/libpng12 -D__USE_UNIX98 -D_GNU_SOURCE -DVERSION=\"1.2.9\" -c -o libdvdcss/libdvdcss.o libdvdcss/libdvdcss.c libdvdcss/libdvdcss.c: In function 'to_hex': libdvdcss/libdvdcss.c:155: warning: initialization makes integer from pointer without a cast libdvdcss/libdvdcss.c:156: warning: initialization makes integer from pointer without a cast libdvdcss/libdvdcss.c:159: error: invalid type argument of 'unary *' libdvdcss/libdvdcss.c:159: warning: passing argument 1 of 'put_hex4' makes pointer from integer without a cast libdvdcss/libdvdcss.c:160: error: invalid type argument of 'unary *' libdvdcss/libdvdcss.c:160: warning: passing argument 1 of 'put_hex4' makes pointer from integer without a cast make: *** [libdvdcss/libdvdcss.o] Error 1
I have attached the patch for reference.
Diego
This is once again prove that Diego have 0 talent to understand C code. Why he is allowed to mess with it is beyond my ability to understand. I guess that's because nobody can stop him. Here is the fixed patch, against current svn that you can test. Two * were missing. Reimar would you commit your code?
On Thu, Aug 21, 2008 at 10:41:53PM +0200, Diego Biurrun wrote:
AFAICT sprintf behaviour is unspecified when the types mismatch as is the case without this patch. But actually this is moot, since AFAICT sprintf isn't really specified sufficiently to be used in this way if you are pedantic. Which is why I proposed a different patch on libdvdcss-devel
Your patch does not even compile:
Nor does it work, one of the to_hex calls should be using psz_tmp. This was just a quick copy-and-paste hack to get some comments. I mainly do not like the current code because it assumes sprintf will have a certain output - but its behaviour can completely change simply because a different locale is set) even though it is very unlikely in case of %x. Greetings, Reimar Döffinger
On Sat, Jul 07, 2007 at 12:07:36AM +0200, reimar wrote:
Log: Fix CSS format strings, they could print more than two digits for "negative" numbers.
This was rejected by Sam Hocevar on libdvdcss-devel: As discussed on IRC, a fix for this issue was actually already in trunk. I do not feel the need to switch to PRIx8 conversions because arguments smaller than int will be promoted with the expected sign extension anyway (C standard 6.5.2.3p7 for variadic function rules, 6.3.1.1 for integer promotion). So I'm backing this out with the libdvdcss sync (1.2.10 has just been released). Diego
On 8/29/08, Diego Biurrun <diego@biurrun.de> wrote:
On Sat, Jul 07, 2007 at 12:07:36AM +0200, reimar wrote:
Log: Fix CSS format strings, they could print more than two digits for "negative" numbers.
This was rejected by Sam Hocevar on libdvdcss-devel:
As discussed on IRC, a fix for this issue was actually already in trunk. I do not feel the need to switch to PRIx8 conversions because arguments smaller than int will be promoted with the expected sign extension anyway (C standard 6.5.2.3p7 for variadic function rules, 6.3.1.1 for integer promotion).
So I'm backing this out with the libdvdcss sync (1.2.10 has just been released).
That's nonsense. This is like removing car belts because the car is equipped with air bags. The problem is in the sign extension. If it is negative you get 0xffffffff, the 0.2 format require to have at least 2 digits, but it allows to have more than 2, if negative it would be 6 symbols. That causes write after the end of the array and stack corruption. Indeed making the array unsigned is enough to prevent the sign extension, but you can never be too cautious with sprintf.
participants (4)
-
Diego Biurrun -
Ivan Kalvachev -
reimar -
Reimar Döffinger