buglwIP - A Lightweight TCP/IP stack - Bugs: bug #49827, wrong cast to size_t on 16-bit x86...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #49827: wrong cast to size_t on 16-bit x86 architecture

Submitter:  Alexander <pronkin>
Submitted:  Mon 12 Dec 2016 08:10:55 AM UTC
   
 
Category:  Platform ports Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  2.0.1
lwIP version:  2.0.0

Jump to the original submission

Tue 13 Dec 2016 08:22:10 PM UTC, comment #18: 

Simon,

I have nothing about making things compile with strange compilers or warnings enabled. But I think it is far more appropriate to by default define LWIP_CONST_CAST and friends using simple casts (a correct and safe default), and leave it up to the user to override these macros if that results in warnings.

Speaking of warnings, why would a hypothetical compiler on full warning levels complain about casting away const but not about converting pointers to integers?

Ambroz Bizjak <abizjak>
Tue 13 Dec 2016 07:40:26 PM UTC, comment #17: 

Simon/Dirk,

We are on the same page in regards to when to cast away const and ideally it would never be done.  The conditions where  LWIP_CONST_CAST() are applied are the same conditions where I'd explicitly cast away the const.

I guess I haven't worked with a toolchain configuration where  explicitly casting away const caused a warning, but clearly it is a feature of some compilers, possibly because most of the time const is casted away, it's not done carefully as we discussed, but by a sloppy developer.

If I'm understanding correctly, the purpose of LWIP_CONST_CAST is to document that the cast is well thought out and to also avoid the compiling issuing a warning because we might have done something dangerous

Joel Cunningham <jcunningham>
Group Member
Tue 13 Dec 2016 10:43:52 AM UTC, comment #16: 


> I have test another 16Bit compable compiler (DigitalMars)


So same result. As I said, not a DOS (or compiler) issue but a platform issue. Now fixed by the new macro which can be overridden. I'll add an assert to check that the macro behaves correctly.

> And if We start talk about warnings


We do talk about warnings, but please: not off-topic. If you want bugs or warnings fixed, please add a bug tracker entry. Writing this here makes it almost sure it gets forgotten ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 13 Dec 2016 09:06:38 AM UTC, comment #15: 

I have test another 16Bit compable compiler (DigitalMars):

printf("%p\n", &lwip_cyclic_timers[i]);
printf("%p\n", LWIP_CONST_CAST(void*, &lwip_cyclic_timers[i]));

and got the results:
317B:000C
0000:000C

And if We start talk about warnings, there are some places in LwIP (api_msg:1520, dnc.c:1223)
where compiler warns if int is 16 bit long:

if ((int)(res_idx + lwip_htons(ans.len)) > 0xFFFF)
dns.c(1223): Warning! W124: Comparison result always 0

By standart: int Capable of containing at least the [−32767, +32767] range.

Alexander <pronkin>
Tue 13 Dec 2016 07:43:45 AM UTC, comment #14: 

Re Ambroz:
We are using a lot of 3rd party libraries at work, many of them with poor code quality.

Having a library like that, you have three options:
a) live with the warnings and risk missing warnings in your own code because there are so many warnings when compiling
b) globally disabling some warnings (but you lose them in your own code, too)
c) setting up an own set of warning for every library

Because we don't want lwIP users to go through this, we try to make lwIP code as "clean" as possible. It might not be perfect yet, but we're trying to get there.

Re Joel:
It works with GCC's standard warnings, but not if you try to enable as many warnings as you can: as Dirk said, in some places it's OK to cast, in some it's not. Marking the places where you explicitly say "this must be OK, I thought about it" is what we're trying to do here.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 13 Dec 2016 06:02:25 AM UTC, comment #13: 

You correctly described the problems that could happen when you "cast away constness": you may accidentally modify data in read-only memory.
A good interface declares all parameters const that are not modified during the function call.
Then, how do you verify that you do not accidentally pass const data to function that tries to modify it? Turn on gcc -Wconst-cast warning. The LWIP_CONST_CAST macro makes this warning go away by casting via a number. By using this macro, you declare "I know this is dangerous, and I know what I'm doing.
Last, const parameters help the compiler to optimize, which is desirable in a lightweight stack.

Dirk Ziegelmeier <dziegel>
Group administrator
Tue 13 Dec 2016 01:44:50 AM UTC, comment #12: 

Just my two cents here, but most of the C code I work with (aside from LwIP) casts away const on function parameters via an explicit cast, const void to void .  Most of the code isn't trying to modify the memory, but is simply hooking up two interfaces where one doesn't use const.

I've not had any problems with various versions of the ARM compiler, GCC, or MSVC.

Joel Cunningham <jcunningham>
Group Member
Mon 12 Dec 2016 09:35:33 PM UTC, comment #11: 

Simon,

Why do you think there is a problem? Do you get a compiler error or warning?

It is a common misconception that casting "const T " to "T " is invalid. But in reality it is perfectly valid by itself. What is invalid is using the latter to modify a pointed-too object which is read-only (e.g. a string literal or an object declared with const; note it IS valid to modify an object which is not read-only).

I could go quoting standards but I assume I won't change your mind that way :) Here's a StackOverflow answer though it fouces on C++:

