Ondrej, this looks buggy: +static inline void htonlsab(void *h, void *n, u16 len) { memcpy(n, h, len); }; +static inline void ntohlsab(void *n, void *h, u16 len) { memcpy(h, n, len); }; memcpy is not defined to handle overlapping memory. Best to add: if (n != h) memcpy(...) Might as well do that for these too: +static inline void htonlsah(struct ospf_lsa_header *h, struct ospf_lsa_header *n) { *n = *h; }; +static inline void ntohlsah(struct ospf_lsa_header *n, struct ospf_lsa_header *h) { *h = *n; };
On Thu, Apr 29, 2010 at 11:03:32PM +0200, Joakim Tjernlund wrote:
Ondrej, this looks buggy:
+static inline void htonlsab(void *h, void *n, u16 len) { memcpy(n, h, len); }; +static inline void ntohlsab(void *n, void *h, u16 len) { memcpy(h, n, len); };
memcpy is not defined to handle overlapping memory. Best to add: if (n != h) memcpy(...)
Yes, but all usages of htonlsab()/ntohlsab() are non-overlapping. (overlapping/on place usage was replaced with htonlsab1()/ntohlsab1()).
Might as well do that for these too: +static inline void htonlsah(struct ospf_lsa_header *h, struct ospf_lsa_header *n) { *n = *h; }; +static inline void ntohlsah(struct ospf_lsa_header *n, struct ospf_lsa_header *h) { *h = *n; };
*n = *h should be OK even if n == h, AFAIK. -- Elen sila lumenn' omentielvo Ondrej 'SanTiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Ondrej Zajicek <santiago@crfreenet.org> wrote on 2010/04/29 23:15:22:
On Thu, Apr 29, 2010 at 11:03:32PM +0200, Joakim Tjernlund wrote:
Ondrej, this looks buggy:
+static inline void htonlsab(void *h, void *n, u16 len) { memcpy(n, h, len); }; +static inline void ntohlsab(void *n, void *h, u16 len) { memcpy(h, n, len); };
memcpy is not defined to handle overlapping memory. Best to add: if (n != h) memcpy(...)
Yes, but all usages of htonlsab()/ntohlsab() are non-overlapping. (overlapping/on place usage was replaced with htonlsab1()/ntohlsab1()).
I see, but it would be safer to have that check in case you used them in the wrong way?
Might as well do that for these too: +static inline void htonlsah(struct ospf_lsa_header *h, struct ospf_lsa_header *n) { *n = *h; }; +static inline void ntohlsah(struct ospf_lsa_header *n, struct ospf_lsa_header *h) { *h = *n; };
*n = *h should be OK even if n == h, AFAIK.
Hopyfully, but isn't gcc allowed to use memcpy to do that assignment? If you add that if (h!=n) test, doesn't the need for Xlasb1() go away as well? Jocke
Ondrej Zajicek <santiago@crfreenet.org> wrote on 2010/04/29 23:15:22:
On Thu, Apr 29, 2010 at 11:03:32PM +0200, Joakim Tjernlund wrote:
Ondrej, this looks buggy:
+static inline void htonlsab(void *h, void *n, u16 len) { memcpy(n, h, len); }; +static inline void ntohlsab(void *n, void *h, u16 len) { memcpy(h, n, len); };
memcpy is not defined to handle overlapping memory. Best to add: if (n != h) memcpy(...)
Yes, but all usages of htonlsab()/ntohlsab() are non-overlapping. (overlapping/on place usage was replaced with htonlsab1()/ntohlsab1()).
I see, but it would be safer to have that check in case you used them in the wrong way?
Might as well do that for these too: +static inline void htonlsah(struct ospf_lsa_header *h, struct ospf_lsa_header *n) { *n = *h; }; +static inline void ntohlsah(struct ospf_lsa_header *n, struct ospf_lsa_header *h) { *h = *n; };
*n = *h should be OK even if n == h, AFAIK.
Hopyfully, but isn't gcc allowed to use memcpy to do that assignment? If you add that if (h!=n) test, doesn't the need for Xlasb1() go away as well?
..and use memmove() instead. That way h == n becomes a NOP and h!=n can be overlapping as memmove can do that.
On Thu, Apr 29, 2010 at 11:57:32PM +0200, Joakim Tjernlund wrote:
Ondrej Zajicek <santiago@crfreenet.org> wrote on 2010/04/29 23:15:22:
On Thu, Apr 29, 2010 at 11:03:32PM +0200, Joakim Tjernlund wrote:
Ondrej, this looks buggy:
+static inline void htonlsab(void *h, void *n, u16 len) { memcpy(n, h, len); }; +static inline void ntohlsab(void *n, void *h, u16 len) { memcpy(h, n, len); };
memcpy is not defined to handle overlapping memory. Best to add: if (n != h) memcpy(...)
Yes, but all usages of htonlsab()/ntohlsab() are non-overlapping. (overlapping/on place usage was replaced with htonlsab1()/ntohlsab1()).
I see, but it would be safer to have that check in case you used them in the wrong way?
I think this is no issue. Even if someone used them in a wrong way (which is improbable, as these functions are used just in a few places), memcpy() would probably do nothing when src==dst (although it is unspecified).
Might as well do that for these too: +static inline void htonlsah(struct ospf_lsa_header *h, struct ospf_lsa_header *n) { *n = *h; }; +static inline void ntohlsah(struct ospf_lsa_header *n, struct ospf_lsa_header *h) { *h = *n; };
*n = *h should be OK even if n == h, AFAIK.
Hopyfully, but isn't gcc allowed to use memcpy to do that assignment?
Definitely isn't allowed to do something that breaks the specified behavior of C operators.
If you add that if (h!=n) test, doesn't the need for Xlasb1() go away as well?
I think it is cleaner to have Xlsab1() for that purpose. -- Elen sila lumenn' omentielvo Ondrej 'SanTiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Ondrej Zajicek <santiago@crfreenet.org> wrote on 2010/04/30 10:19:15:
On Thu, Apr 29, 2010 at 11:57:32PM +0200, Joakim Tjernlund wrote:
Ondrej Zajicek <santiago@crfreenet.org> wrote on 2010/04/29 23:15:22:
On Thu, Apr 29, 2010 at 11:03:32PM +0200, Joakim Tjernlund wrote:
Ondrej, this looks buggy:
+static inline void htonlsab(void *h, void *n, u16 len) { memcpy(n, h, len); }; +static inline void ntohlsab(void *n, void *h, u16 len) { memcpy(h, n, len); };
memcpy is not defined to handle overlapping memory. Best to add: if (n != h) memcpy(...)
Yes, but all usages of htonlsab()/ntohlsab() are non-overlapping. (overlapping/on place usage was replaced with htonlsab1()/ntohlsab1()).
I see, but it would be safer to have that check in case you used them in the wrong way?
I think this is no issue. Even if someone used them in a wrong way (which is improbable, as these functions are used just in a few places), memcpy() would probably do nothing when src==dst (although it is unspecified).
Yes, it is unlikely but if that were to happen you will have a very hard time finding the problem as the real cause would not be visible and only on some platform/gcc version.
Might as well do that for these too: +static inline void htonlsah(struct ospf_lsa_header *h, struct ospf_lsa_header *n) { *n = *h; }; +static inline void ntohlsah(struct ospf_lsa_header *n, struct ospf_lsa_header *h) { *h = *n; };
*n = *h should be OK even if n == h, AFAIK.
Hopyfully, but isn't gcc allowed to use memcpy to do that assignment?
Definitely isn't allowed to do something that breaks the specified behavior of C operators.
Sure, but I am not sure if pointer references are covered fully by C.
If you add that if (h!=n) test, doesn't the need for Xlasb1() go away as well?
I think it is cleaner to have Xlsab1() for that purpose.
Jocke
Hello!
Yes, it is unlikely but if that were to happen you will have a very hard time finding the problem as the real cause would not be visible and only on some platform/gcc version.
... or add an ASSERT.
Sure, but I am not sure if pointer references are covered fully by C.
This case is pretty clear. Have a nice fortnight -- Martin `MJ' Mares <mj@ucw.cz> http://mj.ucw.cz/ Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth American patent law: two monkeys, fourteen days.
Martin Mares <mj@ucw.cz> wrote on 2010/04/30 14:51:41:
Hello!
Yes, it is unlikely but if that were to happen you will have a very hard time finding the problem as the real cause would not be visible and only on some platform/gcc version.
... or add an ASSERT.
hmm, why bomb out when not needed?
Hello!
hmm, why bomb out when not needed?
It does not matter. It must not occur in production versions either, it should exist only as a warning for developers during testing. Have a nice fortnight -- Martin `MJ' Mares <mj@ucw.cz> http://mj.ucw.cz/ Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth "Oh no, not again!" -- The bowl of petunias
Martin Mares <mj@ucw.cz> wrote on 2010/04/30 15:25:20:
From: Martin Mares <mj@ucw.cz> To: Joakim Tjernlund <joakim.tjernlund@transmode.se> Cc: bird-users@trubka.network.cz, Ondrej Zajicek <santiago@crfreenet.org> Date: 2010/04/30 15:25 Subject: Re: Xtonlsab bug
Hello!
hmm, why bomb out when not needed?
It does not matter. It must not occur in production versions either, it should exist only as a warning for developers during testing.
FYI: struct my_struct { long a100[100]; }; static inline func(struct my_struct *h1, struct my_struct *h2) { *h1 = *h2; } mytest(struct my_struct *my_h1) { func(my_h1, my_h1); } uses memcpy on ppc: mytest: mflr 0 stwu 1,-16(1) mr 4,3 li 5,400 stw 0,20(1) bl memcpy lwz 0,20(1) addi 1,1,16 mtlr 0 blr Pehaps this it is always safe to use memcpy in this case, but the man page doesn't. It is also a waste of cycles as the whole op is a NOP. Jocke
Hello!
Pehaps this it is always safe to use memcpy in this case, but the man page doesn't.
Neither it is allowed by the C standard.
It is also a waste of cycles as the whole op is a NOP.
Sure -- that's why we have split the copying and in-place conversion. Have a nice fortnight -- Martin `MJ' Mares <mj@ucw.cz> http://mj.ucw.cz/ Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth How do I type 'for i in *.dvi ; do xdvi $i ; done' in a GUI?
Martin Mares <mj@ucw.cz> wrote on 2010/04/30 16:39:00:
Hello!
Pehaps this it is always safe to use memcpy in this case, but the man page doesn't.
Neither it is allowed by the C standard.
It is also a waste of cycles as the whole op is a NOP.
Sure -- that's why we have split the copying and in-place conversion.
I am just suggesting to use a bit of defensive programming to avoid potential future bugs but if you don't want too, OK by me. Jocke
Ondrej Zajicek <santiago@crfreenet.org> wrote on 2010/04/30 10:19:15:
On Thu, Apr 29, 2010 at 11:57:32PM +0200, Joakim Tjernlund wrote:
Ondrej Zajicek <santiago@crfreenet.org> wrote on 2010/04/29 23:15:22:
On Thu, Apr 29, 2010 at 11:03:32PM +0200, Joakim Tjernlund wrote:
Ondrej, this looks buggy:
+static inline void htonlsab(void *h, void *n, u16 len) { memcpy(n, h, len); }; +static inline void ntohlsab(void *n, void *h, u16 len) { memcpy(h, n, len); };
memcpy is not defined to handle overlapping memory. Best to add: if (n != h) memcpy(...)
Yes, but all usages of htonlsab()/ntohlsab() are non-overlapping. (overlapping/on place usage was replaced with htonlsab1()/ntohlsab1()).
I see, but it would be safer to have that check in case you used them in the wrong way?
I think this is no issue. Even if someone used them in a wrong way (which is improbable, as these functions are used just in a few places), memcpy() would probably do nothing when src==dst (although it is unspecified).
gcc mail list does not agree, memcpy(p, p, len) is a bug and you cannot rely on it. Jocke
participants (3)
-
Joakim Tjernlund -
Martin Mares -
Ondrej Zajicek