| |||||||||||||
trying to work out some problem pointstrying to work out some problem pointsPosted Dec 7, 2011 11:46 UTC (Wed) by johill (subscriber, #25196)In reply to: trying to work out some problem points by khim Parent article: Fedora security update to nginx I'm quite fluent in C, but still the code is puzzling. You're confusing "fluent in C" and "fluent in DNS" and assuming that everybody who reads that code must understand the DNS protocol with all its bits in its entirety, and completely know what each bit means. Take an example from a different domain, and consider this code I'm writing just now:
switch (mgmt->u.action.category) {
case WLAN_CATEGORY_HT:
...
/* verify action & smps_control are present */
if (len < IEEE80211_MIN_ACTION_SIZE + 2)
goto invalid;
switch (mgmt->u.action.u.ht_smps.action) {
case WLAN_HT_ACTION_SMPS: {
It seems like you're suggesting it is equivalent to:
switch (*(frame + 24)) {
case 7:
...
if (len < 27)
goto invalid;
switch (*(frame + 25)) {
case 1: {
which is true on a binary/compiler level (there's a good chance it would compile to the same binary unless I made some small errors in my manual replacement), but not for people.
(Log in to post comments)
When I said "fluent in C", I meant "fluent in C", sorry... Posted Dec 7, 2011 13:20 UTC (Wed) by khim (subscriber, #9252) [Link] You're confusing "fluent in C" and "fluent in DNS" and assuming that everybody who reads that code must understand the DNS protocol with all its bits in its entirety, and completely know what each bit means. I assume nothing of the sort. I knew very little about DNS protocol. In fact I've read C code first (it was quite simple, really), got confused by one small thing (why do we ignore top two bits totally?), and only then opened the RFC to see what exact structure we are parsing here. It seems like you're suggesting it is equivalent to: The code you've showed includes "random numbers" which have no internal logic. It's easy to imagine different RFC where IEEE80211_MIN_ACTION_SIZE will be 15 or 30 and where WLAN_CATEGORY_HT will be equal to 10. Worse: it's hard to imagine that you implement your logic using the textual description which refer to "24th byte" or "7th category"! But the code which started the discussion is radially different. It implements process which description uses terms like the first two bits of the octet (which is quite literally "c & 0xc0" in C) and the remaining six bits (which, again, is "c & 0x3f" in C). If, indeed, the canonical description in your case includes 7th category and not the "WLAN HT category" (or something like that) then yes, I think it's better to keep "magical numbers" in your code: it'll be easier to understand then try to constantly convert named constants to "magic numbers" and back ("gcc -E" can be a big help here as I've noted). Except 27. I think "25 + 2" will be more readable even in this case...
|
|||||||||||||