Handling strings containing \0

[austinEng]

Currently, all const char* members and arguments are null-terminated strings.

This doesn't work well for strings that contain the \0 character which the WebGPU JavaScript API allows. The C API should have a way to express such strings.

One suggestion for doing this would be to add an extra member/argument for every string that is an explicit length.

It could be optional, such that zero means strlen and non-zero is an explicit length. However, this would make it impossible to express non-null strings that are zero-length. Perhaps that's ok? If not, the alternative can be to use SIZE_MAX to mean "use strlen".

[Kangz]

@cwfitzgerald @teoxoy WDYT?

IMHO having zero mean strlen works, and if someone really wants to represent the empty string with an explicit length faithfully, then they can change the pointer to point to "\0".

[cwfitzgerald]

I'm obviously a little biased, but I don't really see the point of zero-deliminated strings when we can provide size, and as zero is a perfectly valid size (i.e. do not dereference this pointer), u32::MAX seems like the most reasonable value of "this is zero-deliminated, figure it out yourself".

[Kangz]

For C, and this is a C header, 0 initialisation is very convenient whereas typing a WGPU_STRLEN_LENGTH all the time would be very annoying. (it would be SIZE_T_MAX). And strings tend to be null-terminated ones. But this is also a little bit biased :) On a scale of 1 to 10 I think I care at around 3. How about you? (basically it would be a wart but somewhat manageable for developers)

[cwfitzgerald]

As an implementer, about 1 (it will go in a string parsing function which can do whatever) - as a user I have no authority to speak :)

[austinEng]

I also don't care so much. I've realized though, we do have #define WGPU_WHOLE_MAP_SIZE SIZE_MAX. perhaps we should also use SIZE_MAX to mean strlen here for consistency? All of the "undefined" constants are also the largest representable integer:

#define WGPU_ARRAY_LAYER_COUNT_UNDEFINED (0xffffffffUL)
#define WGPU_COPY_STRIDE_UNDEFINED (0xffffffffUL)
#define WGPU_LIMIT_U32_UNDEFINED (0xffffffffUL)
#define WGPU_LIMIT_U64_UNDEFINED (0xffffffffffffffffULL)
#define WGPU_MIP_LEVEL_COUNT_UNDEFINED (0xffffffffUL)

[Kangz]

Ok let's go with SIZE_MAX then.

[greggman]

I was looking into writing this and I'm wondering if we can just get away with not changing this header.

If I understand correctly there are 3 places in the API that take strings

1. labels
2. WGSL code in a shader descriptor
3. entryPoint in a pipeline descriptor

For labels the spec says what happens with them is undefined. That means if the user passes in foo\0bar it's valid for the backend to treat that as just foo (or ignore it or do anything it wants, sha5 it, :P)

For 2 and 3, WGSL says \0 is not allowed

That means passing a \0 string in case 2 is an error and because of that, passing a \0 string to 3 is always guaranteed to be an error as well.

It feels like 2 and 3 could (should?) be handled at a higher level.

Again, if I understand correctly, the biggest issue is that errors are reported asynchronously so in JavaScript, if the user passes a WGSL string with \0 or an pipeline module entryPoint with \0 then the browser implementation could solve this by synthesizing an async error.

As a hacky solution for 2, Chrome/Dawn/Tint, Chrome could scan the string for \0, if found then Chrome internally calls pushErrorScope, then compiles a tiny wGSL shader it knows will fail, then it does popErrorScope, and then inserts a synthetic validation error "\0 is invalid WGSL". This means it returns an invalid GPUShaderModule as the spec requires.

I'm guessing it could do something similar for 3. It checks if \0 is in any entryPoint. If found it does pushErrorScope, generates a simple pipeline it knows will fail, popErrorScope, adds the synthetic error "entryPoint can't contain '\0'" and then returns the invalid GPUXXXPipeline

It sounds a little hacky but it sounds better than adding a string size everywhere what has no purpose since \0 in 2 and 3 above are errors and in 1 it's irrelevant.

[greggman]

Let me also suggest, maybe the JavaScript API should just throw for cases 2 and 3

I know WebGPU generally reports errors asynchronously but not all errors are async. Example

device.createShaderModule();  // throws

throws TypeError: Failed to execute 'createShaderModule' on 'GPUDevice': The provided value is not of type 'GPUShaderModuleDescriptor'

device.createShaderModule("");

throws TypeError: Failed to execute 'createComputePipeline' on 'GPUDevice': 1 argument required, but only 0 present

