This is assuming that the display name will always be packed right after the service name. We can't guarantee that this will always be the case, since it's not documented anywhere. This is an opportunity to make the unmarshall_str function more general and easier to use: change it so that it unmarshalls a null-terminated unicode string from anywhere in the data based on offset. I'll describe better up at unmarshall_str.

Instead of requiring the caller to provide endpos, decoder, and offset, write it as a simpler and easier to use function and move it to msrpctypes.lua:

--- Unmarshalls a null-terminated Unicode string based upon a 32-bit offset (LPTSTR)
-- @param data The data being processed
-- @param pos  The current position within the data
-- @return The new position
-- @return The string with null removed
function unmarshall_lptstr(data, pos)

This function will unmarshall the offset, then start from that offset looking at every 2 bytes for a "\0\0" which is the null terminator. When that is found, extract the string from the offset to the terminator and return it. The returned position will be just after the 4-byte offset; the string itself is not part of that calculation.

Example use:

pos, serviceName = unmarshall_lptstr(arguments, pos)
pos, displayName = unmarshall_lptstr(arguments, pos)
serviceName = unicode.utf16to8(serviceName)
displayName = unicode.utf16to8(displayName)

Remove a few of these debug statements now that it's working. Printing the arguments directly and in hex is particularly unnecessary now. Also, may want to check that result["arguments"] actually exists here; otherwise this will result in a script crash.

Unpack the rest of the return values here, too. Most especially you need to get the return value of the function call, since this could indicate that there's no point in continuing. Here are the possibilities based on error number:

• ERROR_SUCCESS - Probably not going to happen because we sent 0 bytes of buffer, but it could mean there are no services available.
• ERROR_INSUFFICIENT_BUFFER or ERROR_MORE_DATA - This is what we expect: we need to send a bigger buffer and/or there is more data after unpacking the current buffer.
• ERROR_ACCESS_DENIED or anything else - stop processing and return an error; no point in making further calls. This will be the most common state for modern Windows without domain admin credentials.

The max buffer size is defined for WinXP and Server 2003 as 64K according to MSDN. So this should be 0xfa00 instead.

These explanatory lines won't make sense in the NSEdoc layout of the website, and the results are similar on Windows 10. I would leave them out.

state isn't showing up, which is because it's being mixed up with a different enum. The constants in msrpctypes.lua for svcctl_State are for the dwServiceState input parameter, and are unrelated to the actual values defined for dwCurrentState in the SERVICE_STATE struct. I don't see that these are used anywhere else, either, so just change the contents of the svcctl_State table in msrpctypes.lua to match the values in that reference, which ought to fix it.

Should I change the table being passed in here, https://github.com/nmap/nmap/blob/master/nselib/msrpctypes.lua#L4470 ? Or should I create a duplicate of msrpctypes.unmarshall_SERVICE_STATUS function and change this line, https://github.com/nmap/nmap/blob/master/nselib/msrpctypes.lua#L4515 ?

check_point, wait_hint, win32_exit_code, and service_exit_code are not really helpful or interesting items, so in the interest of reducing the size of the output somewhat, let's leave them out. Loop over the return table of svcctl_enumservicesstatusw and create a stdnse.output_table for each one, filling it out with the values we care about. This will ensure that the keys are in the same order for each service, which is important in reading and comparing results.

Here is the recommended format of the result table, with each of the tables being the result of a call to stdnse.output_table() to preserve the ordering of the keys:

  ALG:
    display_name: Application Layer Gateway Service
    state:
      SERVICE_STATE_RUNNING
    type:
      SERVICE_TYPE_WIN32_OWN_PROCESS
    controls_accepted:
      SERVICE_CONTROL_NETBINDADD

Of course I've just truncated the lists for illustration; you would still show all the values for controls, type, and state. Maybe later once we've committed it we can work on better display formats. But this is at least not as large as the current format.

Let's make the default SERVICE_ACTIVE. This will reduce output somewhat and also makes it more interesting to look at, since the set of running services can vary more than the set of available services depending on system configuration. Later, we can consider making this a script-arg.

instead of using string.unpack, you can use string.sub to grab a 2-byte substring. Then just continue calculating the current position and return string.sub(arguments, startpos, curpos). This avoids repeated string building via concatenation, which can be very bad for performance due to repeated reallocation of str.

Your idea of using string.sub degrades the performance because when we know the end position there is no point in iterating all chars finding for NULL byte. I think its better to pass endpos argument to unmarshall_lptstr to improve the performance.

Your views?

With the above changes you suggested I made a PR #991 but for the sake of this script I added a local function in msrpc.lua as optimized_unmarshall_lptstr() which accepts end position of the string as a parameter and this is a bit faster than the function in #991 .

Why is the offset 5? This function should handle reading the offset, extracting the string, and returning the new position and the string. In pseudocode:

function unmarshall_lptstr(args, pos)
  pos, offset = unmarshall_int32(args, pos)
  endpos = offset
  while endpos < #args
    if args:sub(endpos, endpos+2) == "\0\0"
      break
    end
  end
  return pos, args:sub(offset, endpos)

Offset 5 must be added to line 4545 because while receive the response from the server the first 4 bytes are allocated for displaying the size of the packet and so we have to start unmarshalling from the 5th position.

While we extract the starting position of the strings from the binary response, we get the starting position wrt to 1, since we were starting unmarshalling from 5th position, 5 has to be added to the unpack function.

If you think this will be a problem, we can change this by passing a parameter as offset like

function unmarshall_lptstr(arguments, startpos, offset)
offset = offset or 0
......
end

Does this look nice?