| Message ID | 20170104003626.4211-2-judge.packham@gmail.com |
|---|---|
| State | Accepted |
| Commit | f267e40f9679fec02a3166577c78cb290017b7e3 |
| Delegated to: | Tom Rini |
| Headers | show |
Dear Chris Packham, In message <20170104003626.4211-2-judge.packham@gmail.com> you wrote: > Ensure '.' is used to separate octets. If another character is seen > reject the string outright and return 0.0.0.0. What is this good for? The old code was forgiving and would accept 192,168,1,2 as well. Do we need to be so strict here - at the cost of added code size? Also, at least crtitical parts of the network code (NFS, TFTP) do not check the return value of string_to_ip() - so what is the benefit of this change? Best regards, Wolfgang Denk
On Wed, Jan 4, 2017 at 11:09 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Chris Packham, > > In message <20170104003626.4211-2-judge.packham@gmail.com> you wrote: >> Ensure '.' is used to separate octets. If another character is seen >> reject the string outright and return 0.0.0.0. > > What is this good for? > > The old code was forgiving and would accept 192,168,1,2 as well. Technically you can't enter that. The env_flags.c code prevents that from being added to environment variables that have been tagged as ip addresses. These patches are pushing the logic down a bit further. The code handling env_flags_vartype_ipaddr could be updated to use string_to_ip instead. > Do we need to be so strict here - at the cost of added code size? > > Also, at least crtitical parts of the network code (NFS, TFTP) do not > check the return value of string_to_ip() - so what is the benefit of > this change? > The reasoning behind this change is to prepare for parsing ipv6 addresses, which can contain ipv4 format addresses provided they are at the end. e.g. This is a valid ipv6 address "::ffff:192.168.1.1" and so is "::ffff:0192:0168:0001:0001" but the former triggers the ipv4 mapping logic.
Dear Chris, In message <CAFOYHZDK+cOs-1ODjSCBF7OTERTzMgBgBhRC+WSRG4C5iToB+A@mail.gmail.com> you wrote: > > > The old code was forgiving and would accept 192,168,1,2 as well. > > Technically you can't enter that. The env_flags.c code prevents that > from being added to environment variables that have been tagged as ip > addresses. These patches are pushing the logic down a bit further. The > code handling env_flags_vartype_ipaddr could be updated to use > string_to_ip instead. I'm not 100% sure about that. U-Boot environment is a complex thing. For example, what happens if we use "env import" to import variable settings from text or binary formats? Are all these checks present then, too? [Sorry for asking, I really don't know.] > > Also, at least crtitical parts of the network code (NFS, TFTP) do not > > check the return value of string_to_ip() - so what is the benefit of > > this change? > > > > The reasoning behind this change is to prepare for parsing ipv6 > addresses, which can contain ipv4 format addresses provided they are > at the end. > > e.g. This is a valid ipv6 address "::ffff:192.168.1.1" and so is > "::ffff:0192:0168:0001:0001" but the former triggers the ipv4 mapping > logic. Ah, I see. Makes sense. Should we add error handling to TFTP and NFS, then? Best regards, Wolfgang Denk
On Mon, Jan 9, 2017 at 7:45 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Chris, > > In message <CAFOYHZDK+cOs-1ODjSCBF7OTERTzMgBgBhRC+WSRG4C5iToB+A@mail.gmail.com> you wrote: >> >> > The old code was forgiving and would accept 192,168,1,2 as well. >> >> Technically you can't enter that. The env_flags.c code prevents that >> from being added to environment variables that have been tagged as ip >> addresses. These patches are pushing the logic down a bit further. The >> code handling env_flags_vartype_ipaddr could be updated to use >> string_to_ip instead. > > I'm not 100% sure about that. U-Boot environment is a complex thing. > For example, what happens if we use "env import" to import variable > settings from text or binary formats? Are all these checks present > then, too? [Sorry for asking, I really don't know.] > I can say for sure either. What I can say is that some checking already exists so technically this is duplicating functionality. The change to actually replace one with the other would need to consider these cases that you've raise. >> > Also, at least crtitical parts of the network code (NFS, TFTP) do not >> > check the return value of string_to_ip() - so what is the benefit of >> > this change? >> > >> >> The reasoning behind this change is to prepare for parsing ipv6 >> addresses, which can contain ipv4 format addresses provided they are >> at the end. >> >> e.g. This is a valid ipv6 address "::ffff:192.168.1.1" and so is >> "::ffff:0192:0168:0001:0001" but the former triggers the ipv4 mapping >> logic. > > Ah, I see. Makes sense. > > Should we add error handling to TFTP and NFS, then? > I think the "checking" I'm referring to in net_check_prereq() seems to cover the bases for now. If we want to distinguish between invalid, set to 0.0.0.0 and not set then we'd need to start adding additional error handling.
On Wed, Jan 04, 2017 at 01:36:26PM +1300, Chris Packham wrote: > Ensure '.' is used to separate octets. If another character is seen > reject the string outright and return 0.0.0.0. > > Signed-off-by: Chris Packham <judge.packham@gmail.com> Applied to u-boot/master, thanks!
diff --git a/lib/net_utils.c b/lib/net_utils.c index 8f81e7801033..d06be22849fb 100644 --- a/lib/net_utils.c +++ b/lib/net_utils.c @@ -28,6 +28,10 @@ struct in_addr string_to_ip(const char *s) addr.s_addr = 0; return addr; } + if (i != 3 && *e != '.') { + addr.s_addr = 0; + return addr; + } addr.s_addr <<= 8; addr.s_addr |= (val & 0xFF); if (s) {
Ensure '.' is used to separate octets. If another character is seen reject the string outright and return 0.0.0.0. Signed-off-by: Chris Packham <judge.packham@gmail.com> --- Changes in v2: - new END lib/net_utils.c | 4 ++++ 1 file changed, 4 insertions(+)