http://stackoverflow.com/questions/8570139/is-a-cast-from-pointer-to-const-to-pointer-to-non-const-invalid-c

It is then also obviously valid to cast the "T " to "void ". And it should also be perfectly valid to cast "const T " directly to "void ".

Ambroz Bizjak <abizjak>
Mon 12 Dec 2016 09:20:36 PM UTC, comment #10: 

Reading standards is diffcult ;-)

The cast is not OK. Const is the problem, not types.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 12 Dec 2016 09:14:09 PM UTC, comment #9: 

Hi,

What exactly is the problem with just casting to void*?

sys_timeout(lwip_cyclic_timers[i].interval_ms, cyclic_timer, (void*)&lwip_cyclic_timers[i]);

Here's an extract from ANSI C which says it's OK:

"A pointer to void may be converted to or from a pointer to any
incomplete or object type.  A pointer to any incomplete or object type
may be converted to a pointer to void and back again; the result shall
compare equal to the original pointer."

Ambroz Bizjak <abizjak>
Mon 12 Dec 2016 07:43:30 PM UTC, comment #8: 


> I realize that DOS is dead, but not quite dead.


That's not a DOS issue, I'll have to check our M16C compiler; I bet it's the same problem there. Although I don't think they have ptrdiff_t and this type predefined, you're now forced to think about it yourself.

Using size_t for this was wrong, and maybe using ptrdiff_t is wrong, too. But searching for a good default that works out of the box for most people, there's not much left other than ptrdiff_t?

However, it seems a bit strange to me that ptrdiff_t is 16 bit when pointers are 32 bit effectively. Probably they assume you don't mix pointers from different segments as that wouldn't work anyway... At least it's a programming model I didn't have in mind when writing on lwip code... (but in the end, why do you want an IP stack for DOS, anyway :-)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 12 Dec 2016 07:16:37 PM UTC, comment #7: 

I use OpenWatcom compiler and have next line in headers:
typedef int ptrdiff_t;
(source https://github.com/open-watcom/open-watcom-v2/blob/master/bld/hdr/ptrdifft.sp#L48)

int in my system is 16 bit (intel 80186 clone)
pointer size depends on memory model that I use.
If I use SMALL memory model, so pointer size is 16 bit (only 16bit offset to default data segment) and default LWIP_CONST_CAST works in this case.
But SMALL data model allows me to use only 64Kb of data, because of that I have to use LARGE memory model, where
pointer size is 32 bit (16 bit offset + 16 bit segment), so in this case (my default scenario) I need to override LWIP_CONST_CAST.

I think that if I choose HUGE memory model, so ptrdiff_t will be long:
typedef long ptrdiff_t;
but in this case overhead of pointers usage will appear.

I don't know how to write LWIP_CONST_CAST to be more portable to my case.
I have the same problems with other community projects.

I realize that DOS is dead, but not quite dead.

Alexander <pronkin>
Mon 12 Dec 2016 01:09:18 PM UTC, comment #6: 

Out of curiosity, can you tell us why ptrdiff_t does not work? What type is ptrdiff_t on your system?

Dirk Ziegelmeier <dziegel>
Group administrator
Mon 12 Dec 2016 12:50:39 PM UTC, comment #5: 

I've tested fixes.

now I can override LWIP_CONST_CAST macro to my needs.

but default value:
#define LWIP_CONST_CAST(target_type, val) ((target_type)((ptrdiff_t)val))
results wrong pointer in real mode Application with Large memory model.

It must be remembered when porting.






Alexander <pronkin>
Mon 12 Dec 2016 09:18:05 AM UTC, comment #4: 

Fixed, thanks for reporting

Dirk Ziegelmeier <dziegel>
Group administrator
Mon 12 Dec 2016 09:17:31 AM UTC, comment #3: 

Ok, thanks.
I will follow it in HEAD and check the solution on my configuration(OpenWatcom under DOS) after fix.

Alexander <pronkin>
Mon 12 Dec 2016 08:51:22 AM UTC, comment #2: 

from 'const void *' to 'void*'...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 12 Dec 2016 08:50:22 AM UTC, comment #1: 

This cast is not unnecessary: we need to get from 'void*' to 'const void*'. Your compiler seems to ignore that fact...

However, it's true that just casting via 'size_t' here is not a true protable solution. We'll have to fix that.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 12 Dec 2016 08:10:55 AM UTC, original submission:  

I work on DOS-like 16-bit OS and recently updated lwip from 1.4.1 to 2.0.0. and found that in timouts.c:182 (http://git.savannah.gnu.org/cgit/lwip.git/tree/src/core/timeouts.c#n182)

sys_timeout(lwip_cyclic_timers[i].interval_ms, cyclic_timer, (void*)(size_t)&lwip_cyclic_timers[i]);

unnecessary cast to size_t (before cast to void *) results wrong pointer value in real mode x86 applications.

Alexander <pronkin>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jcunningham (Posted a comment)
  • -email is unavailable- added by abizjak (Posted a comment)
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by pronkin (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-12-12 dziegel StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2016-12-12 goldsimon Planned ReleaseNone 2.0.1
    2016-12-12 goldsimon StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code