Skip to content

Commit

Permalink
Removed trailing slashes from folder paths to avoid kernel building u…
Browse files Browse the repository at this point in the history 
…nder Windows (MSVS).
  • Loading branch information
@troky
troky committed Jun 10, 2014
1 parent 9f6bf16 commit cbc2282
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 14 deletions.
6 changes: 5 additions & 1 deletion ocl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -369,8 +369,12 @@ _clState *initCl(unsigned int gpu, char *name, size_t nameSize, algorithm_t *alg
strcpy(build_data->source_filename, filename);
strcpy(build_data->platform, name);
strcpy(build_data->sgminer_path, sgminer_path);
if (opt_kernel_path && *opt_kernel_path)
if (opt_kernel_path && *opt_kernel_path) {
build_data->kernel_path = opt_kernel_path;
}
else {
build_data->kernel_path = NULL;

This comment has been minimized.

Sign in to view

Copy link
@mrbrdo

mrbrdo Jun 10, 2014

Contributor

I believe build_data is calloc'd and this should not be necessary?

This comment has been minimized.

Sign in to view

Copy link
@troky

troky Jun 10, 2014

Author Contributor

check build_kernel.c @ line 65: data->kernel_path is not NULL without above code and line 67 throws an exception.

This comment has been minimized.

Sign in to view

Copy link
@mrbrdo

mrbrdo Jun 10, 2014

Contributor

You're right, it wasn't calloc'd, just alloca'd. Nasty bug, nice that you caught it.
}

build_data->work_size = clState->wsize;
build_data->has_bit_align = clState->hasBitAlign;
Expand Down
11 changes: 5 additions & 6 deletions ocl/build_kernel.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ static char *file_contents(const char *filename, int *length)
if (!f) {
/* Then from `pwd`/kernel/ */
strcpy(fullpath, sgminer_path);
strcat(fullpath, "kernel/");
strcat(fullpath, "/kernel/");
strcat(fullpath, filename);
f = fopen(fullpath, "rb");
}
Expand Down Expand Up @@ -49,7 +49,7 @@ static char *file_contents(const char *filename, int *length)
void set_base_compiler_options(build_kernel_data *data)
{
char buf[255];
sprintf(data->compiler_options, "-I \"%s\" -I \"%skernel\" -I \".\" -D WORKSIZE=%d",
sprintf(data->compiler_options, "-I \"%s\" -I \"%s\\kernel\" -I \".\" -D WORKSIZE=%d",
data->sgminer_path, data->sgminer_path, (int)data->work_size);
applog(LOG_DEBUG, "Setting worksize to %d", (int)(data->work_size));

Expand All @@ -61,7 +61,7 @@ void set_base_compiler_options(build_kernel_data *data)
applog(LOG_DEBUG, "cl_amd_media_ops found, setting BITALIGN");
} else
applog(LOG_DEBUG, "cl_amd_media_ops not found, will not set BITALIGN");

if (data->kernel_path) {
strcat(data->compiler_options, " -I \"");
strcat(data->compiler_options, data->kernel_path);
Expand Down Expand Up @@ -126,10 +126,9 @@ cl_program build_opencl_kernel(build_kernel_data *data, const char *filename)
applog(LOG_ERR, "Error %d: Building Program (clBuildProgram)", status);
status = clGetProgramBuildInfo(program, *data->device, CL_PROGRAM_BUILD_LOG, 0, NULL, &log_size);

char *sz_log = (char *)malloc(log_size + 1);

This comment has been minimized.

Sign in to view

Copy link
@mrbrdo

mrbrdo Jun 10, 2014

Contributor

Are you 1000% about this?

clGetProgramBuildInfo docs are crap, but at least vsnprintf is clear:
The generated string has a length of at most n-1, leaving space for the additional terminating null character.

What this means is, if sz_log is not null terminated (which I don't know if it is), the last char from it will be cut by vsnprintf (from applogsiz).

This comment has been minimized.

Sign in to view

Copy link
@mrbrdo

mrbrdo Jun 10, 2014

Contributor

I don't think *printf is fine with giving it %s and a non-null-terminated string. The docs aren't 100% clear on it, but judging from this answer: http://stackoverflow.com/questions/3767284/using-printf-with-a-non-null-terminated-string it is not safe. So I would just revert to previous code of allocating log_size + 1 chars and setting the last char to 0. Using applogsiz is fine, though, but that only determines the size of the output buffer (which still needs to be the number of chars + 1).
It seems there are no guarantees weather clGetProgramBuildInfo is null-terminated or not, so we need to assume it's not.

Edit: actually printf docs are clear when describing .number: For s: this is the maximum number of characters to be printed. By default all characters are printed until the ending null character is encountered. Perhaps it would not cause issues because the output buffer is limited in this case, but I wouldn't rely on it.

This comment has been minimized.

Sign in to view

Copy link
@troky

troky Jun 10, 2014

Author Contributor

I agree. char *sz_log = (char *)malloc(log_size + 1); should be reverted and then in applogsize with char *tmp42 = (char *)calloc(size + 1, 1); we'll have null termination by calloc-ing size+1 buffer so explicitly null-terminating sz_log isn't neccessary

This comment has been minimized.

Sign in to view

Copy link
@mrbrdo

mrbrdo Jun 10, 2014

Contributor

I also thought of it this way first, but I think it is not safe passing a non-null-terminated string to *printf when using %s, because the specification does imply that the string needs to be null-terminated. So we need to also sz_log[log_size] = "\0";, because if something is not clear in specs then one should assume worst-case (because of different implementations on different systems).

I think doing *tmp42 = (char *)calloc(size + 1, 1); makes sense because someone may assume you need to tell it the size without the null-terminator, so when doing size + 1 there is no chance for mistakes.
char *sz_log = (char *)malloc(log_size);
status = clGetProgramBuildInfo(program, *data->device, CL_PROGRAM_BUILD_LOG, log_size, sz_log, NULL);
sz_log[log_size] = '\0';
applog(LOG_ERR, "%s", sz_log);
applogsiz(LOG_ERR, log_size, "%s", sz_log);
free(sz_log);
goto out;
}
Expand Down
7 changes: 0 additions & 7 deletions sgminer.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7913,10 +7913,8 @@ int main(int argc, char *argv[])
s = strdup(argv[0]);
strcpy(sgminer_path, dirname(s));
free(s);
strcat(sgminer_path, "/");
#else
GetCurrentDirectory(PATH_MAX - 1, sgminer_path);
strcat(sgminer_path, "\\");
#endif

/* Default algorithm specified in algorithm.c ATM */
Expand Down Expand Up @@ -8001,11 +7999,6 @@ int main(int argc, char *argv[])
char *old_path = opt_kernel_path;
opt_kernel_path = (char *)alloca(PATH_MAX);
strcpy(opt_kernel_path, old_path);

This comment has been minimized.

Sign in to view

Copy link
@mrbrdo

mrbrdo Jun 10, 2014

Contributor

this part is not necessary either then.. I added this not long ago because opt_kernel_path could be from config, in which case it is invalid to strcat on it.. but if we don't strcat on it then you can remove above code too

This comment has been minimized.

Sign in to view

Copy link
@troky

troky Jun 10, 2014

Author Contributor

I want to be sure that there is no new bugs introduced with this commit... then I'll remove above code.

This comment has been minimized.

Sign in to view

Copy link
@mrbrdo

mrbrdo Jun 10, 2014

Contributor

Cool. But it should definitely not introduce any bugs if you remove this, because: 954c0e6#diff-ccb83336fe4e8722c874e79c5c4c4dddR8000
#ifdef _MSC_VER
strcat(opt_kernel_path, "\\");
#else
strcat(opt_kernel_path, "/");
#endif
}


Expand Down

0 comments on commit cbc2282

Please sign in to comment.