I know both of these happen at a JavaScript level but would it be so bad if WebGPU throw for cases 2 and 3 above? It's arguably the simplest solution and it's unlike anyone is going to run into it (no one is going to pass strings with \0 on purpose as they're invalid anyway). It seems like even the hacky workaround above is overkill just for this tiny edge case of an error.

[Kangz]

This has been discussed at least twice in https://github.com/gpuweb/gpuweb/issues/1816 and https://github.com/gpuweb/gpuweb/issues/3576 and in both cases the WebGPU group decided to not do the change. Not sure if we want to revisit again.

[jzm-intel]

In the very first comment:

This doesn't work well for strings that contain the \0 character which the WebGPU JavaScript API allows. The C API should have a way to express such strings.

I think after the discussion in WebGPU WG, JavaScript API CAN accept a string contains \0, but almost always result in validation error (if not JS type error). An exception might be for label, but we can always trace the JS label within JS level and not touching C API.

So I agree with Greggman's comment https://github.com/gpuweb/gpuweb/issues/3576#issuecomment-1414101440. Making C API accepting string containing \0 seems unnecessary and troubling in my currently opinion.

[greggman]

ok, well, IIUC, the "WebGPU" committee only cares about the "web" api. What happens in webgpu-native is not their responsibility. In the WebGPU API you can pass \0. But, it's possible to implement the web api (WebGPU) on top of webgpu.h without changing webgpu.h

It's strictly worse to change the C API from one where you can't pass \0 to one where you can but you get an error if you do.

So, the suggestion is to leave the C API as is, leave the WebGPU spec as is. And browsers can work around the difference in their integration

The list

• no work needed

  • label in GPUObjectBase - no work needed in JS as what happens to labels is undefined
  • groupLabel for pushDebugGroup and markerLabel for insertDebugMarker in GPUDebugCommandsMixin. No work needed here as again what happens to these markers is undefined
  • requiredLimits and requiredFeatures in GPUDeviceDescriptor - No work needed. \0 is already rejected as unknown enum/feature
  • parameter unmaskHints of requestAdapterInfo - same as previous

• workaround needed (see below)

  • code and hints in GPUShaderModuleDescriptor
  • entryPoint and constants in GPUProgrammableStage DOMString

The workaround is to effectively wrap the calls to createShaderModel and createXXXPipelineXXX like this

// PSEUDO CODE!!!

createShaderModule(descriptor) {
  if (descriptor.code.includes('\0')) {

     wgpuDevicePushErrorScope(device, "validation");

     // generate an invalid shader module
     invalidModule = wgpuDeviceCreateShaderModule(device, descriptorThatWillFailValidation)

     // capture the error from the invalid shader module and synthesize a webgpu error
     wgpuDevicePopErrorScope(device, [](...) {
        synthesizeWebGPUError("invalid WGSL, \\0 is not allow");
     });

     return invalidModule;

  } else {
     return wgpuCreateShaderModule(device, descriptor);
  }
}

The same pattern can be used for the 4 createXXXPipeline??? functions.

[kainino0x]

This all makes sense to me.

It's probably a bit more straightforward overall to add a special way to create an invalid shader module with a given error:

// PSEUDO CODE!!!

createShaderModule(descriptor) {
  if (descriptor.code.includes('\0')) {
     return dawnCreateInvalidShaderModuleWithError(device, "invalid WGSL, \\0 is not allowed");
  } else {
     return wgpuCreateShaderModule(device, descriptor);
  }
}

(https://github.com/gpuweb/gpuweb/issues/3576#issuecomment-1413101815) For all the entryPoint and constants in createXXXPipeline it should also be possible to just replace NUL with some other invalid character(s) like \ 0 or �, AFAIK this wouldn't change validation results.

[greggman]

I'm not against replacing \0 with � given the spec does not require specific errors. It would certainly be easier than the wrap.

On the otherhand, it would be wierd to get an error "invalid character '�' in WGSL at line 50,column 12" and then search for � and not find it beacuse it's something else. But, I'd be fine with it because it should be so rare that it doesn't matter.

Same for entry\0Point saying "no entryPoint called 'entry�Point`, but again, it should never come up so I think I'd choose this solution over the synthetic error.

[kainino0x]

On the otherhand, it would be wierd to get an error "invalid character '�' in WGSL at line 50,column 12" and then search for � and not find it beacuse it's something else. But, I'd be fine with it because it should be so rare that it doesn't matter.

This is basically why one of my suggestions was the string \ 0, but that's also kind of weird in its own way, and agreed it doesn't matter.

[kainino0x]

OK, so we've revisited this finally in a webgpu.h meeting, and the language bindings issue¹ (which I know we've discussed before) won out, and we want to change all of the strings to take an explicit size, using a size+pointer struct that we use in place of all of the strings.

The length can also be set to a sentinel value that says to use strlen (say, WGPU_STRLEN = SIZE_MAX), even though it's a little unergonomic in C. Thinking on this further, we can make strlen the default value in the INIT struct, that way you don't have to touch it when using C directly. The various combinations:

• {WGPU_STRLEN, null pointer} = nil / JS's undefined, for anywhere that's valid, e.g.:
  • GPUObjectDescriptorBase.label, where it means "default to empty string"
  • EDIT: WGPUProgrammableStageDescriptor.entryPoint, where it means "pick the default entry point"
  • (Though note neither of these cases actually needs to differentiate nil from empty string)
• {WGPU_STRLEN, non-null pointer} = use null-terminated string
• {0, any} = empty string
• {>0, null pointer} = crash (null dereference)
• {>0, non-null pointer} = use explicit-length string

¹ The issue is that when binding the C API into other languages, you'll have to get that language's string type into a null-terminated string, which may require making a copy of the string, e.g. C++ with string_view, or Rust, Go, or most other newer languages. This is a significant enough issue that I think it warrants the work needed to update a lot of existing code.

EDIT: example code - why this encoding works well for language bindings, using C API from higher level languages, etc.:

std::string str = ...; // could be empty or not
wgpuBufferSetLabel(buffer, {
  str.size(), // could be 0 or not, so 0 can't mean "strlen"
  str.data(), // never null
});

[austinEng]

I started looking at doing this in Dawn. One interesting thing is that if we do this for all strings, it'll look like you can pass a std::string_view / WGPUString for labels, BUT once this gets down to the backend, it's going to end up getting truncated if there are any \0 in it. The debug APIs for PIX / Vulkan expect null-terminated strings and don't have a length field. And the implementation will also need to append a \0 if the string isn't yet null-terminated.

When Dawn reports object labels itself from the frontend, strings with \0 will be preserved though.

[kainino0x]

That seems fine. Implementations could even replace the nulls with placeholder characters before passing it to the driver (we have to make a copy anyway to add a null terminator)

[austinEng]

Another thing - we lose WGPU_NULLABLE annotations (or rather the lack of WGPU_NULLABLE)

nullptr becomes always valid since {0, any} = empty string. Thus the data ptr in WGPUString is always nullable, like:

typedef struct WGPUString {
    WGPU_NULLABLE const char* data;
    size_t length;
} WGPUString WGPU_STRUCTURE_ATTRIBUTE;

[kainino0x]

Two details we should discuss:

• Do we use this struct for output strings, or only input strings? I believe the bindings argument only really applies to input strings (for output strings you can use strlen() to get the length, and don't need to reallocate - but you do have to run strlen()).
• Can we guarantee that all output strings are null-terminated - that is, guarantee that they won't have null characters in the middle of the string? I think yes - for example if a label contains a null in the middle, and we need to print it in an error message, we should sanitize it (either replace the null or cut off the label at the null)

[kainino0x]

Jul 18 meeting:

• Should output strings use WGPUString or is null-terminated const char* OK?
• CF: Yes.
• KN: Think we should guarantee (1) output strings are null-terminated, and (2) don't have nulls in the middle.
• CF: Guaranteeing that is a pain for Rust implementation, we'll have to add the null terminator.
• KN: Ah, was thinking about the bindings problem (Rust->C), didn't think about the implementation problem (C->Rust)
• AE: Think we can do it, just a lot of work to change everything
  • Strings in callbacks (uncaptured error, error scope, lost, buffer map, pipeline)
  • adapter info
  • compilation messages
  • (input) GetProcAddress??
• Change output strings too. Don't guarantee null termination.
  • printf must use %.*s (string with explicit size) to printf with
  • AE: Some compatibility risk because Dawn may always have the null terminator (depending on the implementation of std::string) but wgpu will never have it.
  • LK: Apps would have to do str.data and ignore str.length, so would be somewhat obvious.
  • KN: In C++, string_view?
  • AE: Everything would be a struct that's implicitly convertible to string_view. Pretty sure we can extract that implicit conversion into a separate header too (so the c++ bindings don't have to take a dependency on \<string_view>
• Naming for the struct: “WGPUStringView” ?
  • KN: "WGPUString" looks like it owns the string.
  • AE: SGTM
  • Do it
• String encoding: always utf8? Strings in java are utf16; need to copy and convert all strings
  • AE: I don’t think we actually avoid the copying
  • KN: In #246 we decided to remove the utf-16 offsets from the compilation messages. Though those were only offsets (no actual strings - they were the offsets if the string were to be converted to utf-16) and in dawn we were going to add an extension to keep them.
  • (other discussion): painful to implement generally, most people won't need it, implementation would have to do the conversion anyway so it doesn't really matter if the app does it.
  • Yes, always utf-8.

[Kangz]

Question raised by @toji on a Dawn code review. Could we have a WGPU_NULL_TERMINATED_STRING = SIZE_MAX to better convey the intent?

[kainino0x]

Yes, we should. In my proposal above it was WGPU_STRLEN, but the exact name is not very important.

[Kangz]

@cwfitzgerald @rajveermalviya would WGPU_NULL_TERMINATED_STRING work for you all? (it would be the default in _INIT structs so it doesn't need to be typed out very often).