Skip to content

fix: correct address validation and memcpy usage in CSphUrl::Connect #3804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: correct address validation and memcpy usage in CSphUrl::Connect #3804

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32ffca1
fix: correct address validation and memcpy usage in CSphUrl::Connect
sanikolaev Oct 8, 2025
3077bf9
fix: remove unnecessary memory deallocation in CSphUrl::Connect
sanikolaev Oct 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions mysqlse/snippets_udf.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ int CSphUrl::Connect()
#if MYSQL_VERSION_ID>=50515
struct addrinfo *hp = NULL;
tmp_errno = getaddrinfo ( m_sHost, NULL, NULL, &hp );
if ( !tmp_errno || !hp || !hp->ai_addr )
if ( tmp_errno!=0 || !hp || !hp->ai_addr )
{
bError = true;
if ( hp )
Expand All @@ -413,7 +413,7 @@ int CSphUrl::Connect()
}

#if MYSQL_VERSION_ID>=50515
memcpy ( &sin.sin_addr, hp->ai_addr, Min ( sizeof(sin.sin_addr), (size_t)hp->ai_addrlen ) );
memcpy ( &sin.sin_addr, &( (struct sockaddr_in *)hp->ai_addr )->sin_addr, sizeof(sin.sin_addr) );
Copy link
Collaborator Author

@sanikolaev sanikolaev Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter 1: Destination (&sin.sin_addr)

  • Purpose: The destination where we want to store the IP address
  • Type: struct in_addr * (pointer to the IP address field in the socket address structure)
  • Unchanged: This parameter remains the same in both versions because the destination is correct

Parameter 2: Source (the key difference)

Original (Incorrect) Source: hp->ai_addr

  • Type: struct sockaddr * (generic socket address structure)
  • Problem: This points to the entire socket address structure, not just the IP address
  • Structure Layout: 
    struct sockaddr {
    sa_family_t sa_family;    // Address family (AF_INET, AF_INET6, etc.)
    char sa_data[14];         // Address data (contains IP + port + padding)
}
  • Issue: Copying the entire structure would copy the address family and other metadata, not just the IP address

Fixed Source: &( (struct sockaddr_in *)hp->ai_addr )->sin_addr

  • Type: struct in_addr * (pointer specifically to the IP address field)
  • Breakdown of the expression:
    1. hp->ai_addr - Get the generic socket address pointer
    2. (struct sockaddr_in *)hp->ai_addr - Cast it to IPv4-specific socket address structure
    3. ( ... )->sin_addr - Access the IP address field within that structure
    4. &( ... ) - Get the address of that IP address field
  • Structure Layout: 
    struct sockaddr_in {
    sa_family_t sin_family;   // Address family (AF_INET)
    in_port_t sin_port;       // Port number
    struct in_addr sin_addr;  // IP address ← This is what we want
    char sin_zero[8];         // Padding
}

Parameter 3: Size (the other key difference)

Original (Incorrect) Size: Min ( sizeof(sin.sin_addr), (size_t)hp->ai_addrlen )

  • Logic: Copy the minimum of the destination size or the entire addrinfo structure size
  • Problem: hp->ai_addrlen is the size of the entire sockaddr_in structure (16 bytes), but sin.sin_addr is only 4 bytes
  • Result: Would attempt to copy 4 bytes (the minimum), but from the wrong starting position

Fixed Size: sizeof(sin.sin_addr)

  • Logic: Copy exactly the size of an IP address (4 bytes for IPv4)
  • Correctness: This matches exactly what we're copying - just the IP address field
  • Result: Copies exactly 4 bytes from the correct source location to the correct destination

Why This Matters

Memory Layout Visualization

Original (incorrect):
hp->ai_addr points to: [family|port|IP_ADDR|padding]
                        ^
                        Copying from here (wrong!)

Fixed (correct):
&(...)->sin_addr points to:     [IP_ADDR]
                                 ^
                                 Copying from here (correct!)

Impact of the Bug

  1. Wrong data: The original code would copy the address family and port number instead of the IP address
  2. Connection failure: The socket would have incorrect IP address data, causing connection attempts to fail
  3. Hostname resolution appeared broken: Even though getaddrinfo() successfully resolved the hostname, the wrong data was being used for the connection

Consistency with SphinxSE

This fix makes the UDF's hostname resolution consistent with the working SphinxSE engine implementation in ha_sphinx.cc, which uses the same correct approach:

memcpy ( &sin.sin_addr, &( (struct sockaddr_in *)hp->ai_addr )->sin_addr, sizeof(sin.sin_addr) );

Result

After this fix, the sphinx_snippets() UDF can now properly resolve hostnames from /etc/hosts and DNS, making it consistent with the SphinxSE engine's behavior.

Copy link
Collaborator Author

@sanikolaev sanikolaev Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomatolog does this explanation make sense?

Copy link
Contributor

@tomatolog tomatolog Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klirichek could you check that the last fix is really good now?

The explanation said it is a right fix for the original issue.

freeaddrinfo ( hp );
#else
memcpy ( &sin.sin_addr, hp->h_addr, Min ( sizeof(sin.sin_addr), (size_t)hp->h_length ) );
Expand Down
Loading