14362 2012-07-12 19:57:00 +0000 Incorrect ioctl declaration 2013-05-15 14:28:49 +0000 1 1 1 Unclassified glibc libc 2.15 All All NEW P2 normal --- 1 torvalds unassigned carlos drepper.fsp kosaki.motohiro mmaruska ondra 56292 torvalds 2012-07-12 19:57:42 +0000 Both SuS and the man-pages agree: the correct declaration for ioctl() is extern int ioctl(int fd, int request, ...); but glibc headers incorrectly have "unsigned long" for the request. This means that this simple C program results in an incorrect type conflict error: #include <sys/ioctl.h> extern int ioctl(int fd, int request, ...); even though the declaration clearly matches documentation. Perhaps more importantly, it's misleading for anybody who actually reads the header files. That's rare, but still.. We had this discussion on the subsurface mailing list, because OS X has the same buggy declaration, and what is worse, the OS X kernel is apparently buggy too, because doing int action = (level ? TIOCSBRK : TIOCCBRK); ... ioctl(fd, action, ...); will sign-extend the 'int' into 'unsigned long', and the OS X FreeBSD-based kernel apparently actually looks at all 64 bits of an 'unsigned long' when it does things, leading to actual bugs (ie ioctl returning ENOTTY because the sign-extended 64-bit value is not recognized). So having "unsigned long" in there can result in actual bugs. You don't see this on Linux, because 64-bit Linux will correctly truncate the request value to 32 bits (and then internally uses 'unsigned' to make sure no sign extension happens, but that's a separate issue). So please fix the ioctl() declaration. "unsigned long" is misleading and actively incorrect and can cause bugs on non-Linux operating systems. In case anybody uses glibc on OS X, I would suggest doing int ioctl(int fd, int __request, ...) { unsigned long request = (unsigned int) __request; ... to avoid the OS X bug. 56502 6551 carlos_odonell 2012-07-24 03:02:29 +0000 Created attachment 6551 WIP patch to convert ioctl request type to `int'. 56503 carlos_odonell 2012-07-24 03:07:30 +0000 The history in the glibc ChangeLogs indicate that it was changed from `int' to `unsigned long int' around 1993 in order to align itself with BSD 4.4. I consider this a serious issue for compliance with POSIX which has ioctl defined as `int ioctl(int fildes, int request, ... /* arg */);'. I'll float around a WIP patch for discussion around the pros/cons of changing this established interface. 56504 carlos_odonell 2012-07-24 03:58:52 +0000 Notes: http://sourceware.org/ml/libc-alpha/2012-07/msg00446.html 56521 kosaki.motohiro 2012-07-24 15:26:44 +0000 Link to man-pages bugzilla. https://bugzilla.kernel.org/show_bug.cgi?id=42705 56522 torvalds 2012-07-24 17:41:28 +0000 (In reply to comment #4) > Link to man-pages bugzilla. > > https://bugzilla.kernel.org/show_bug.cgi?id=42705 I think that one brings up interesting points, but is not actually correct or really relevant. But it does show some of the confusion in this area, yes, which makes it interesting. The Linux kernel internally uses 'unsigned int' exactly because sign extension is such a ^&%*@ confusing pain in the ass when you have random big integers (and yes, they really historically are big, with high bits used for various things). At the same time, in Linux, the kernel doesn't even really care: the sign extension is irrelevant as long as the comparison is always done in 32 bits. So the important part is not the "unsigned int", but that it is *some* kind of "int". Not a "long". But I point out the internal use of "unsigned int", because I do think it's nice: it avoids the worry about sign extension, and makes things that have the high bit set not have any special behavior. So "unsigned int" is a better type, I think. But when that bugzilla entry says that int x = IOCTL; if (x == IOCTL) /* ... */ Will generate a "comparison always false due to limited range of data type" warning from gcc, and presumably will indeed compare false, which is clearly not the expected result. it is actually incorrect. Because the IOCTL value is (or rather - can be) an 'unsigned int', when you do the comparison, the C typecasting rules will do the comparison in 'unsigned int', and everything will be correct. The manpages bugzilla entry is correct about this being *confusing*, though, and might generate warnings about comparing signed and unsigned values. Now, if you do things in "long", you really can get screwed. Then you might sign-extend one and compare it against a non-sign-extended one, and then you're really screwed. That is, in fact, exactly what happens on OS X right now due to the OS X *incorrect* use of "unsigned long". So I think the manpages bugzilla is wrong, but does bring up a valid source of confusion. I absolutely agree that we would have been *much* better off if POSIX had just said that the argument is "unsigned int". But that's not what they did. And I think we should conform to POSIX, and admit that it's ugly, but that it's absolutely better than "unsigned long", which is just broken and causes actual bugs. (Side note: the bugzilla would be correct if some IOCTL value is declared as a single constant, eg #define IOCTL 2147483648 because such a constant would be implicitly converted to "long", and then the comparison with an "int" (which would be sign-extended) would never be true. However, that's not how the ioctl's with the high bits set are defined, they are done by shifting values that are of type "unsigned int" around, or done as hex constants, both of which will stay as "unsigned int"). And mixing "unsigned int" and "int" values can be *confusing*, but it won't be wrong - nothing will be extended to "long", but "int" will be silently converted to "unsigned int". Yeah, C type conversion can be subtle. 59487 schwab 2013-01-15 11:32:46 +0000 Current FreeBSD still uses unsigned long. Note that the POSIX ioctl is for STREAMS only and marked obsolete. I think this should be considered a documentation bug. 59496 torvalds 2013-01-15 17:03:58 +0000 (In reply to comment #6) > Current FreeBSD still uses unsigned long. Note that the POSIX ioctl is for > STREAMS only and marked obsolete. I think this should be considered a > documentation bug. Why would you consider it a documentation bug, when it clearly results in real bugs, and the FreeBSD/Darwin type is *wrong*? The fact is, casting it to unsigned int is the correct thing to do. There are actual bug reports about BSD itself due to this exact same problem. If you google for it, you'll see this discussed on the gnulib lists, on the perl mailing lists, etc etc. The FreeBSD/Darwin ioctl() declaration is *wrong*. And it's not wrong because it actually *uses* 64 bits. No, it is "unsigned long" just because of historical accidents, back when "long" was 32-bit. Even *Apple* knows it is a bug, and actually uses "unsigned int" in their own code. See for example http://valgrind-variant.googlecode.com/svn-history/r66/trunk/valgrind/coregrind/m_syswrap/syswrap-darwin.c which has a big Apple copyright, and look for ioctl. Look at the code that actually reads the arguments (PRE_REG_READ3): the "request" argument is read as an "unsigned int". As noted, this does not actually cause problems on Linux, because unlike FreeBSD, Linux knows what the f*ck it is doing, and just ignores the upper bits exactly because of possible sign confusion. So I don't care as a Linux person. But I think glibc should try to actually help Darwin. Because there are ioctl commands on Darwin that have bit 31 set, and they cause problems. Go and look at the BSD sys/sys/ioccom.h file itself if you don't believe me. The constants are clearly 32-bit ones, with IOC_IN being bit#31 (ok, so I don't know where the actual Darwin SDK is, and I wouldn't want to download it anyway, but I can find the FreeBSD sources, and there is even a comment there talking about how the *high* three bits specify the direction. And they are high bits only in 32-bit. See for example http://fxr.watson.org/fxr/source/sys/ioccom.h The fact is, the FreeBSD/Darwin *declaration* is wrong. And no, it's not just wrong in the documentation sense. It's wrong in the "real code breaks" sense. 61538 ondra 2013-05-15 10:09:47 +0000 What is status of patch? 61540 carlos 2013-05-15 14:28:49 +0000 (In reply to comment #8) > What is status of patch? Someone needs to work through all of the feedback that we received on list for the MIPS, Power and TILE implementations. Link provided in comment 3. Off the top of my head: (a) Binary compatibility in the cases where ioctl has a C wrapper in glibc. (b) MIPS, Power, and TILE interfaces when older kernels are used need to be verified with help from those maintainers. (c) Validate that the changes, including casts in INLINE_SYSCALL don't break what strace shows for the arguments to ioctl (even if the claim is that strace looks at only the low 16-bits). (c) At a minimum look over the ruby code and verify their ioctl glue doesn't break with this change. 6551 2012-07-24 03:02:00 +0000 2012-07-24 03:02:29 +0000 WIP patch to convert ioctl request type to `int'. bz14362.diff text/plain 4764 carlos_odonell MjAxMi0wNy0yMyAgQ2FybG9zIE8nRG9uZWxsICA8Y2FybG9zX29kb25lbGxAbWVudG9yLmNvbT4K CgkqIGluY2x1ZGUvc3lzL2lvY3RsLmg6IFVzZSBgaW50JyBmb3IgX19yZXF1ZXN0IGFyZ3VtZW50 CglvZiBmdW5jdGlvbiBfX2lvY3RsLgoJKiBtaXNjL2lvY3RsLmMgKF9faW9jdGwpOiBMaWtld2lz ZS4KCSogbWlzYy9zeXMvaW9jdGwuaDogTGlrZXdpc2UuCgkqIHN5c2RlcHMvbWFjaC9odXJkL2lv Y3RsLmMgKF9faW9jdGwpOiBMaWtld2lzZS4KCSogc3lzZGVwcy91bml4L3N5c3YvbGludXgvcG93 ZXJwYy9pb2N0bC5jIChfX2lvY3RsKTogTGlrZXdpc2UuCgkqIHBvcnRzL3N5c2RlcHMvdW5peC9z eXN2L2xpbnV4L21pcHMvbWlwczY0L242NC9pb2N0bC5TOgoJQWRkIEZJWE1FLgoJKiBwb3J0cy9z eXNkZXBzL3VuaXgvc3lzdi9saW51eC90aWxlL3RpbGVneC9pb2N0bC5TOgoJQWRkIEZJWE1FLgoK ZGlmZiAtLWdpdCBhL2luY2x1ZGUvc3lzL2lvY3RsLmggYi9pbmNsdWRlL3N5cy9pb2N0bC5oCmlu ZGV4IGViYWRkNTIuLjg4M2MxMmUgMTAwNjQ0Ci0tLSBhL2luY2x1ZGUvc3lzL2lvY3RsLmgKKysr IGIvaW5jbHVkZS9zeXMvaW9jdGwuaApAQCAtMiw1ICsyLDUgQEAKICNpbmNsdWRlIDxtaXNjL3N5 cy9pb2N0bC5oPgogCiAvKiBOb3cgZGVmaW5lIHRoZSBpbnRlcm5hbCBpbnRlcmZhY2VzLiAgKi8K LWV4dGVybiBpbnQgX19pb2N0bCAoaW50IF9fZmQsIHVuc2lnbmVkIGxvbmcgaW50IF9fcmVxdWVz dCwgLi4uKTsKK2V4dGVybiBpbnQgX19pb2N0bCAoaW50IF9fZmQsIGludCBfX3JlcXVlc3QsIC4u Lik7CiAjZW5kaWYKZGlmZiAtLWdpdCBhL21pc2MvaW9jdGwuYyBiL21pc2MvaW9jdGwuYwppbmRl eCA2MTZlZTJkLi5hNWQ0YTgxIDEwMDY0NAotLS0gYS9taXNjL2lvY3RsLmMKKysrIGIvbWlzYy9p b2N0bC5jCkBAIC0xLDQgKzEsNCBAQAotLyogQ29weXJpZ2h0IChDKSAxOTkxLCA5MywgOTQsIDk1 LCA5NiwgOTcgRnJlZSBTb2Z0d2FyZSBGb3VuZGF0aW9uLCBJbmMuCisvKiBDb3B5cmlnaHQgKEMp IDE5OTEtMjAxMiBGcmVlIFNvZnR3YXJlIEZvdW5kYXRpb24sIEluYy4KICAgIFRoaXMgZmlsZSBp cyBwYXJ0IG9mIHRoZSBHTlUgQyBMaWJyYXJ5LgogCiAgICBUaGUgR05VIEMgTGlicmFyeSBpcyBm cmVlIHNvZnR3YXJlOyB5b3UgY2FuIHJlZGlzdHJpYnV0ZSBpdCBhbmQvb3IKQEAgLTIzLDcgKzIz LDcgQEAKIGludAogX19pb2N0bCAoZmQsIHJlcXVlc3QpCiAgICAgIGludCBmZDsKLSAgICAgdW5z aWduZWQgbG9uZyBpbnQgcmVxdWVzdDsKKyAgICAgaW50IHJlcXVlc3Q7CiB7CiAgIF9fc2V0X2Vy cm5vIChFTk9TWVMpOwogICByZXR1cm4gLTE7CmRpZmYgLS1naXQgYS9taXNjL3N5cy9pb2N0bC5o IGIvbWlzYy9zeXMvaW9jdGwuaAppbmRleCBjNDljNGFiLi44MzQwYTBhIDEwMDY0NAotLS0gYS9t aXNjL3N5cy9pb2N0bC5oCisrKyBiL21pc2Mvc3lzL2lvY3RsLmgKQEAgLTEsNCArMSw0IEBACi0v KiBDb3B5cmlnaHQgKEMpIDE5OTEsIDkyLCA5MywgOTQsIDk2LCA5OCwgOTkgRnJlZSBTb2Z0d2Fy ZSBGb3VuZGF0aW9uLCBJbmMuCisvKiBDb3B5cmlnaHQgKEMpIDE5OTEtMjAxMiBGcmVlIFNvZnR3 YXJlIEZvdW5kYXRpb24sIEluYy4KICAgIFRoaXMgZmlsZSBpcyBwYXJ0IG9mIHRoZSBHTlUgQyBM aWJyYXJ5LgogCiAgICBUaGUgR05VIEMgTGlicmFyeSBpcyBmcmVlIHNvZnR3YXJlOyB5b3UgY2Fu IHJlZGlzdHJpYnV0ZSBpdCBhbmQvb3IKQEAgLTM4LDcgKzM4LDcgQEAgX19CRUdJTl9ERUNMUwog LyogUGVyZm9ybSB0aGUgSS9PIGNvbnRyb2wgb3BlcmF0aW9uIHNwZWNpZmllZCBieSBSRVFVRVNU IG9uIEZELgogICAgT25lIGFyZ3VtZW50IG1heSBmb2xsb3c7IGl0cyBwcmVzZW5jZSBhbmQgdHlw ZSBkZXBlbmQgb24gUkVRVUVTVC4KICAgIFJldHVybiB2YWx1ZSBkZXBlbmRzIG9uIFJFUVVFU1Qu ICBVc3VhbGx5IC0xIGluZGljYXRlcyBlcnJvci4gICovCi1leHRlcm4gaW50IGlvY3RsIChpbnQg X19mZCwgdW5zaWduZWQgbG9uZyBpbnQgX19yZXF1ZXN0LCAuLi4pIF9fVEhST1c7CitleHRlcm4g aW50IGlvY3RsIChpbnQgX19mZCwgaW50IF9fcmVxdWVzdCwgLi4uKSBfX1RIUk9XOwogCiBfX0VO RF9ERUNMUwogCmRpZmYgLS1naXQgYS9wb3J0cy9zeXNkZXBzL3VuaXgvc3lzdi9saW51eC9taXBz L21pcHM2NC9uNjQvaW9jdGwuUyBiL3BvcnRzL3N5c2RlcHMvdW5peC9zeXN2L2xpbnV4L21pcHMv bWlwczY0L242NC9pb2N0bC5TCmluZGV4IDNmZjBkMGUuLjFjYjM1NDEgMTAwNjQ0Ci0tLSBhL3Bv cnRzL3N5c2RlcHMvdW5peC9zeXN2L2xpbnV4L21pcHMvbWlwczY0L242NC9pb2N0bC5TCisrKyBi L3BvcnRzL3N5c2RlcHMvdW5peC9zeXN2L2xpbnV4L21pcHMvbWlwczY0L242NC9pb2N0bC5TCkBA IC0xLDQgKzEsNCBAQAotLyogQ29weXJpZ2h0IDIwMDMsIDIwMDUgRnJlZSBTb2Z0d2FyZSBGb3Vu ZGF0aW9uLCBJbmMuCisvKiBDb3B5cmlnaHQgMjAwMy0yMDEyIEZyZWUgU29mdHdhcmUgRm91bmRh dGlvbiwgSW5jLgogICAgVGhpcyBmaWxlIGlzIHBhcnQgb2YgdGhlIEdOVSBDIExpYnJhcnkuCiAK ICAgIFRoZSBHTlUgQyBMaWJyYXJ5IGlzIGZyZWUgc29mdHdhcmU7IHlvdSBjYW4gcmVkaXN0cmli dXRlIGl0IGFuZC9vcgpAQCAtMTksNiArMTksOSBAQAogCiAjaW5jbHVkZSA8c3lzL2FzbS5oPgog CisvKiBGSVhNRTogTm93IHRoYXQgdGhlIHVzZXJzcGFjZSBBUEkgY2xhaW1zIHRoYXQgdGhlIGFy Z3VtZW50CisgICAgICAgICAgaWYgb2YgdHlwZSBgaW50JyBkb2VzIHRoaXMgY29kZSBuZWVkIHVw ZGF0aW5nPyAgKi8KKwogLyogU2lnbi1leHRlbmQgdGhlIGlvY3RsIG51bWJlciwgc2luY2UgdGhl IGtlcm5lbCB3YW50cyBpdCBhcyBhCiAgICBzaWduLWV4dGVuZGVkIDMyLWJpdCB2YWx1ZSwgYnV0 IG91ciBwcm90b3R5cGUgaXMgdGhhdCBvZiBhIGxvbmcuICAqLwogCmRpZmYgLS1naXQgYS9wb3J0 cy9zeXNkZXBzL3VuaXgvc3lzdi9saW51eC90aWxlL3RpbGVneC9pb2N0bC5TIGIvcG9ydHMvc3lz ZGVwcy91bml4L3N5c3YvbGludXgvdGlsZS90aWxlZ3gvaW9jdGwuUwppbmRleCBlOTY2YzRkLi5i NjE1OTJjIDEwMDY0NAotLS0gYS9wb3J0cy9zeXNkZXBzL3VuaXgvc3lzdi9saW51eC90aWxlL3Rp bGVneC9pb2N0bC5TCisrKyBiL3BvcnRzL3N5c2RlcHMvdW5peC9zeXN2L2xpbnV4L3RpbGUvdGls ZWd4L2lvY3RsLlMKQEAgLTEsNCArMSw0IEBACi0vKiBDb3B5cmlnaHQgKEMpIDIwMTEgRnJlZSBT b2Z0d2FyZSBGb3VuZGF0aW9uLCBJbmMuCisvKiBDb3B5cmlnaHQgKEMpIDIwMTEtMjAxMiBGcmVl IFNvZnR3YXJlIEZvdW5kYXRpb24sIEluYy4KICAgIFRoaXMgZmlsZSBpcyBwYXJ0IG9mIHRoZSBH TlUgQyBMaWJyYXJ5LgogICAgQ29udHJpYnV0ZWQgYnkgQ2hyaXMgTWV0Y2FsZiA8Y21ldGNhbGZA dGlsZXJhLmNvbT4sIDIwMTEuCiAKQEAgLTE2LDYgKzE2LDggQEAKICAgIExpY2Vuc2UgYWxvbmcg d2l0aCB0aGUgR05VIEMgTGlicmFyeS4gIElmIG5vdCwgc2VlCiAgICA8aHR0cDovL3d3dy5nbnUu b3JnL2xpY2Vuc2VzLz4uICAqLwogCisvKiBGSVhNRTogTm93IHRoYXQgdGhlIHVzZXJzcGFjZSBB UEkgY2xhaW1zIHRoYXQgdGhlIGFyZ3VtZW50CisgICAgICAgICAgaWYgb2YgdHlwZSBgaW50JyBk b2VzIHRoaXMgY29kZSBuZWVkIHVwZGF0aW5nPyAgKi8KIAogLyogVElMRS1HeCBzcGVjaWZpZXMg dGhhdCAidW5zaWduZWQgaW50IiBpcyBzaWduIGV4dGVuZGVkIGluIHRoZSBoaWdoCiAgICAzMiBi aXRzLiAgQnV0IHNpbmNlIHRoZSB1c2Vyc3BhY2UgQVBJIGNsYWltcyB0byBiZSAidW5zaWduZWQg bG9uZyIsCmRpZmYgLS1naXQgYS9zeXNkZXBzL21hY2gvaHVyZC9pb2N0bC5jIGIvc3lzZGVwcy9t YWNoL2h1cmQvaW9jdGwuYwppbmRleCA1NDNkNDM3Li42YWVlY2E0IDEwMDY0NAotLS0gYS9zeXNk ZXBzL21hY2gvaHVyZC9pb2N0bC5jCisrKyBiL3N5c2RlcHMvbWFjaC9odXJkL2lvY3RsLmMKQEAg LTM2LDcgKzM2LDcgQEAKIC8qIFBlcmZvcm0gdGhlIEkvTyBjb250cm9sIG9wZXJhdGlvbiBzcGVj aWZpZWQgYnkgUkVRVUVTVCBvbiBGRC4KICAgIFRoZSBhY3R1YWwgdHlwZSBhbmQgdXNlIG9mIEFS RyBhbmQgdGhlIHJldHVybiB2YWx1ZSBkZXBlbmQgb24gUkVRVUVTVC4gICovCiBpbnQKLV9faW9j dGwgKGludCBmZCwgdW5zaWduZWQgbG9uZyBpbnQgcmVxdWVzdCwgLi4uKQorX19pb2N0bCAoaW50 IGZkLCBpbnQgcmVxdWVzdCwgLi4uKQogewogI2lmZGVmIE1BQ0hfTVNHX1RZUEVfQ0hBUgogICAv KiBNYXAgaW5kaXZpZHVhbCB0eXBlIGZpZWxkcyB0byBNYWNoIElQQyB0eXBlcy4gICovCmRpZmYg LS1naXQgYS9zeXNkZXBzL3VuaXgvc3lzdi9saW51eC9wb3dlcnBjL2lvY3RsLmMgYi9zeXNkZXBz L3VuaXgvc3lzdi9saW51eC9wb3dlcnBjL2lvY3RsLmMKaW5kZXggZThmNWQxNi4uYjQ3MGFlOCAx MDA2NDQKLS0tIGEvc3lzZGVwcy91bml4L3N5c3YvbGludXgvcG93ZXJwYy9pb2N0bC5jCisrKyBi L3N5c2RlcHMvdW5peC9zeXN2L2xpbnV4L3Bvd2VycGMvaW9jdGwuYwpAQCAtMjUsNyArMjUsNyBA QAogICAgdXNpbmcgdGhlIG5ldy1zdHlsZSBzdHJ1Y3QgdGVybWlvcywgYW5kIHRyYW5zbGF0ZSB0 aGVtIHRvIG9sZC1zdHlsZS4gICovCiAKIGludAotX19pb2N0bCAoaW50IGZkLCB1bnNpZ25lZCBs b25nIGludCByZXF1ZXN0LCAuLi4pCitfX2lvY3RsIChpbnQgZmQsIGludCByZXF1ZXN0LCAuLi4p CiB7CiAgIHZvaWQgKmFyZzsKICAgdmFfbGlzdCBhcDsK