* QA Notice: Package has poor programming practices which may compile * fine but exhibit random runtime failures. * fonts.c:822: warning: dereferencing type-punned pointer will break strict-aliasing rules fonts.c:824: warning: dereferencing type-punned pointer will break strict-aliasing rules fonts.c:826: warning: dereferencing type-punned pointer will break strict-aliasing rules fonts.c:828: warning: dereferencing type-punned pointer will break strict-aliasing rules modules.c:181: warning: dereferencing type-punned pointer will break strict-aliasing rules modules.c:183: warning: dereferencing type-punned pointer will break strict-aliasing rules modules.c:185: warning: dereferencing type-punned pointer will break strict-aliasing rules modules.c:187: warning: dereferencing type-punned pointer will break strict-aliasing rules pangofc-fontmap.c:1315: warning: dereferencing type-punned pointer will break strict-aliasing rules Reproducible: Always Steps to Reproduce:
Created attachment 108084 [details, diff] Fixes strict aliasing, and some minor conformance issue, avoid a latent bug -Fixed the offending code. -Made GET_SYMBOL define more safe by adding some paraentheses -Used GET_SYMBOL in modules.c in order to increase conformance with code used in querymodules.c The patch resolves the issues, but may not be "the best" (TM) way of solving them.
- The union does not fix the bug, even if you don't consider other compilers than gcc. The code assumes that the size and representation of an enumeration type is identical to that of signed int. This is a guarantee gcc doesn't make: in particular, it is not true on any system that defaults to -fshort-enums. To actually fix the code, make PangoStyle e.a. typedefs for int, or rework the code to make find_field_any not use find_field. - An intermediate conversion through void* also does not fix any bug. It tells the compiler "trust me, I know what I'm doing", but you left the aliasing violation in. - If you change filename from char* to FcChar8*, you need to update the code that expects it to be a char*. pango already contains invalid implicit conversions between pointers, but that is not a reason to introduce even more.
(In reply to comment #2) > - The union does not fix the bug, even if you don't consider other compilers > than gcc. The code assumes that the size and representation of an enumeration > type is identical to that of signed int. This is a guarantee gcc doesn't make: > in particular, it is not true on any system that defaults to -fshort-enums. To > actually fix the code, make PangoStyle e.a. typedefs for int, or rework the > code to make find_field_any not use find_field. I agree, I didn't think that far, updating patch to reflect this. Also made an assert which I can't see any conceivable way of hitting, but I did it anyway. > - An intermediate conversion through void* also does not fix any bug. It tells > the compiler "trust me, I know what I'm doing", but you left the aliasing > violation in. Of course, but this cannot be fixed within this package and will not be fixed upstream and they think this is the way it should be done. See "http://mail.gnome.org/archives/gtk-devel-list/2004-March/msg00044.html" In my opinion it is better to get rid of the warning. The use of GET_SYMBOL makes the code more conform. If you see any other way of solving this problem, please let me know. > - If you change filename from char* to FcChar8*, you need to update the code > that expects it to be a char*. pango already contains invalid implicit > conversions between pointers, but that is not a reason to introduce even more. This is a struct internal to pangofc-fontmap.c, as there is no internal conversion problems. There should not be any problems outside this file. If there is more bad coding practices elsewhere, I can't make any promises it wouldn't break there, but then those were errors already.
Created attachment 108158 [details, diff] Fixes strict aliasing, and some minor conformance issue, avoid a latent bug Updated union problems with former patch, added an assert.
(In reply to comment #3) > (In reply to comment #2) > > - The union does not fix the bug, even if you don't consider other compilers > > than gcc. The code assumes that the size and representation of an enumeration > > type is identical to that of signed int. This is a guarantee gcc doesn't make: > > in particular, it is not true on any system that defaults to -fshort-enums. To > > actually fix the code, make PangoStyle e.a. typedefs for int, or rework the > > code to make find_field_any not use find_field. > > I agree, I didn't think that far, updating patch to reflect this. Also made an > assert which I can't see any conceivable way of hitting, but I did it anyway. The problem is exactly that the assert /can/ be hit. Again I'll point out the -fshort-enums option provided by gcc. #include <stdio.h> enum E { X }; int main(void) { printf("sizeof(enum E) == %zu, sizeof(int) == %zu\n", sizeof(enum E), sizeof(int)); } This prints 4 and 4 on my system when compiled with default options, and pango expects the two numbers to be equal, but I get 1 and 4 when I compile this with -fshort-enums. > > - An intermediate conversion through void* also does not fix any bug. It tells > > the compiler "trust me, I know what I'm doing", but you left the aliasing > > violation in. > > Of course, but this cannot be fixed within this package and will not be fixed > upstream and they think this is the way it should be done. > > See "http://mail.gnome.org/archives/gtk-devel-list/2004-March/msg00044.html" > > In my opinion it is better to get rid of the warning. The use of GET_SYMBOL > makes the code more conform. It does not make the code conform any more than `append-flags -Wno-strict-aliasing` would. :) > If you see any other way of solving this problem, please let me know. One way of solving this is to use an intermediate pointer-to-void (or gpointer), and use memcpy() to store that in the actual function pointer. This is not guaranteed to work by the C standard, but is guaranteed to work by SUSv3. Another way of solving this -- relying on a GCC extension -- is to use a union exactly the same way you modified the first aliasing violation. In this case, it would be a valid fix as far as GCC is concerned. > > - If you change filename from char* to FcChar8*, you need to update the code > > that expects it to be a char*. pango already contains invalid implicit > > conversions between pointers, but that is not a reason to introduce even more. > > This is a struct internal to pangofc-fontmap.c, as there is no internal > conversion problems. There should not be any problems outside this file. If > there is more bad coding practices elsewhere, I can't make any promises it > wouldn't break there, but then those were errors already. From pangofc-fontmap.c: static guint pango_fc_coverage_key_hash (PangoFcCoverageKey *key) { return g_str_hash (key->filename) ^ key->id; } static gboolean pango_fc_coverage_key_equal (PangoFcCoverageKey *key1, PangoFcCoverageKey *key2) { return key1->id == key2->id && strcmp (key1->filename, key2->filename) == 0; } These rely on key->filename being a pointer-to-char, or being implicitly converted to pointer-to-char. pointer-to-FcChar8 is cannot implicitly be converted to pointer-to-char, and you should have got warnings from the compiler about that; you need to add casts there. There are more similar examples.
(In reply to comment #5) > (In reply to comment #3) > > (In reply to comment #2) > > > - The union does not fix the bug, even if you don't consider other compilers > > > than gcc. The code assumes that the size and representation of an enumeration > > > type is identical to that of signed int. This is a guarantee gcc doesn't make: > > > in particular, it is not true on any system that defaults to -fshort-enums. To > > > actually fix the code, make PangoStyle e.a. typedefs for int, or rework the > > > code to make find_field_any not use find_field. > > > > I agree, I didn't think that far, updating patch to reflect this. Also made an > > assert which I can't see any conceivable way of hitting, but I did it anyway. > > The problem is exactly that the assert /can/ be hit. Again I'll point out the > -fshort-enums option provided by gcc. > > #include <stdio.h> > enum E { X }; > int main(void) { > printf("sizeof(enum E) == %zu, sizeof(int) == %zu\n", sizeof(enum E), > sizeof(int)); > } > > This prints 4 and 4 on my system when compiled with default options, and pango > expects the two numbers to be equal, but I get 1 and 4 when I compile this with > -fshort-enums. Yes, of course that's true, but that problem doesn't arise in the new patch. The union never assumes any int in its definition. It consists solely of enums. What it does assume however is that all enums compile to the same type. Referring to the C standrd, a char is guaranteed to be able to hold a positive number of 127 or less. Meaning that as long as no enums contain more that 128 values. I can't see any reasonable compiler choosing to compile the enums to different sizes. > > > - An intermediate conversion through void* also does not fix any bug. It tells > > > the compiler "trust me, I know what I'm doing", but you left the aliasing > > > violation in. > > > > Of course, but this cannot be fixed within this package and will not be fixed > > upstream and they think this is the way it should be done. > > > > See "http://mail.gnome.org/archives/gtk-devel-list/2004-March/msg00044.html" > > > > In my opinion it is better to get rid of the warning. The use of GET_SYMBOL > > makes the code more conform. > > It does not make the code conform any more than `append-flags > -Wno-strict-aliasing` would. :) Perhaps I wasn't clear enough. The use of GET_SYMBOL makes it internally conform. The GET_SYMBOL macro is used elsewhere to do exactly the same task. O have never made any claim it would be standard conform. > > If you see any other way of solving this problem, please let me know. > > One way of solving this is to use an intermediate pointer-to-void (or > gpointer), and use memcpy() to store that in the actual function pointer. This > is not guaranteed to work by the C standard, but is guaranteed to work by > SUSv3. Another way of solving this -- relying on a GCC extension -- is to use a > union exactly the same way you modified the first aliasing violation. In this > case, it would be a valid fix as far as GCC is concerned. Again, this is probably true, but even though it would be more standard conform, I don't like to work around upstreams lack of conformance if it is not strictly necessary. But of course, this is not my show to run, I'm just trying to patch myself away from a very real bug that's causing firefox to crash for me. > > > - If you change filename from char* to FcChar8*, you need to update the code > > > that expects it to be a char*. pango already contains invalid implicit > > > conversions between pointers, but that is not a reason to introduce even more. > > > > This is a struct internal to pangofc-fontmap.c, as there is no internal > > conversion problems. There should not be any problems outside this file. If > > there is more bad coding practices elsewhere, I can't make any promises it > > wouldn't break there, but then those were errors already. > > From pangofc-fontmap.c: > > static guint > pango_fc_coverage_key_hash (PangoFcCoverageKey *key) > { > return g_str_hash (key->filename) ^ key->id; > } > > static gboolean > pango_fc_coverage_key_equal (PangoFcCoverageKey *key1, > PangoFcCoverageKey *key2) > { > return key1->id == key2->id && strcmp (key1->filename, key2->filename) == 0; > } > > These rely on key->filename being a pointer-to-char, or being implicitly > converted to pointer-to-char. pointer-to-FcChar8 is cannot implicitly be > converted to pointer-to-char, and you should have got warnings from the > compiler about that; you need to add casts there. There are more similar > examples. > Do you get warnings about this? That must be one piece of esoteric hardware you're working on. According to the fontconfig spec, FnChar8 will be a pointer to a char of size 8bits, as long as the compiler can handle it. It is clear that we could write our way around this, but in the case anyone should get the crazy idea to change anything, I would much rether see the warning, than quietly suppress it. As an endnote, I'm very aware that these issues could be solved easily by writing more complex code. I'm just not in the mood to reduce any performance if not strictly necessary.
(In reply to comment #6) > Yes, of course that's true, but that problem doesn't arise in the new patch. > The union never assumes any int in its definition. It consists solely of enums. Ah, sorry about that, I didn't look closely enough. Yes, then personally, I agree that that part should be okay. > Perhaps I wasn't clear enough. The use of GET_SYMBOL makes it internally > conform. The GET_SYMBOL macro is used elsewhere to do exactly the same task. O > have never made any claim it would be standard conform. Okay, using the GET_SYMBOL macro with its current definition does not fix the bug as far as C is concerned, and hides the warning. Others may disagree, but personally I would then prefer to leave that part of the code untouched, keeping the warning in. :) It should not be any more or less likely to break with that part of your patch. However, if you want to try it, and find this to be untrue, then no objections from me. > Again, this is probably true, but even though it would be more standard > conform, I don't like to work around upstreams lack of conformance if it is not > strictly necessary. But of course, this is not my show to run, I'm just trying > to patch myself away from a very real bug that's causing firefox to crash for > me. That's understandable. > > These rely on key->filename being a pointer-to-char, or being implicitly > > converted to pointer-to-char. pointer-to-FcChar8 is cannot implicitly be > > converted to pointer-to-char, and you should have got warnings from the > > compiler about that; you need to add casts there. There are more similar > > examples. > > Do you get warnings about this? That must be one piece of esoteric hardware > you're working on. Pretty standard x86 Linux system, without any warning options not enabled by pango itself. FcChar8 is a typedef for unsigned char. char * is not unsigned char *, and there are no implicit conversions between the two, but casting between the two is appropriate in this case. > As an endnote, I'm very aware that these issues could be solved easily by > writing more complex code. I'm just not in the mood to reduce any performance > if not strictly necessary. Adding casts between pointer types generates no extra assembly code with GCC even with -O0, thankfully, so at least that part is safe to do.
Created attachment 108372 [details, diff] Fixes strict aliasing, and some minor conformance issue, avoid a latent bug Did a few changes. 1. Still using the first union, but moved the asserts to the configure script. 2. Still using GET_SYMBOL to increase internal conformance, but moved the macro definition to a header, and defined it without the cast per your desire. 3. Using a temporary union to avoid changing much of the code. Hopefully this union will be optimized away. 4. Wrote some configure stuff, to ensure the typecasting should be troublefree. I have never written a configure script before in my life, so I urge you to check those parts of the patch with a high degree of scrutiny. I have only patched the input files for autoconf, and not the results file, so you need to run autogen.sh to remake them. This is done partly to increase readability of the patch, and partly to ensure that I haven't made any dependency assumptions from my local system.
(In reply to comment #8) > 2. Still using GET_SYMBOL to increase internal conformance, but moved the macro > definition to a header, and defined it without the cast per your desire. Actually, you did it with the double cast. :) Consider my objection to this withdrawn, though; I still think it's not a good idea, but it's probably a worse idea to patch it in a way that upstream is unlikely to accept. > 3. Using a temporary union to avoid changing much of the code. Hopefully this > union will be optimized away. At least to me, this looks sane. > 4. Wrote some configure stuff, to ensure the typecasting should be troublefree. > > I have never written a configure script before in my life, so I urge you to > check those parts of the patch with a high degree of scrutiny. I have only > patched the input files for autoconf, and not the results file, so you need to > run autogen.sh to remake them. This is done partly to increase readability of > the patch, and partly to ensure that I haven't made any dependency assumptions > from my local system. I know only little about autotools myself. The idea is as good as you can reasonably get, but this part definitely needs a comment from someone else. :)
(In reply to comment #9) > (In reply to comment #8) > > 2. Still using GET_SYMBOL to increase internal conformance, but moved the macro > > definition to a header, and defined it without the cast per your desire. > > Actually, you did it with the double cast. :) Consider my objection to this > withdrawn, though; I still think it's not a good idea, but it's probably a > worse idea to patch it in a way that upstream is unlikely to accept. I must have been sleepwalking, sorry. Either way my handling in the header makes no sense, as I left the original version commented and made an exact copy below. Please fix this as you see fit. I too disagree with upstream, but as we have both pointed out, no need to try to fix something they have no intention of changing. PS: Sleepwalking can also cause one to ignore that ones /etc/ld.so.conf is broken, leading firefox to crash with another version of the library than that one is trying to patch. :-/
