Menu ▾ ▴

#277 Minor issue with ipmi_intf_session_set_hostname()

version-1.8.15
closed-fixed
Zdenek Styblik
https://sourceforge.net/p/ipmitool/bugs/313/ (1)
5
2014-10-17
2013-09-30
Holger Liebig
No

It currently clears out only partially the structure element (16 of the 64 characters) and if a hostname of more than 64 characters is specified overwrites the following structure member. And last but not least, an ASCII FQDN as hostname can be up to 253 octets long - see RFC1034 and RFC1035 or http://en.wikipedia.org/wiki/Domain_Name_System

Related

Bugs (use GitHub instead): #277

Discussion

  • Zdenek Styblik

    Zdenek Styblik - 2014-09-20
    • status: open --> pending
    • assigned_to: Zdenek Styblik
    • Group: version-1.8.14 --> version-1.8.15
     

    • Holger Liebig

      Holger Liebig - 2014-09-22

      Hi Zdenek,
      I have to admit that I haven't looked at the actual ipmitool code lately since I use my version from last year in day to day use.
      This all depends on your networking environment. Assume you are working in a primary DNS domain example.com and are also sporadically accessing network devices in acme.c. If you have set your DNS search path to example.com and acme.com and specify 'host42' as hostname, the resolver will try host42.example.com and host42.acme.com for you automatically without the need for specifying the complete FQDN. If you want to access a networking device / BMC in bmc0815.domain.org (which is not in your current domain search path) you have to specify the complete FQDN for the resolver. And this can - by definition - be up to 253octets. A DNS name can be used with ipmitool and will be passed as input to getaddrinfo() in addition to an IPv4 or IPv6 address string and will be passed to the resolver - see ipmi_intf_socket_connect(). If you limit this to 64characters only this will meet most of today's real world scenarios with quite short FQDN's but in a datacenter with many different subnets you might (and someday will) hit the 64 character boundary.
      At least if you replace the hard coded values (16 and 64) with sizeof(intf->session->hostname) you would be flexible for future changes. Or use pointer and strdup.

      void
      ipmi_intf_session_set_hostname(struct ipmi_intf * intf, char * hostname)
      {
      if (intf->session == NULL)
      return;

        memset(intf->session->hostname, 0, sizeof(intf->session->hostname));

  if (hostname != NULL) {
        memcpy(intf->session->hostname, hostname,
               __min(strlen(hostname), sizeof(intf->session->hostname) - 1));
  }

      }

      From: Zdenek Styblik [mailto:stybla@users.sf.net]
      Sent: Saturday, September 20, 2014 10:34 PM
      To: [ipmitool:bugs]
      Subject: [ipmitool:bugs] #277 Minor issue with ipmi_intf_session_set_hostname()

      • status: open --> pending
      • assigned_to: Zdenek Styblik
      • Group: version-1.8.14 --> version-1.8.15
      • Comment:

      Holger, I believe you're wrong:

      [...]
      A domain name consists of one or more parts, technically called labels, that are conventionally concatenated, and delimited by dots, such as example.com.
      [...]
      Each label may contain up to 63 characters. The full domain name may not exceed the length of 253 characters in its textual representation.
      [...]

      Therefore, unless FQDN is expected and supposed to be passed into structure, I believe everything is in order. I admit, it's possible code should do more work than it does now like checking input length or actually getting hostname part, but hostname being limited to 64 chars including '\0' seems fine to me.

      [bugs:#277]http://sourceforge.net/p/ipmitool/bugs/277 Minor issue with ipmi_intf_session_set_hostname()

      Status: pending
      Group: version-1.8.15
      Created: Mon Sep 30, 2013 01:12 PM UTC by Holger Liebig
      Last Updated: Mon Sep 30, 2013 01:12 PM UTC
      Owner: Zdenek Styblik

      It currently clears out only partially the structure element (16 of the 64 characters) and if a hostname of more than 64 characters is specified overwrites the following structure member. And last but not least, an ASCII FQDN as hostname can be up to 253 octets long - see RFC1034 and RFC1035 or http://en.wikipedia.org/wiki/Domain_Name_System

      Sent from sourceforge.net because you indicated interest in https://sourceforge.net/p/ipmitool/bugs/277/https://sourceforge.net/p/ipmitool/bugs/277

      To unsubscribe from further messages, please visit https://sourceforge.net/auth/subscriptions/https://sourceforge.net/auth/subscriptions

       

      Related

      Bugs (use GitHub instead): #277

      • Zdenek Styblik

        Zdenek Styblik - 2014-09-22

        I'm sorry, but what exactly is your point? Just in couple sentences, really. Because I don't follow.

         

        • Holger Liebig

          Holger Liebig - 2014-09-22

          From: Zdenek Styblik [mailto:stybla@users.sf.net]
          Sent: Monday, September 22, 2014 8:14 AM
          To: [ipmitool:bugs]
          Subject: [ipmitool:bugs] Re: #277 Minor issue with ipmi_intf_session_set_hostname()

          I'm sorry, but what exactly is your point? Just in couple sentences, really. Because I don't follow.

          [Liebig, Holger]

          A) ipmitool supports DNS names. A DNS name can be longer than 64 characters when specified as FQDN.

          B) The current code does not clear out the complete structure element, together with the memcpy you can end up with a mixture of old and new DNS name when the new one is exactly 16 characters and the old one was longer.

          C) If you don't want to change it, this is fine with me

          [bugs:#277]http://sourceforge.net/p/ipmitool/bugs/277 Minor issue with ipmi_intf_session_set_hostname()

          Status: pending
          Group: version-1.8.15
          Created: Mon Sep 30, 2013 01:12 PM UTC by Holger Liebig
          Last Updated: Sat Sep 20, 2014 08:34 PM UTC
          Owner: Zdenek Styblik

          It currently clears out only partially the structure element (16 of the 64 characters) and if a hostname of more than 64 characters is specified overwrites the following structure member. And last but not least, an ASCII FQDN as hostname can be up to 253 octets long - see RFC1034 and RFC1035 or http://en.wikipedia.org/wiki/Domain_Name_System

          Sent from sourceforge.net because you indicated interest in https://sourceforge.net/p/ipmitool/bugs/277/https://sourceforge.net/p/ipmitool/bugs/277

          To unsubscribe from further messages, please visit https://sourceforge.net/auth/subscriptions/https://sourceforge.net/auth/subscriptions

           

          Related

          Bugs (use GitHub instead): #277

          alternate

          • Zdenek Styblik

            Zdenek Styblik - 2014-09-22

            A) ipmitool supports DNS names. A DNS name can be longer than 64 characters when specified as FQDN.

            I've never denied this, did I?

            B) The current code does not clear out the complete structure element, together with the memcpy you can end up with a mixture of old and new DNS name when the new one is exactly 16 characters and the old one was longer.

            Indeed given the code above.

            C) If you don't want to change it, this is fine with me

            Again, I've never said anything like that. It's likely this is similar to https://sourceforge.net/p/ipmitool/bugs/313/
            Feel free to post a patch.

            Thanks for the summary. I really did get lost in the long version.

            Z.

             

  • Zdenek Styblik

    Zdenek Styblik - 2014-09-20

    Holger, I believe you're wrong:

    [...]
    A domain name consists of one or more parts, technically called labels, that are conventionally concatenated, and delimited by dots, such as example.com.
    [...]
    Each label may contain up to 63 characters. The full domain name may not exceed the length of 253 characters in its textual representation.
    [...]

    Therefore, unless FQDN is expected and supposed to be passed into structure, I believe everything is in order. I admit, it's possible code should do more work than it does now like checking input length or actually getting hostname part, but hostname being limited to 64 chars including '\0' seems fine to me.

     

  • Zdenek Styblik

    Zdenek Styblik - 2014-09-22
    • status: pending --> open
     

  • Holger Liebig

    Holger Liebig - 2014-09-22

    Here is a small patch proposal.
    Thanks,
    Holger

     

    • Zdenek Styblik

      Zdenek Styblik - 2014-09-22

      Do you reckon malloc() would be too much trouble? That's why I've rejected work-around in bug#313 in first evaluation.
      Only concern I have about malloc() or strdup() is not to forget to free() afterwards. It also requires addition of hostname_len into struct in question.

       

      • Holger Liebig

        Holger Liebig - 2014-09-22

        Sure, this could be possible. Currently there seems no session_cleanup() and it would require additional changes in all places where free(intf->session) is currently called. The length could always be determined dynamically with strlen(). Let me know if you want me to come up with another patch proposal.

        From: Zdenek Styblik [mailto:stybla@users.sf.net]
        Sent: Monday, September 22, 2014 10:42 AM
        To: [ipmitool:bugs]
        Subject: [ipmitool:bugs] Re: #277 Minor issue with ipmi_intf_session_set_hostname()

        Do you reckon malloc() would be too much trouble? That's why I've rejected work-around in bug#313 in first evaluation.
        Only concern I have about malloc() or strdup() is not to forget to free() afterwards. It also requires addition of hostname_len into struct in question.

        [bugs:#277]http://sourceforge.net/p/ipmitool/bugs/277 Minor issue with ipmi_intf_session_set_hostname()

        Status: open
        Group: version-1.8.15
        Created: Mon Sep 30, 2013 01:12 PM UTC by Holger Liebig
        Last Updated: Mon Sep 22, 2014 08:14 AM UTC
        Owner: Zdenek Styblik

        It currently clears out only partially the structure element (16 of the 64 characters) and if a hostname of more than 64 characters is specified overwrites the following structure member. And last but not least, an ASCII FQDN as hostname can be up to 253 octets long - see RFC1034 and RFC1035 or http://en.wikipedia.org/wiki/Domain_Name_System

        Sent from sourceforge.net because you indicated interest in https://sourceforge.net/p/ipmitool/bugs/277/https://sourceforge.net/p/ipmitool/bugs/277

        To unsubscribe from further messages, please visit https://sourceforge.net/auth/subscriptions/https://sourceforge.net/auth/subscriptions

         

        Related

        Bugs (use GitHub instead): #277

        alternate

        • Zdenek Styblik

          Zdenek Styblik - 2014-09-24

          The length could always be determined dynamically with strlen().

          Indeed, I wanted to verify this/wasn't sure about and I did.

          I really would like to go following way rather than static allocation:
          1] probably check length of the input
          2] strdup()/strndup()
          3] clean up

          Even at cost of "more work" rather than static allocation. If you want to take a shot at it, please, do so. I might have time to give this a look over the weekend, I might not.

           

  • Holger Liebig

    Holger Liebig - 2014-09-26

    Alternate patch proposal which strdups/allocates the hostname instead of fixed size in structure. Also introduces ipmi_intf_session_cleanup().

     
    hostname.patch

    • Zdenek Styblik

      Zdenek Styblik - 2014-10-01

      Patch looks good with the exception of formatting. However, I'm probably missing something, but why can't we call ipmi_intf_session_cleanup() from ipmi_cleanup()?

       

      • Zdenek Styblik

        Zdenek Styblik - 2014-10-01

        I'm also amazed by the fact we're strdup()-ing stuff around :) Just a comment not related to the patch itself.

         

      • Holger Liebig

        Holger Liebig - 2014-10-02

        From: Zdenek Styblik [mailto:stybla@users.sf.net]
        Sent: Wednesday, October 01, 2014 6:09 PM
        To: [ipmitool:bugs]
        Subject: [ipmitool:bugs] Re: #277 Minor issue with ipmi_intf_session_set_hostname()

        Patch looks good with the exception of formatting. However, I'm probably missing something, but why can't we call ipmi_intf_session_cleanup() from ipmi_cleanup()?

        [Liebig, Holger] We most likely could if we move the ipmi_cleanup() call after the ipmi_main_intf->close() call. But to be on the save side (I do not have all the required setup e.g. to test for serial sessions) I simply replaced all the previous calls where the session structure was free'd. There could be more stuff placed into a generic cleanup/close call (e.g. close(intf->fd) which is currently also handled on a per interface base.

        [bugs:#277]http://sourceforge.net/p/ipmitool/bugs/277 Minor issue with ipmi_intf_session_set_hostname()

        Status: open
        Group: version-1.8.15
        Created: Mon Sep 30, 2013 01:12 PM UTC by Holger Liebig
        Last Updated: Fri Sep 26, 2014 05:49 AM UTC
        Owner: Zdenek Styblik

        It currently clears out only partially the structure element (16 of the 64 characters) and if a hostname of more than 64 characters is specified overwrites the following structure member. And last but not least, an ASCII FQDN as hostname can be up to 253 octets long - see RFC1034 and RFC1035 or http://en.wikipedia.org/wiki/Domain_Name_System

        Sent from sourceforge.net because you indicated interest in https://sourceforge.net/p/ipmitool/bugs/277/https://sourceforge.net/p/ipmitool/bugs/277

        To unsubscribe from further messages, please visit https://sourceforge.net/auth/subscriptions/https://sourceforge.net/auth/subscriptions

         

        Related

        Bugs (use GitHub instead): #277

        alternate

        • Zdenek Styblik

          Zdenek Styblik - 2014-10-02

          Sure. I was just being curious and it somewhat made and still makes sense rather than code duplication. I believe all of those are copies of one or another.

          Anyway, who's going to clean up formatting in the patch? I guess I could do it.

           

          • Holger Liebig

            Holger Liebig - 2014-10-02

            From: Zdenek Styblik [mailto:stybla@users.sf.net]
            Sent: Thursday, October 02, 2014 11:26 AM
            To: [ipmitool:bugs]
            Subject: [ipmitool:bugs] Re: #277 Minor issue with ipmi_intf_session_set_hostname()

            Sure. I was just being curious and it somewhat made and still makes sense rather than code duplication. I believe all of those are copies of one or another.

            Anyway, who's going to clean up formatting in the patch? I guess I could do it.

            [Liebig, Holger] Yes, please go ahead. Formatting looks good on my side though (with visible tabs and spaces), don't know what is causing this.

            Thanks,
            Holger

            [bugs:#277]http://sourceforge.net/p/ipmitool/bugs/277 Minor issue with ipmi_intf_session_set_hostname()

            Status: open
            Group: version-1.8.15
            Created: Mon Sep 30, 2013 01:12 PM UTC by Holger Liebig
            Last Updated: Fri Sep 26, 2014 05:49 AM UTC
            Owner: Zdenek Styblik

            It currently clears out only partially the structure element (16 of the 64 characters) and if a hostname of more than 64 characters is specified overwrites the following structure member. And last but not least, an ASCII FQDN as hostname can be up to 253 octets long - see RFC1034 and RFC1035 or http://en.wikipedia.org/wiki/Domain_Name_System

            Sent from sourceforge.net because you indicated interest in https://sourceforge.net/p/ipmitool/bugs/277/https://sourceforge.net/p/ipmitool/bugs/277

            To unsubscribe from further messages, please visit https://sourceforge.net/auth/subscriptions/https://sourceforge.net/auth/subscriptions

             

            Related

            Bugs (use GitHub instead): #277

            alternate

  • Zdenek Styblik

    Zdenek Styblik - 2014-10-17
     

Log in to post a comment.

MongoDB Logo MongoDB
Gen AI apps are built with MongoDB Atlas
Atlas offers built-in vector search and global availability across 125+ regions. Start building AI apps faster, all in one place.
Try Free →