This is an old revision of the document!
====== Todo: Audit the use of << and >> (shift) operators in gPXE ====== Per a discussion by mcb30: <code> [3:11pm] mcb30: rwcr: I am puzzled by this (1<<32) issue [3:11pm] mcb30: (I just replied to your e-mail) [3:12pm] ogreinside joined the chat room. [3:13pm] rwcr: Hmm. I didn't think about the overflow; that *should* make it work. [3:13pm] mcb30: It doesn't [3:13pm] rwcr: Aha. [3:13pm] mcb30: I just tested compiling the damn code [3:13pm] mcb30: I understand why the x86 code fails, but [3:13pm] rwcr: The x86 processor truncates CL to the bottom 5 bits when using it as the shift. [3:13pm] rwcr: So a shift by 32 turns into a shift by 0. [3:13pm] mcb30: Yes, but that is the kind of artefact that should not be visible at this level [3:14pm] mcb30: i.e. if x86 has that behaviour, but C says that << is a "pure" shift, then the compiler should insert a >=32 check on every shift [3:14pm] mdc: 32 mod 32 :) [3:14pm] mcb30: This must be mandated by the language spec somewhere [3:15pm] rwcr: I suspect it says shifts by more than the size of the variable have undefined or implementation-defined behavior. [3:15pm] mcb30: What concerns me is that I suspect I have relied on what I thought would be the correct behaviour in various bits of code over the years [3:16pm] mdc: https://www.securecoding.cert.org/confluence/display/seccode/INT13-C.+Use+bitwise+operators+only+on+unsigned+operands [3:16pm] rwcr: "The results of some bitwise operations on signed integers" are implementation-defined. [3:16pm] mdc: (not quite the case here, but interesting) [3:16pm] rwcr: Try it with 1u << 32. [3:16pm] rwcr: C90 6.3, C95 6.5. [3:17pm] mcb30: I was using 1UL already in my test [3:17pm] rwcr: Eek. [3:18pm] rwcr: I get 0 for all types of 1 << (var == 32) shift. [3:18pm] mcb30: http://paste.etherboot.org:80/?page=view&id=1249931905 [3:19pm] mcb30: How are you forcing var to be variable (i.e. preventing the compiler optimising it to a constant shift)? [3:19pm] mcb30: (I made it a global variable to prevent this from happening) [3:19pm] rwcr: Ah, that'd be it - I used a local. [3:19pm] rwcr: Sure enough, turns out 1 now. [3:19pm] rwcr: This is rather scary. [3:20pm] mcb30: I found a mailing list post with the same problem, and someone saying that it is indeed undefined behaviour [3:21pm] rwcr: Disassembly shows a straight shl %cl,%eax without any checking. [3:21pm] rwcr: I think the behavior of that opcode changed on the 286 or so to truncate to 5 bits. [3:23pm] mcb30: 6.5.7 of C99 does indeed say "undefined" [3:23pm] mcb30: Hmmm [3:24pm] mcb30: I really don't feel like auditing every use of shift operators in the gPXE codebase [3:26pm] rwcr: There are 93 shifts by a variable count in gpxe.lkrn. [3:26pm] rwcr: Sorry, that's left shifts. 19 right shifts. [3:26pm] ogreinside1 joined the chat room. [3:26pm] MMlosh left the chat room. ("rebooting router to ensure proper configuration of readonly and readwrite paths") [3:26pm] mcb30: mdc_mobile: want something to do? ;) [3:27pm] • mdc suspects another "warnings purge" type of project :) [3:27pm] mdc: mcb30: I think we need an http://etherboot.org/wiki/todo page :) [3:27pm] mdc: With karma points for completing tasks therein :) [3:28pm] mcb30: Social solutions to technical problems :) [3:28pm] mdc: mcb30: Yes, since social beings need to do the work, it does often make sense :) [3:28pm] mdc: Ask the Chinese :) [3:29pm] mdc: Anyway, I take it we need to audit the use of << and >> in gPXE? [3:29pm] mcb30: Basically, yes [3:29pm] mcb30: Constant shifts are ok; the compiler already warns about those if they go out-of-range [3:30pm] mcb30: (which I thought was just it being helpful in the same vein as the "unsigned integer >= 0 is always true" warnings) [3:30pm] mdc: Can you make a page http://etherboot.org/wiki/todo/audit-the-shifts and describe the task so I can come back to it? (and perhaps make a link on http://etherboot.org/wiki/todo ? [3:31pm] mcb30: etherboot.org/wiki/todo doesn't exist [3:31pm] mcb30: (yet) [3:31pm] mdc: :) [3:31pm] • mdc likes dropping hints :) [3:31pm] mdc: hee hee [3:31pm] mcb30: That seems like a "project leader" type task to me [3:31pm] mdc: alright [3:32pm] mcb30: :) [3:32pm] mdc: lol [3:33pm] mcb30: I did my wiki-contributing for today already [3:33pm] mcb30: (documenting the SRP boot firmware table) [3:33pm] mdc: I'm making the ToDo page :) </code>
